Adjust function literal return type inference to avoid spurious null

[eernstg]

See dart-lang/sdk#59669 where this topic came up. Thanks to @chiholai-sanasofthk for spotting this issue!

This PR changes one item in the list of actions taken during function literal return type inference: A return; statement only adds Null to the return type in cases where the given function literal is a non-generator.

With the current version, Null is added to the return type also in cases like () sync* { yield 1; return; } such that this function literal gets the inferred return type Iterable<int?>. This is an unnecessary loss of typing precision because null is never actually added to the returned iterable. With this update, the inferred return type is Iterable<int>.

[lrhn]

I really think it should change the return type to void, but I guess that's a bigger change.

[eernstg]

Perhaps that's already covered pretty well?

For function literal return type inference, the type void is given special treatment: If the context type has a return type which is void then the return type of the function literal is also taken to be void.

void main() {
  void Function() f = () { return; }; // Return type of function literal is `void`.
  f = () { return 3; }; // Error, can't return an `int` from a void function.
}

When the return type from the context type is any other type, the inferred return type of the function literal will not be void unless it actually returns an expression of that type.

void main() {
  Object? Function() f = () { return; };
  print(f.runtimeType); // Something that means `Null Function()`.
  f = () { return print('Hah!'); };
  print(f.runtimeType); // Something that means `void Function()`.
}

This means that the function will always be treated as a void function when this is what the context expects, and it will almost always be treated as a non-void function when this is what the context expects.

In the case where a return; statement occurs in a function literal whose inferred return type is not void, we already have other diagnostics:

void main() {
  int? Function(bool b) f = (bool b) {
    if (b) {
      return; // Error: The return value is missing after `return`.
    } else {
      return 1;
    }
  };
}

I think this would already deal with the potential code smells that you were targeting.

[lrhn]

The "for each" seems redundant. How about "if the body contains any return; statement ..."?

[eernstg]

All four items in this list use a similar quantifier (actually, they all use 'for each'). How would it be redundant? We're running an algorithm, and at this point it needs to iterate over the function body to find all occurrences of return .... Do you think it would improve the overall text to use a natural language version of recursion, rather than the iteration-ish "for each"?

[lrhn]

Not: "in the block" is imprecise. It can be read to include the bodies of nested functions, and it can be read as only applying to immediate sub-statements of the block.
Maybe "return; statements returning from this function" or something?

[eernstg]

Right, done!

[lrhn]

Definitely a spec bug, never intended to make any difference to the return type that a generator contains a return.

[eernstg]

Updated, PTAL.

[leafpetersen]

What do the two implementations do? I think from the linked issue, at least the analyzer follows the old spec? So this is a breaking change? No objection to the change in principle, but we need to be sure that it gets tracked as a breaking change, and scheduled for implementation as needed.

[eernstg]

What do the two implementations do? ... the analyzer follows the old spec

Right, the analyzer includes ? in the inferred element type, the CFE does not. This would imply that there is no change in the dynamic semantics, and also that all existing programs where the CFE has been used (that's all programs, I guess?) will pass without errors if the analyzer is updated as proposed here.

Double checking, the following program (based on the current specification) is accepted by the analyzer, but the CFE reports errors as shown:

// ignore_for_file: unused_element

void main() {
  //==================== sync* ===============================

  withOutReturnSync() sync* {
    yield 1;
  }
  withOutReturnSync.expectStaticType<Exactly<Iterable<int> Function()>>;

  withReturnSync() sync* {
    yield 1;
    return;
  }
  withReturnSync.expectStaticType<Exactly<Iterable<int?> Function()>>; // Analyzer OK, CFE error.

  //===================== async* =================================

  withoutReturnASync() async* {
    yield 1;
  }
  withoutReturnASync.expectStaticType<Exactly<Stream<int> Function()>>;

  withReturnASync() async* {
    yield 1;
    return;
  }
  withReturnASync.expectStaticType<Exactly<Stream<int?> Function()>>; // Analyzer OK, CFE error.
}

typedef Exactly<X> = X Function(X);

extension<X> on X {
  X expectStaticType<Y extends Exactly<X>>() => this;
}

[eernstg]

Based on my previous comment, it seems likely to me that nothing will break. So we could run the breaking change process with an attached "but, most likely, nothing will actually break" plus an explanation. Or we could test internally and conclude that it isn't actually breaking if nothing breaks there.