[Breaking Change Request] Inference change when using Null values in a FutureOr context

[leafpetersen]

Summary

Inference works by solving subtype constraints containing free generic method parameters against concrete types. Currently, when solving the subtype constraint Null <: FutureOr<T> for T, our implementations behave divergently: the analyzer produces a solution of Null for T, but the CFE produces dynamic. Both of these are valid solutions, but can have divergent downstream behavior. We propose to make the implementations behave consistently. We expect breakage to be fairly minor.

Example 1: Code which breaks at runtime when using analyzer style inference.

main() {
  new Future<int>.error(42).then((x) {
    throw "foo";
  }, onError: (y) => true);
}

For this example, the analyzer infers the generic type argument to the .then call as Null. When the onError call back is called, the result (true) is cast to FutureOr<Null>, which fails.

The CFE infers the generic type argument to the .then call as dynamic. When the onError call back is called, the result (true) is cast to FutureOr<dynamic>, which succeeds.

Example 2: Code which breaks at runtime when using CFE style inference.

main() {
  var x = new Future.value(3).then((x) => null);
  Future<int> y = x;
}

For this example, the analyzer again infers the generic type argument to the .then call as Null. When running under DDC (which uses the analyzer's inference), the assignment of x to y therefore succeeds.

The CFE infers the generic type argument to the .then call as dynamic. As a result, the assignment of x to y is an implicit downcast, which fails at runtime.

Details

As described above, the core of the issue is that when solving the subtype constraint Null <: FutureOr<T> for T, our implementations behave divergently. The analyzer tries to solve the sub-constraint Null <: Future<T>, notices that it succeeds without producing any useful information, and proceeds to try the sub-constraint Null <: T, which produces a solution which constrains T to Null. The CFE, on the other hand, stops searching for further constraints after checking Null <: Future<T>, since the equation is trivially true under no assumptions on T. More discussion can be found in this issue.

Proposed change

Making the two implementations behave consistently is the paramount concern. Either choice of inference is valid. We propose to change inference in the CFE to match the behavior in the analyzer, since this behavior results in a more generally useful result, and since we seem to see less breakage from making the change in this direction. In particular, as measured on internal code, we see more test failures when changing DDC to use the CFE behavior than when changing the other platforms to use the DDC/analyzer behavior.

Expected impact

Currently, the analyzer and DDC use the analyzer style inference, and dart2js, DDK, the VM, and the AOT compilers all use the CFE based inference. After this change, all platforms will use the analyzer style inference. This can cause both static and runtime breakage in the following situations.

Any code which uses the analyzer and/or runs on DDC, and also runs on any of the CFE based tools will not see any static breakage.

Any code which runs on DDC, and also runs on any of the CFE based tools will not see any runtime breakage.

Code which never uses the analyzer and never runs on DDC, may see breakage. It is possible (but fairly unlikely) that it will see static breakage, but most likely any breakage will be in the form of runtime cast failures. We suspect that the onError pattern in Example 1 above is likely to be the most common failure mode: a Future is created with a callback that has a return type of Null (usually because it throws an error directly or indirectly), and an onError callback which returns some concrete value intended to handle the thrown error. When the onError callback is invoked and the result is cast to Null, a runtime cast failure will result.

Mitigation

In general, static or runtime failures that result from this change will be the result of inference filling in a generic type parameter with Null where previously dynamic was inferred. The previous behavior can always be restored by explicitly passing dynamic as the type argument instead of relying on inference. Concretely, the code from Example 1 would be changed by passing an explicit type argument to the .then method as follows:

main() {
  new Future<int>.error(42).then<dynamic>((x) {
    throw "foo";
  }, onError: (y) => true);
}

With this modification, the code will behave (on all platforms) as it did on the CFE before this breaking change.

[leafpetersen]

cc @stefantsov @johnniwinther @mraleph @vsmenon @Markzipan @aadilmaan @mit-mit @lrhn @eernstg

[aadilmaan]

@matanlurey @dgrove @Hixie

Your review and approval is required for this BREAKING CHANGE.

[leafpetersen]

Emails sent to announce@dart-lang, flutter-dev, and dart-misc .

[Hixie]

I don't have an opinion about the right way this should be implemented, but I agree that consistency seems wise.

[matanlurey]

LGTM, considering I think this has no impact on users we care about.

[dgrove]

LGTM.

[aadilmaan]

Catching up on paperwork. This is approved.
Leaf - Please move this to whomever is making the change.

[leafpetersen]

This has landed in bleeding edge. I will follow up on the mailing list.