Make function literal return type inference infer void from context

[eernstg]

Cf. #1063, in particular #1063 (comment).

This PR changes function literal return type inference to infer void from context (rather than using the bottom-up computed actual returned type when the context type is void or, for an async function, Future<void> and similar types like FutureOr<void>?). This means that the usual diagnostics emitted for void functions will now also be emitted for function literals passed where the context type has return type void/Future<void>.

[Edit, clarification: Note that this change is only applicable with null-safety.]

[scheglov]

When I run my analyzer CL, this breaks many language/ tests.
Do you plan updating them?
Or is this a bug in my implementation?

Note, that there are also failures in Flutter.
I'm updating Flutter.

[leafpetersen]

@scheglov This was intended to be a null safety only change, but in looking at this PR again, I notice it wasn't specified as such. @eernstg we may want to clarify that. I think that may be the cause of the issues in flutter? The language tests look like legitimate breakages, for those you can approve and file an issue (@eernstg do you mind handling updating those)?

@jakemac53 @kevmoo There's a possibility that this change, when fixed and landed (see the CL linked by @scheglov above) will break ported code.

cc @davidmorgan as well for the heads up for when this rolls. Maybe we should consider doing a presubmit with the CL from @scheglov first to see if it will break anything?

[eernstg]

@scheglov wrote:

When I run my analyzer CL, this breaks many language/ tests.
Do you plan updating them?

Yes, certainly. The breakages that I immediately expect are the following (where there is an error with a function declaration due to void as the return type or future value type, and the corresponding error does not occur with a function literal):

async_invalid_return_05_test.dart async_invalid_return_08_test.dart async_invalid_return_11_test.dart  
async_invalid_return_14_test.dart async_invalid_return_17_test.dart async_invalid_return_20_test.dart 
async_invalid_return_52_test.dart async_invalid_return_53_test.dart  async_invalid_return_54_test.dart 
async_invalid_return_55_test.dart async_invalid_return_56_test.dart async_invalid_return_57_test.dart 
async_invalid_return_58_test.dart async_invalid_return_59_test.dart

Any test where a function literal has a context type whose return type has void as the return type or future value type is subject to new constraints, so there could be more.

Comparing this to the CL results, I can see that _5{6,7,8,9}_ are not reported as errors, even though the inferred return type is Future<void> for each of those functions.

Could it be that when the imposed return type schema of an async function literal is FutureOr<void>, the analyzer does not compute the context type for returned expressions to be FutureOr<void>?

@leafpetersen wrote:

@scheglov This was intended to be a null safety only change, but in looking at
this PR again, I notice it wasn't specified as such. @eernstg we may want to clarify that.

Right, the PR changes inference.md, and that document had sentences like 'If the greatest closure of K is S and S is a subtype of Null, then T is Object?' before this PR, so I assumed that any change to that document would be null-safety only.

Looking again, it looks like most of inference.md does not refer to null-safety specific constructs. Do we need to adjust inference.md such that it specifically marks certain paragraphs as valid with null-safety and others as valid without null-safety, and the rest is valid for both?

[eernstg]

Test updates here.

[eernstg]

Implementation issues dart-lang/sdk#42720, dart-lang/sdk#42721.

[davidmorgan]

Thanks. If null safe only it's low risk for google3. If it applies everywhere, yes, we should presubmit :)

[eernstg]

Yep, the change was intended to be applied with null-safety only, and we just need to make sure the documents say so.

Edit: Based on discussions with @leafpetersen, we won't introduce explicit disambiguation (to say: this rule applies with/without null-safety) into inference.md, because there is very little ambiguity in practice.

[leafpetersen]

@eernstg I do still think it's worth calling out this change (in the same way that the change in local function inference is called out).

@davidmorgan are you worried about the possibility that this will break opted in third_party packages during the roll?

[davidmorgan]

Doesn't seem likely--should roll in on Monday.

[eernstg]

it's worth calling out this change

Sounds good, cf. #1102.