Specification: Define 'statically known to not complete normally'

[eernstg]

(Note that this was reported as dart-lang/sdk#35330, which was closed in favor of this one because language and specification issues are in general handled in this repo.)

Until now, the language specification did not specify that it is a compile-time error for a function declaration to have a control flow where the end of the function can be reached (that is, the body 'completes normally', as opposed to other completions like 'returns with a value' or 'throws'), and hence the null object is returned in situations where that is not likely to be intended.

A future enhancement of Dart with nullable types will give further urgency to fix this omission, because it would have to be a run time error to make that attempt to return the null object, and such an error should be detected at compile-time.

The Dart tools have emitted errors for such situations for a long time, we just need to get it specified (and resolve inconsistencies that tools currently exhibit, if any).

Obviously, this is in general an undecidable question, so we would want to specify a conservative approximation: For this function body, it is statically guaranteed that it will not complete normally. A function body that fails this check will be an error, even though it may be semantically correct (because the reasons for not completing normally are too complex for the analysis to recognize).

Here is a proposal from @lrhn about how we might specify such an approximation, that is, how we would conclude that for a statement S, it is statically known that S will not complete normally (and if we cannot conclude that, a compile-time error occurs, except when the return type is among a short list of exceptions):

A statement is known to not complete normally iff:

• It is a return statement.
• It is a "throw statement" (statement expression where the expression is a throw expression).
• It is a rethrow statement.
• It is a break statement.
• It is a continue statement.
• It is a block statement whose last statement is known to not complete normally.
• It is an if statement with an else branch where both branches are known to not complete normally.
• It is a try-finally statement where the finally block statement is known to not complete normally.

Moreover:

• An unlabeled switch with a default is known to not complete normally if every case and the default block are all known to not complete normally, and the switch does not contain any break targeting the switch.
• An unlabeled do-while loop is known to not complete normally if its body is known to not complete normally and it contains no break or continue targeting the loop.
• A for loop, while loop or do-while loop with a condition expression that is a constant expression evaluating to true, or a for loop with no condition, are known to not complete normally if their body contains no break targeting the loop.

The goal will be to have a decision criterion which is not "too stupid" to detect that a particular function body does indeed never complete normally (in which case developers would have to write extra statements like return 0; /* Unreachable! */, in spite of the fact that said statement is "obviously" not reachable), and at the same time the complexity of the analysis must be limited at some point, because the underlying question is undecidable.

[kasperpeulen]

Until now, the language specification did not specify that it is a compile-time error for a function declaration to have a control flow where the end of the function can be reached (that is, the body 'completes normally', as opposed to other completions like 'returns with a value' or 'throws'), and hence the null object is returned in situations where that is not likely to be intended.

It will only be a compile time error only when the return type is non-nullable right? Or must it explicitly return null when the type is nullable? And what about when the type is void?

[lrhn]

It should be an error if the return type is non-void.
There is no soundness issue, this is an attempt to help user catch errors, in particular forgetting to return a value from a non-void function.
Since all functions are still nullable, allowing it for nullable functions would mean that we will never detect the error.

[eernstg]

The rules for valid returns are being integrated into the language specification in this CL. It is true that it is currently a warning rather than an error to declare int f() {}, but it is an error to declare int f() { return; }, which does not seem particularly consistent.

But the point here is that we haven't yet specified how to determine which cases should be flagged, and then we will have to settle on a suitably level (error or warning), too. We might use warnings everywhere because it is not a soundness issue, but that's in any case a discussion that we can take separately from the one about how to specify 'does not complete normally'. Note that we do know the rules need some iterations before they are ready.

[kasperpeulen]

It is true that it is currently a warning rather than an error to declare int f() {}, but it is an error to declare int f() { return; }, which does not seem particularly consistent.

Yeah, that feels quite inconsistent. Good that you guys are going to look at this.
So for non-void functions it is an error to implicitly return null with return;.
But then I would say that for void function, it should be an error to explicitly return null;.

void foo() {
  return null;
}

And only allow the function to return with an empty return; statement.
I also don't understand why this doesn't raise an error:

Future<void> foo() async {
  return 'bar';
}

[eernstg]

.. for void function, it should be an error to explicitly return null;.

We had a lot of iterations on the underlying feature specification, and I don't think we're going to introduce an extra error for return null; in a function returning void. It is a breaking change, and we looked at the breakage when these rules were chosen (and I think null was allowed here exactly because of the actual breakage we saw). Also, at this point we're avoiding breaking changes unless we have a really good reason to ask developers to change their code.

But lints are outside the specification, so it would be much less disruptive to flag that situation as a lint.

The situation is similar for an async function returning Future<void>; again, Null was allowed because of the breakage associated with outlawing it.

[munificent]

Clarifying this would be great. Better checking for missing returns is a real usability improvement. A couple of thoughts:

It is a "throw statement" (statement expression where the expression is a throw expression).

What about cases where the throw is buried in a subexpression? To handle that correctly with short-circuiting operators, we'd need to define whether an expression completes normally too. (throw "blah"); never completes normally, but foo && throw "blah"; might.

It is a block statement whose last statement is known to not complete normally.

Shouldn't this be if any statement directly in the block doesn't complete normally?

{
  return;
  "unreachable";
}

This block still definitely doesn't complete normally.

It is a try-finally statement where the finally block statement is known to not complete normally.

Do we allow finally blocks that don't complete normally? I thought we had restrictions on exiting out of finally since doing so is so weird.

What about try-catch statements where both the try and catch block(s) all don't complete normally?

[kmillikin]

The rules for break, block statements, if, and try/finally need to be tweaked to be correct. All of those can be labeled and so control flow can reach the end in various ways.

[lrhn]

@munificent

What about cases where the throw is buried in a subexpression? To handle that correctly with short-circuiting operators, we'd need to define whether an expression completes normally too. (throw "blah"); never completes normally, but foo && throw "blah"; might.

These rules are kept (relatively) simple.

They do not try to understand expression control flow. If you have an expression statement that is guaranteed to throw, but can do so in several ways, like foo ? throw e1 : throw e2;, then we won't help you. Rewrite it as a statement (you can) or a throw foo ? e1 : e2; expression statement. It's more readable too.

For statement blocks, we assume no unreachable statements. It's simplistic, but shouldn't affect any well written code. If anything, it might tell you that you have unreachable code. With optional semicolons, that might save you from:

    return
      (longName.bar())

(We likely want better dead-code detection for optional semicolon syntax).
We could do more, make the rule more complicated, but why bother?
(The answer would be: because it helps while developing or debugging, so you can insert a return; anywhere, but if that matters, the block would probably still have a final return as well).

@kmillikin
The rules all state either "not labeled" (implicitly because a labeled block statement is not a block statement, it's a labeled statement, and a labeled statement is not mentioned as a statement that can be known to not complete normally, or explicitly), or, for labeled loops, they say "that does not contain a break (and/or continue) targeting this loop/switch" (which means either a break with no label not inside another nested loop, or a break with a label that is labeling that loop).

The "constant true condition" loop case is the only one which should allow a label on the loop (you might need it for a continue).

[stereotype441]

I have two concerns about this line:

• A for loop, while loop or do-while loop with a condition expression that is a constant expression evaluating to true, or a for loop with no condition, are known to not complete normally if their body contains no break targeting the loop.

First, my understanding is that the front end team is planning to implement constant evaluation as a post-processing step after the compilation to kernel. (I believe this is necessary to support constructors like bool.fromEnvironment). If this is the case, then the front end won't know whether the loop condition evaluates to true at the time it needs to analyze whether a statement is "statically known to not complete normally".

Second, some users (myself included) use constants as flags to switch experimental behaviors on and off during development. It would be a shame if switching such a flag caused a compile error.

Speaking for myself as a user, I'd prefer if the definition of "statically known not to complete normally" only took into account literal true, and not constants that equal true.

[bwilkerson]

We attempted to add something similar to the analyzer at one point and got pushback for exactly the reason @stereotype441 pointed out. We eventually just removed the support. There is at least one issue in the sdk repository related to this if you'd like to see older discussions of this topic.

[eernstg]

the front end won't know whether the loop condition evaluates to true
at the time it needs to analyze whether a statement is "statically known
to not complete normally".

Due to the element of undecidability, we know that this analysis will have to be a conservative approximation. With that in mind, I think it would make sense to use a conservative approximation also for these boolean expressions, which means that we should report an error if there exists a boolean value such that there would be an error.

This would also make sense from a software engineering point of view:

int foo() {
  while (bool.fromEnvironment("whatever")) {
    ...
    if (...) return 42;
    ...
  }
}

We should take into account that the boolean value could be false, so we should also emit a diagnostic message saying that it is possible for this code to return the null object implicitly, because we might fall out at the end of the function body.

After all, if there are some compilations where this problem actually occurs then there is no other location where the developer can fix the problem than in the code (there is no such thing as "the code for just that compilation where the problem occurs"), so it makes sense to tell the developer that this code is wrong, as seen from a static analysis that must cover both situations.

older discussions of this topic

@bwilkerson, maybe you could add a reference to those older discussions? They didn't come up with a few searches that I just tried.

[bwilkerson]

Sorry, but I can't find it/them either (I've never had much success finding issues in Github). So I'll summarize my recollection of the conversation.

Analyzer had checking that looked at the values of conditionals and generated a diagnostic if the value would prevent the body of the statement from being executed. Doing this for literal values, while relatively useless, was not contested, but Peter pointed out that it isn't uncommon for developers to write code of the form:

if (debug) {
  ...
}

where debug is a static field somewhere with a knowable value. In those cases it is counter-productive to tell the user that the code inside the if won't be executed when the flag is set to false because they already know this and intended it to be that way.