Split-init allowed past guaranteed-exit control flow

[DanilaFe]

In production, this program compiles just fine:

proc test() {
  var x;
  if true then
    return;
  x = 1;
}

but the following program does not:

proc test() {
  var x;
  if true then
    return;
}

I find this surprising, because, as far as Chapel is concerned, the code below return doesn’t happen. This is particularly true when we use early-returns to avoid compilerError:

proc bla() {
  if true then return;
  compilerError("Unexpected condition!"); // does not error
}

combining all this, I think we end up with odd behavior in the following case:

proc test() {
  var x;
  if true then
    return;
  compilerWarning("no");
  x = 1;
}

I believe that we should emit an in both cases about missing split-initialization for x (whether x = 11 is present or not). We should also clarify the specification to say that split init is not allowed across returns.

[bradcray]

Thinking about this a bit more over the weekend, I'm increasingly convinced that the first two programs should be equivalent to:

proc foo() {
  var x;
}

foo();

which generates the reasonable error in production ATO:

/ATO/code.chpl:1: In function 'foo':
/ATO/code.chpl:2: error: 'x' is not initialized and has no type
/ATO/code.chpl:2: note: cannot find initialization point to split-init this variable

[mppf]

Spec says

[split init applies through a conditional if] every branch without an applicable assignment statement always returns or throws. Note that an if statement written without an else is considered to have an empty else branch. Similarly, a select without an otherwise is considered to have an empty otherwise.

Here is an example:

proc foo() {
  var x;
  if isOK { // assuming isOK is not param here
    x = createSomeValue();
  } else {
    return false;
  }
  // do something with x
  return true;
}

It's similar if, instead of return false, it is a throw (or propagating a throw from a call to a thowing function).

This other example is similar and also intended to work AFAIK:

proc foo() {
  var x;
  if !isOK { // assuming isOK is not param here
    return false;
  }
  x = createSomeValue();
  // do something with x
  return true;
}

As a result, I would suggest solving the problem differently from this suggestion in the OP.

I believe that we should emit an in both cases about missing split-initialization for x (whether x = 11 is present or not). We should also clarify the specification to say that split init is not allowed across returns.

proc test() {
  var x;
  if true then
    return;
  x = 1;
}

IMO this should compile regardless of whether the condition is param. That's exercising an intentional feature of split-init that is documented in the spec.

Regarding this:

proc test() {
  var x;
  if true then
    return;
}

I think we should focus on making decisions about this even simpler program as a starting point:

proc test() {
  var x;
  return;
}

It behaves similarly (doesn't compile with the same error).

My preference would be to keep this an error, of one sort or another. Conceptually, we might consider this legal for purposes of split-init. (Why would we do that? because it would make the rules have a nicer separation of concerns between conditionals and other code). After deciding x is split inited, we could consider it an error because there is a variable that is not initialized on any path through generated function. Or, we could leave it as it is now: the current implementation makes it an error because compilation can't proceed if we have a variable of unknown type.

It would be OK with me to make that a warning rather than an error (since we can certainly compile it and just ignore that var x was there at all; similarly we probably want to have unused variable warnings in the future for variables that are not used after they are initialized). If we wanted to move it to a warning, we would issue the warning and then remove the variable of unknown type.

Note that we have discussed some odd behavior around split init + param conditionals before. See #14996.

[DanilaFe]

IMO this should compile regardless of whether the condition is param. That's exercising an intentional feature of split-init that is documented in the spec.

My understanding is that this conflicts fundamentally with other parts of the compilation process. E.g., we frequently use an early-return pattern + compilerError in our module code. My final example above is one such instance. But if we do some special-case processing in a param conditional, then return early to avoid resolving subsequent (non-working code), we can't also expect that code to be resolved for the purposes of split-init.

E.g.:

proc foo(type t) {
  var x;
  if requiresSpecialHandling(t) {
    return handleSpecially(t);
  }

  x = handleNormally(t); // will throw a compiler error if `requiresSpecialHandling` is true
}

Currently, we do not resolve code after early returns, because early returns often do type-sensitive processing and rule out cases in which code after them will not work. However, determining the type of x via "dead" code would require doing so. I'm not sure how to reconcile these in a principled manner.

[mppf]

IMO this should compile regardless of whether the condition is param. That's exercising an intentional feature of split-init that is documented in the spec.

Currently, we do not resolve code after early returns, because early returns often do type-sensitive processing and rule out cases in which code after them will not work. However, determining the type of x via "dead" code would require doing so. I'm not sure how to reconcile these in a principled manner.

To be clear, what I meant was, "it should count as split init regardless...". See more below.

We could just determine x is dead and forget about doing anything with it. Leave it with unknown type.

I would be OK with making it be an error to have a variable that's split-init when the initialization point can never be reached in the generated code. I think this needs to be an error about the variable being unused & not exactly one about split init.

proc test() {
  var x;
  return;
}

currently an error, I think it should stay an error, but a different error. I think the error should say something like "You have a variable x which is never initialized and cannot be used".

proc test() {
  var x;
  if true then
    return;
}

currently the same error. I think it should get the same treatment: "You have a variable x which is never initialized and cannot be used".

proc test() {
  var x;
  if true then
    return;
  x = 1;
}

currently compiles, but it would be reasonable to make it an error because x is never initialized in the generated function (IMO this shouldn't be a split-init error; as a result it would fail to compile for the same reason as var x; return;: "You have a variable x which is never initialized and cannot be used".)

Depending on what we learn if we try to implement that, we should consider ignoring var x; once we establish that it is never initialized / is dead. In particular, if we discover some important use cases of this pattern in testing, I think we should think about viewing it as a kind of dead-code-elimination to forget about x.

[bradcray]

Another simple program we should consider is:

proc test() {
  var x;
  return;
  x = 1;
}

which passes today, unfortunately for the stance I was taking above, as that suggests that perhaps

proc test() {
  var x;
  if true then return;
  x = 1;
}

should as well. Breaking code post-2.0 questions aside, my instinct would still be to make both of the above illegal, similar to what Michael's saying here:

I would be OK with making it be an error to have a variable that's split-init when the initialization point can never be reached in the generated code.

(which seems like a change from what you said previously, is that right Michael?)

IMO this should compile regardless of whether the condition is param.

[mppf]

(which seems like a change from what you said previously, is that right Michael?)

Yes,

what I meant was, "it should count as split init regardless...".