Analyzer: Implementation of new static analysis for return statements

[eernstg]

The null-safety specification updates in dart-lang/language#941 and dart-lang/language#948 have been unfolded as part of the migration of tests/language/invalid_returns. This issue is concerned with updating the analyzer correspondingly.

The following tests have a missing compile-time error:

language/async/return_types_test
language/invalid_returns/async_invalid_return_48_test
language/invalid_returns/async_invalid_return_50_test
language/invalid_returns/async_invalid_return_51_test
language/invalid_returns/sync_invalid_return_27_test
language/invalid_returns/async_invalid_return_00_test
language/invalid_returns/async_invalid_return_03_test
language/invalid_returns/async_invalid_return_04_test
language/invalid_returns/async_invalid_return_05_test
language/invalid_returns/async_invalid_return_08_test
language/invalid_returns/async_invalid_return_11_test
language/invalid_returns/async_invalid_return_14_test
language/invalid_returns/async_invalid_return_17_test
language/invalid_returns/async_invalid_return_20_test
language/invalid_returns/async_invalid_return_23_test
language/invalid_returns/async_invalid_return_24_test
language/invalid_returns/async_invalid_return_25_test
language/invalid_returns/async_invalid_return_32_test
language/invalid_returns/async_invalid_return_33_test
language/invalid_returns/async_invalid_return_34_test
language/invalid_returns/async_invalid_return_35_test
language/invalid_returns/async_invalid_return_36_test
language/invalid_returns/async_invalid_return_37_test
language/invalid_returns/async_invalid_return_41_test
language/invalid_returns/async_invalid_return_45_test
language/invalid_returns/async_invalid_return_47_test
language/invalid_returns/async_invalid_return_52_test
language/invalid_returns/async_invalid_return_53_test
language/invalid_returns/async_invalid_return_54_test
language/invalid_returns/async_invalid_return_55_test
language/invalid_returns/async_invalid_return_56_test
language/invalid_returns/async_invalid_return_57_test
language/invalid_returns/async_invalid_return_58_test
language/invalid_returns/async_invalid_return_59_test

The main reasons for the above is that the rules about return; are now more strict, and a void expression can no longer be returned in an async function with return type Future<Null> (and several similar types).

The following tests incur some compile-time errors that shouldn't be there:

language/invalid_returns/async_valid_returns_test
language/async/return_types_test

The reason for this is that the old rules required Future<flatten(S)> to be assignable to the return type where S is the type of the returned expression, and the new rules require S to be assignable to the future value type of the function, or flatten(S) to be a subtype thereof.

[scheglov]

Should these new rules be applied for both legacy and opted-in libraries, or only to opted-in libraries?

[eernstg]

Only for code with null-safety (this is also the reason why these tests are in language, and the language_2 tests have not had a similar update).

[scheglov]

I have an issue with this code (language/invalid_returns/async_invalid_return_35_test):

import 'dart:async';

FutureOr<Object?> Function(void) f = (void a) async {
  return a;
  //     ^
  // [analyzer] unspecified
  // [cfe] unspecified
};

For function expressions we need to infer the return type. I'm using these rules.

Imposed type: FutureOr<Object?>.
Context type: FutureOr<Object?>.
Returned types: [void].
Actual returned type: void.

For this rule: Let T be the actual returned type of a function literal as computed above. Let R be the greatest closure of the typing context K as computed above. If T <: R then let S be T. Otherwise, let S be R. Here T = void, R = FutureOr<Object?>, and T <: R by Left Top rule from the subtyping spec.

So, the inferred type of the closure is Future<flatten(S)> = Future<void>.

Then later, when we check return a; with type void, we have T_v = void and flatten(S) = void, and it is not an error. So, the test fails.

@eernstg @leafpetersen Is there a mistake in my calculations? Is this an unexpected interaction between closure return type inference and null safety?

[leafpetersen]

@scheglov Good catch, I agree with your analysis.

This also raises another issue however. I believe that the change to use futureValueType should also be applied to function literal downwards inference. In particular, I believe that the line

Otherwise the context type is FutureOr<flatten(T)> where T is the imposed return type.

Should be replaced with

Otherwise the context type is FutureOr<futureValueType(T)> where T is the imposed return type.

with a suitable definition of futureValueType.

@eernstg can you follow up?

[scheglov]

@eernstg ping

[leafpetersen]

@eernstg I think the test that @scheglov mentions above is incorrect, can you look into it?

[eernstg]

Sorry, don't know why I missed this one. Looking into it now.

[eernstg]

Just created dart-lang/language#989 to adjust inference.md. Here is the analysis again:

language/invalid_returns/async_invalid_return_35_test:

import 'dart:async';

FutureOr<Object?> Function(void) f = (void a) async {
  return a;
};

Rules are here.

Imposed return type schema: FutureOr<Object?>.
Context type: FutureOr<Object?>, so there's no difference here.
Returned types: [void].
Actual returned type: void, also the same.

So the analysis still holds, yielding an inferred return type for the function literal which is Future<void>, and the check on the type of the returned expression is still not an error.

Created https://dart-review.googlesource.com/149163 where the test is updated such that it does not expect an error.

[scheglov]

I'm sorry for not being explicit about this, but the test that you reference is just one of the group of tests looked to me as being similar, and exercise function expressions inference and return types validation.

Once I found this, I decided that this is something more serious with tests, and just chosen randomly one test to explain why it did not look right to me. I don't exclude that some of them might fail by a different reason, but random sampling gives me similar tests.

I can try to implement the change to the spec.
But reading your comment it seems that this would not change the result of analysis.

FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_53_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_47_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_11_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_05_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_24_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/sync_invalid_return_27_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_41_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_55_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_08_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_58_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_17_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_36_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_52_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_valid_returns_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_03_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_25_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_54_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_04_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_59_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_23_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_37_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_51_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_45_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_32_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_57_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_48_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_20_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_34_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_50_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_33_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_56_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_00_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_35_test
FAILED: dart2analyzer-none release_x64 language/invalid_returns/async_invalid_return_14_test

[eernstg]

The change to the spec is needed in order to handle situations like this one:

Future<int>? Function() f = () async => Future<int?>.value(null);

If we use FutureOr<flatten(Future<int>?)> = FutureOr<int?> as the context type then the returned expression is accepted and the return type of the function literal will be Future<int?>. But this means that the initialization of f is an error. We need to find FutureOr<int> as the context type and make the returned expression an error inside the function literal. The underlying principle is that we don't infer some types and then complain that they are wrong.

So this change will be needed in any case.

It's getting late, so I'll look at the long list of tests tomorrow. ;)

[eernstg]

About tests/language/invalid_returns/async_valid_returns_test.dart:

There are 12 errors.

STATIC_TYPE_WARNING.RETURN_OF_INVALID_TYPE where the returned expression has type dynamic amounts to 8 of them, and this should not be reported as an error. This follows from the requirement that the type of the returned expression (dynamic) must be assignable to the future value type of the enclosing function (which can be anything, because dynamic is assignable to anything). So this is a consequence of switching to use the future value type rather than flatten for these return statement checks.

The remaining 4 cases return a Future<int> in a function whose future value type is Future<int> (and the return type is Future<Future<int>>). This should be allowed, because T is assignable to T for any T. This is again a consequence of switching to future value type based checks.

So these test failures are all working as intended.

[eernstg]

About sync_invalid_return_27_test.dart: This error is legitimate. It is a soundness violation to allow returning the value of an expression of type void with return type Null. Cf. #42093.

The errors in 'async_invalid_return_47_test.dart' and 'async_invalid_return_50_test.dart' have the same nature.

[eernstg]

CL https://dart-review.googlesource.com/c/sdk/+/149163/ changes the following tests:

tests/language/invalid_returns/async_invalid_return_00_test.dart
tests/language/invalid_returns/async_invalid_return_03_test.dart
tests/language/invalid_returns/async_invalid_return_04_test.dart
tests/language/invalid_returns/async_invalid_return_05_test.dart
tests/language/invalid_returns/async_invalid_return_08_test.dart
tests/language/invalid_returns/async_invalid_return_11_test.dart
tests/language/invalid_returns/async_invalid_return_14_test.dart
tests/language/invalid_returns/async_invalid_return_17_test.dart
tests/language/invalid_returns/async_invalid_return_20_test.dart
tests/language/invalid_returns/async_invalid_return_23_test.dart
tests/language/invalid_returns/async_invalid_return_24_test.dart
tests/language/invalid_returns/async_invalid_return_25_test.dart
tests/language/invalid_returns/async_invalid_return_32_test.dart
tests/language/invalid_returns/async_invalid_return_33_test.dart
tests/language/invalid_returns/async_invalid_return_34_test.dart
tests/language/invalid_returns/async_invalid_return_35_test.dart
tests/language/invalid_returns/async_invalid_return_36_test.dart
tests/language/invalid_returns/async_invalid_return_37_test.dart
tests/language/invalid_returns/async_invalid_return_47_test.dart
tests/language/invalid_returns/async_invalid_return_48_test.dart
tests/language/invalid_returns/async_invalid_return_53_test.dart
tests/language/invalid_returns/async_invalid_return_55_test.dart
tests/language/invalid_returns/async_invalid_return_58_test.dart
tests/language/invalid_returns/async_invalid_return_59_test.dart

The difference is that the expectation to get an error has been removed from those function literals whose inferred return type is different from the type of the variable that it is assigned to, in such a way that the error doesn't arise. The errors on the regular functions have not changed.

A couple of variables of type void were changed from having the value null to having some other value, to underscore the fact that it is not guaranteed for an expression of type void to have the value null. A corresponding change was made for a couple of variables of type Future<void>, FutureOr<void>, Future<void>?, etc.

So the problems with the tests were concerned with function literals and their inferred return types, and the remaining test failures look legitimate to me.

[scheglov]

I will leave comment for changed tests in the CL that changes them, but there are issues for invalid_returns/async_valid_returns_test.dart that was not changed, so I cannot comment on it.

1. Return without a value into dynamic in async.

dynamic f() async {
  return;
}

We apply this rule:

It is a compile-time error if $s$ is \code{\RETURN{};},
unless $T_v$
is \VOID, \DYNAMIC, or \code{Null}.

Here T = dynamic, T_v = Object?.
So, T_v is not void, dynamic, or Null.
So, the error.

2. The same for dynamic? declared return type. The ? does not change anything in analyzer, we treat dynamic as always nullable. So, the same error.

3. Return void into dynamic in async, lines 285 - 312.

void vv = null;
dynamic async_void_to_dynamic_e() async => vv;

Similar to (1), T = dynamic, T_v = Object?, S = void.

We apply this rule:

It is a compile-time error if $s$ is \code{\RETURN{} $e$;},
$T_v$ is neither \VOID{} nor \DYNAMIC,
and \flatten{S} is \VOID.

So, the error.

[eernstg]

Return without a value into dynamic in async.

That's a good catch! I agree on the analysis, the rules imply that it should be flagged as an error. However, I did not consider that case carefully enough, because it seems inconsistent with the treatment of the type dynamic to have this error.

The rule futureValueType(void) = void was introduced exactly in order to ensure that when we have a fire-and-forget async function then the object that is used to complete the returned future should be considered discarded, exactly like it is discarded when the return type is Future<void> (also, the return type void has an additional effect, namely that the caller cannot await the completion of the invocation). So a return type of plain void is treated as an "extended version" of the return type Future<void>.

Similarly, the return type dynamic could be treated as an extended version of the return type Future<dynamic>, in which case we should allow dynamic f() async { return; }.

We would then add an extra rule, futureValueType(dynamic) = dynamic.

The same for dynamic? declared return type.

Maybe, maybe not, we don't actually give void? the same treatment as void (at least, not unless we rely on normalization to eliminate the ?, and normalization isn't applied eagerly everywhere).

Return void into dynamic in async

This would be covered by a new rule futureValueType(dynamic) = dynamic as well.

[scheglov]

Thank you, I added the new rule for futureValueType(dynamic) = dynamic and ran bots for the CL that implements new rules for null safety. With your CL for updating tests these tests started to pass.

I also made a small fix to the legacy rules.
Unfortunately it seems this breaks existing code.
For example in analysis_server.
And Flutter.

An example:

// @dart = 2.7
Future<void> start(Future<List<void>> v) async {
  return v;
}

The change that I made to legacy implementation is what the spec says, I think?
CFE does not report any error on this code, just like analyzer used to not report.

Is this my misunderstanding of the spec, or a real bug in our implementation?
What should we do - leave the old implementation, or fix Flutter and other pkg/ code?

I have a feeling that I have already asked about this or similar return type issue.

[eernstg]

Considering this example:

// @dart = 2.7
Future<void> start(Future<List<void>> v) async {
  return v;
}

It is true that we have had the rule which requires this error for years (since SDK commit 5d8501a, Dec 2018), but for a long time we hadn't detected that it wasn't enforced.

So for the pre-null-safety code we will have to consider this as a known bug and continue to omit reporting it, because the fix is a breaking change.

But with null-safety we can take the step to report this kind of error.

In any case, the rule is based on the observation that the following two functions have a very similar overall dataflow, even though they are of course different with respect to the detailed control flow and typing:

void f1() {
  return 3;
}

Future<void> f2() async {
  return 3; // Or `return Future.value(3);`.
}

void main() {
  f1(); // Discard 3.
  await f2(); // Discard 3.
}

In both cases, a computation yields 3, and it is passed on to a caller. However, the type void in both cases serves to indicate that this object should be discarded, and the rule about "returning non-void to void" for regular (synchronous, non-generator) functions is then mirrored in the async case in the rule that you mention, such that the developer who is working on the body of f2 will be notified if they attempt to discard the value of a non-voidy expression via a return statement.

[scheglov]

Thank you for the explanation. I understand this as we never going to report this error in legacy libraries, but will report a similar error in migrated libraries.