Include normal completion during return type inference?

[eernstg]

[Edit: A summary of the issues brought up can be found here.]

Cf. dart-lang/sdk#42568, the analyzer and the CFE disagree on some programs with respect to inference of the return type of a function literal.

The difference in behavior fits the following understanding: The analyzer takes normal completion into account (which means that the 'actual returned type' is considered to be nullable when the body of the function can complete normally), but the CFE (and the current rules) do not include that step.

[Correction, July 6th: The current rules do include a step where normal completion includes Null in the return type.]

It may seem reasonable to include this step, because it allows function literals to complete normally when that's convenient, and the result is simply that the return type will be nullable, which is sound.

However, we do not otherwise equip normal completion with such powers:

void main() {
  var b = true;
  f() {
    if (b) return 1;
  }
  g() {
    if (b) return 1;
    if (!b) return null;
  }
}

Here, f is an error because the inferred return type is int, and this makes normal completion of the body an error. However, g is accepted, because return null; makes the inferred return type nullable, in which case it is not an error to reach the end.

So I'd recommend that we keep the current rules and ask the analyzer to report this error. [Correction July 6th: The current rules are actually a bit confusing, because the function literal gets a nullable return type if the body can complete normally, but a local function seems to omit that step.]

(We have otherwise considered new errors from the analyzer to be a breaking change, but it seems likely that the CFE is used in so many contexts today that it is very difficult to write Dart software which is being used for anything and which avoids seeing an error from the CFE. In other words, it seems reasonable to add the rule that it is also not a breaking change to report a new error in the analyzer if the CFE already reports that error.)

@lrhn, @leafpetersen, @natebosch, @stereotype441, @jakemac53, @munificent, WDYT?

[stereotype441]

(We have otherwise considered new errors from the analyzer to be a breaking change, but it seems likely that the CFE is used in so many contexts today that it is very difficult to write Dart software which is being used for anything and which avoids seeing an error from the CFE. In other words, it seems reasonable to add the rule that it is also not a breaking change to report a new error in the analyzer if the CFE already reports that error.)

Just to be clear: I think the change you're proposing is: the analyzer should follow the spec as written and infer the return type of the closure based solely on explicit return statements; it should not act as though there is an invisible return null at the end of the function for purposes of type inference. In which case, a closure like this one:

var f = () {
  if (...) return 0;
};

would have an inferred type of int Function(), so it would result in a compile-time error in an opted-in library, due to the rule that functions that complete normally must have a nullable return type.

Assuming I've got that right, it sounds reasonable to me, and I have no worries about whether or not this is a breaking change, because there's no effect on opted out code.

[nshahan]

In my opinion being forced to be explicit about when a function returns null agrees with the goal of the null safety feature.

@stereotype441 Would this be a problem for the migration tool? Would it change "normal completions" to an explicit return null in the cases where there are other return statements that return values of a non-nullable type?

@eernstg What should the inferred type of a function be with no return statement?

main() {
  h() {
    print('hello');
  }
}

I believe now the inferred type of h() is Null Function().

[stereotype441]

@stereotype441 Would this be a problem for the migration tool? Would it change "normal completions" to an explicit return null in the cases where there are other return statements that return values of a non-nullable type?

Currently it does nothing to such functions, so they would become errors that the user could trivially fix (either pre- or post-migration) by adding a return null. If that's too annoying for people (and it likely will be) we can add the feature of having the tool auto-insert a return null when needed--it wouldn't be too difficult to fit into the existing infrastructure.

[eernstg]

@stereotype441 wrote:

Just to be clear ...

Exactly, I'm proposing what you describe. However, I just noticed that there is a rule which changes the return type to a nullable type when the body of a function literal can complete normally. This means that the analyzer follows the rules, and it is the error from the CFE which is outside the rules. In any case, it is confusing that the function literal and the local function get a different treatment, and we need to make sure that our tools agree on something.

@nshahan wrote:

What should the inferred type of a function be with no return statement?

I agree that the current rules give the example function h the type Null Function().

This puts the spotlight on a tricky case: It is difficult to recognize which function literals should have the inferred return type void. As long as the function literal is allowed to obtain a more special return type based on its returned expressions (and all types are subtypes of void, so anything whatsoever will do), we are giving up on the errors that we otherwise emit when a function returns "something to void that looks meaningful":

void main() {
  void f(void g()) => g();
  f(() { return 42; }); // No problem! Function literal has type `int Function()`.

  // The above situation is similar to a local function with declared return type `void`.
  void h() {
    return 42; // Error: Can't return an `int` when the return type is `void`.
  }
}

Of course, this is an example of the higher-order version of the rules that prevent using the result of evaluating an expression of type void (so Object? x = print(""); is an error). We usually called this 'voidness preservation', and it would flag the assignment of an int Function() to a variable of type void Function() as an error, because that assignment would imply that the returned value is discarded whenever that function is called. This is the reason why h is similar to the situation where () { return 42; } is passed to f.

Surely we can't do this (because it would break existing code), but it might make sense to infer the return type of a function literal as we do today in all cases except when the imposed return type schema is the plain type void. This would make a function literal a "void function" whenever the caller has announced that this is how the function will be used, and then we will get those errors where a non-voidy expression is returned "to void".

[lrhn]

If we allow:

int? foo(bool b) {
  if (b) return 42;
}

which it seems we do (according to nullsafety.dartpad.dev both in the CFE and analyzer), then I don't think we should disallow:

var foo = (bool b) {
  if (b) return 42;
}

because we choose to not to infer the same type, even though we know perfectly well that it's the only type that would make it valid.

I'd be all for disallowing both, but that's not what we did. That was likely because it would break too many things. but then that argument should apply to the function literal as well.

(I want return; and fall-through to be invalid in any function with a return type different from dynamic, void or FutureOr^n<void>. No exception for Null or nullable types.)

[eernstg]

Here's a summary of the situation:

• The analyzer does take normal completion into account when it infers a function return type, and uses it to make the inferred return type nullable. This is consistent with the spec.
• The front end does not do this, and reports an error that the function may return null due to normal completion.

I was only comparing local functions and function literals, because they are the only functions whose return type can be obtained by inference.

@lrhn, I read your comment as a statement of support for keeping the rules unchanged. We would then modify the CFE to take normal completion into account as well (such that the error is no longer reported).

@nshahan and @stereotype441, I read your comments as leaning toward the approach taken by the CFE, noting that it is not particularly null-safe to allow normal completion to implicitly add Null to the return type. We would then modify the analyzer and the spec accordingly.

The special case where the function literal is assigned to a variable of type void Function(...) (or it's async and is assigned to T Function(...) where flatten(T) == void) came up. This special case is handled with the current rules, but it might be handled in an even more helpful manner if we would infer the return type void (and Future<void> etc.) from the context type (rather than a subtype obtained from the return statements), because we would then get the normal diagnostics about "returning non-trivial things to void".

With that I'm leaning towards the following [edit: clarified the options]:

1. Avoid rocking the boat: Keep the rules about normal completion, change the CFE to take normal completion into account; or

2. More radically: Keep the behavior of the CFE (such that normal completion won't implicitly make the return type nullable); and use void as the inferred return type when that's the imposed return type schema of a synchronous non-generator function, and use the greatest closure of the imposed return type schema for an async function when the future value type of that schema is void.

[stereotype441]

@nshahan and @stereotype441, I read your comments as leaning toward the approach taken by the CFE, noting that it is not particularly null-safe to allow normal completion to implicitly add Null to the return type. We would then modify the analyzer and the spec accordingly.

Yeah, that's probably an accurate summary of my comment, though thinking about it a day later I'd be equally happy adjusting the CFE to match the analyzer.

With that I'm leaning towards (1) keep the rules about normal completion, change the CFE to take normal completion into account (just to avoid rocking the boat), and (2), possibly, use void/Future<void> as the inferred return type when that's the imposed return type schema.

SGTM

[nshahan]

@natebosch I just realized this conversation might have been for you as a language team member.

[nshahan]

@nshahan and @stereotype441, I read your comments as leaning toward the approach taken by the CFE, noting that it is not particularly null-safe to allow normal completion to implicitly add Null to the return type. We would then modify the analyzer and the spec accordingly.

I agree with your interpretation of my comment but it's not clear to me that it would be feasible with our backwards compatibility requirements.

In general I'd like to see the Null return types becoming as explicit as possible. IMO the inferred return types of functions has always been confusing to me as a dart user. It's been hard to train myself that top level inference is different, implicit null returns, etc.

[eernstg]

@natebosch I just realized this conversation might have been for you as a language team member.

That's true—sorry about the mistake.

About the compatibility issue: I think it is a special situation when the CFE or the analyzer reports an error, because it is difficult to work with any Dart software without encountering the error message from one or the other. So if we want to make Null very explicit, we may be able to do that without much actual breakage.

[munificent]

If we allow:

int? foo(bool b) {
  if (b) return 42;
}

which it seems we do (according to nullsafety.dartpad.dev both in the CFE and analyzer),

In this example, the user has explicitly written int? so they have communicated both to the language and anyone reading the code "Hey, we all know null can happen here."

then I don't think we should disallow:

var foo = (bool b) {
  if (b) return 42;
}

because we choose to not to infer the same type, even though we know perfectly well that it's the only type that would make it valid.

This code could either mean "I want the return type to be nullable" or "I forgot a return statement". The language and anyone reading the code don't have much evidence to know which of those is what the author had in mind. In the absence of that, I think it's better for this to be an error so that the author has to clarify their intent.

[lrhn]

Te problem is that you can't clarify your intent by adding a return type to a function literal. You have to rewrite the function, which means that the same function body is valid in one place and invalid in another, only because we choose to not infer the type that would make it valid.
That's an inference issue.

With null safety, I'm not so worried about inferring the "wrong thing", because the user will have to deal with the nullable return type. It won't just accidentally slip a null value into your code without you noticing like it used to.