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]

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]

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.

[eernstg]

I think we should be able to proceed with this one.

Checking again, the following program expects the behavior which is being specified if this PR is landed, and it is compiled and executed without errors by the CFE; however, the analyzer behaves according to the current specification (and would need to be adjusted if and when this is landed).

// ignore_for_file: unused_element

void main() {
  withoutReturnSync() sync* {
    yield 1;
  }
  withoutReturnSync().expectStaticType<Exactly<Iterable<int>>>();

  withReturnSync() sync* {
    yield 1;
    return;
  }
  withReturnSync().expectStaticType<Exactly<Iterable<int>>>(); // The analyzer fails here.

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

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

  withReturnAsync() async* {
    yield 1;
    return;
  }
  withReturnAsync().expectStaticType<Exactly<Stream<int>>>(); // The analyzer fails here.

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

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

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

Given that the CFE already follows the proposed specification (at compile time and run time), no programs should break if we land this and adjust the analyzer.

[eernstg]

@dart-lang/language-team, WDYT, can we proceed to land the spec, test the breakage on internal code (no breakage is expected), and adjust the analyzer (if it is true that no breakage is observed)?

[natebosch]

If the CFE already implements the expected behavior I agree that it should not cause problems to roll this out, and using internal code as a double check sounds good to me.