NNBD: Future flattening for Future<Never>works in different ways with dart and analyzer

[iarkh]

Dart VM version: 2.8.0-dev.20.0 (dev) (Fri Apr 3 10:19:55 2020 +0200) on "windows_x64"

Analyzer throws compile error on the lines 5 and 6 with the following source code:

import "dart:async";

dynamic getNull() => null;

Future<Never> test1() async => await getNull();
FutureOr<Never> test2() async => await getNull();
Never test3() => getNull();

main() {
  test1().then((value) { print(value); });
}

Please not that there is no exception for the line 7 here.

Dart does not throw compile error, runtime exception appears here instead.

I am not sure which behavior is correct, but definitely dart and analyzer should run in the same way here.

Sample output is:

$> dartanalyzer --enable-experiment=non-nullable test.dart
Analyzing test.dart...
error - A value of type 'dynamic' can't be returned from function 'test1' because it has a return type of 'Never'. - test.dart:5:32 - return_of_invalid_type
error - A value of type 'dynamic' can't be returned from function 'test2' because it has a return type of 'Never'. - test.dart:6:34 - return_of_invalid_type
2 errors found.

$> dart --enable-experiment=non-nullable --null-safety test.dart
Unhandled exception:
type 'Null' is not a subtype of type 'Future'
#0 test1 (file:///D:/DART/sdk/tests/co19/src/LanguageFeatures/nnbd/test.dart:5:32)

#1 main (file:///D:/DART/sdk/tests/co19/src/LanguageFeatures/nnbd/test.dart:10:3)
#2 _startIsolate. (dart:isolate-patch/isolate_patch.dart:301:19)
#3 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

[eernstg]

I believe this is not fully specified.

I also believe that we should be able to decide that line 7 is an error, cf. dart-lang/language#913 (looking at the example, I consider this one to be line 7: Never test3() => getNull();).

Lines 5 and 6 are currently already an error according to the language specification rules about return statements (here), but that's an anomaly (it happens because Future<dynamic> is not assignable to Future<Never> with null-safety enabled, but since dynamic is assignable to Never it makes regular and async functions different from each other in a way that is not well motivated), so I've suggested that we should change the rules such that lines 5 and 6 are no more an error, cf. dart-lang/language#914. That, however, may involve some more discussions, which is the reason why I treat these two topics separately.

[eernstg]

Marked this issue as blocked, because we need to settle dart-lang/language#913, and possibly dart-lang/language#914.

[leafpetersen]

I believe that the first two tests, as currently specified, are compile time errors, and the third test, as currently specified, is an implicit downcast and hence a runtime error. I don't see any reason to change any of these, I think the CFE is just missing some errors here.

[eernstg]

Agreed, 'as currently specified', test1 and test2 are errors.

I've added a concrete example in #914, along with some discussion illustrating why it is an anomaly and an accident that this error arises .

[srawlins]

@eernstg the blocking issues you mention are closed. I believe this is unblocked?

[eernstg]

Yes, thanks! dart-lang/language#914 gave rise to changes in the static analysis of returns in async functions. Line 5 and 6 are not errors any more.

A fresh front end, b2e33ee, still does not report any errors for the example here, and a fresh analyzer does not report any errors. So it's working as specified and tools are consistent. Closing.