Skip to content

Kernel inferences incorrect type for await expression. #31541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lrhn opened this issue Dec 5, 2017 · 5 comments
Closed

Kernel inferences incorrect type for await expression. #31541

lrhn opened this issue Dec 5, 2017 · 5 comments
Assignees
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler

Comments

@lrhn
Copy link
Member

lrhn commented Dec 5, 2017

Error message:

tests/language_2/await_test.dart:141:47: Error: The method '<' isn't defined for the class 'dart.async::FutureOr<dart.core::int>'.
Try correcting the name to the name of an existing method, or defining a method named '<'.
    for (var i = await(func(0)); await func(i < 5); await func(i++)) {
                                              ^
tests/language_2/await_test.dart:141:65: Error: The method '+' isn't defined for the class 'dart.async::FutureOr<dart.core::int>'.
Try correcting the name to the name of an existing method, or defining a method named '+'.
    for (var i = await(func(0)); await func(i < 5); await func(i++)) {
                                                                ^

where func has type FutureOr<T> Function<T>(T).

This means that the static type of await(func(0)) use to infer the type of i is FutureOr<int>, not int as it should be.

(See language_2/await_test when it lands).

@lrhn lrhn added area-kernel type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Dec 5, 2017
whesse pushed a commit that referenced this issue Jan 15, 2018
Change Fasta type inference and Kernel type checking to use the new
definition for Future flattening, which is really unwrapping (peeling
off one layer of Future or FutureOr).  Use this for inferring types of
`await` expressions and return types from `async` functions.

Ensure that we are using the same notion of flattening for inference
and checking.  (Maybe it was a red flag that we weren't.)

This fixes await_test so that it produces a runtime error rather than
a compile time error - see #31541.

A similar change will need to be made to the analyzer - see #31887.

Change-Id: I7d936e9788969a48fdc216628eaa793389fb5e30
Reviewed-on: https://dart-review.googlesource.com/34504
Commit-Queue: Kevin Millikin <[email protected]>
Reviewed-by: Kevin Millikin <[email protected]>
@stereotype441
Copy link
Member

45b02f8 changed the compile-time error into a runtime error, and ff008ca fixed the runtime error.

There is still a crash when testing DDC with kernel, however, so I'm just going to go ahead and reclassify this as a DDC bug. To repro, remove the line await_test: Crash # Issue 31541 from tests/language_2/language_2_dartdevc.status and run:

tools/test.py --compiler=dartdevk --runtime=chrome --checked --use-sdk --strong --mode=release language_2/await_test

Backtrace looks like this:

NoSuchMethodError: The getter 'typeArguments' was called on null.
Receiver: null
Tried calling: typeArguments
#0      Object.noSuchMethod (dart:core-patch/dart:core/object_patch.dart:46)
#1      ProgramCompiler._getExpectedReturnType (package:dev_compiler/src/kernel/compiler.dart:2787)
#2      ProgramCompiler._emitGeneratorFunction (package:dev_compiler/src/kernel/compiler.dart:2775)
#3      ProgramCompiler._emitFunction (package:dev_compiler/src/kernel/compiler.dart:2634)
#4      ProgramCompiler._emitLibraryFunction (package:dev_compiler/src/kernel/compiler.dart:2370)
#5      MappedIterator.moveNext (dart:_internal/iterable.dart:391)
#6      new List.from (dart:core-patch/dart:core/array_patch.dart:48)
#7      Iterable.toList (dart:core/iterable.dart:358)
#8      ProgramCompiler._emitLibraryProcedures (package:dev_compiler/src/kernel/compiler.dart:2348)
#9      ProgramCompiler._emitLibrary (package:dev_compiler/src/kernel/compiler.dart:411)
#10     Iterable.forEach (dart:core/iterable.dart:237)
#11     ProgramCompiler.emitProgram (package:dev_compiler/src/kernel/compiler.dart:289)
#12     compileToJSModule (package:dev_compiler/src/kernel/command.dart:221)
#13     _compile (package:dev_compiler/src/kernel/command.dart:199)
<asynchronous suspension>
#14     compile (package:dev_compiler/src/kernel/command.dart:33)
<asynchronous suspension>
#15     runBatch (file:///usr/local/google/home/paulberry/dart1/sdk/pkg/dev_compiler/bin/dartdevk.dart:46)
<asynchronous suspension>
#16     main (file:///usr/local/google/home/paulberry/dart1/sdk/pkg/dev_compiler/bin/dartdevk.dart:19)

@jmesserly
Copy link

jmesserly commented Jan 16, 2018

I can fix the crash. Question though for @lrhn and @leafpetersen, given this program:

import 'dart:async';
FutureOr<T> future<T>(T value) async => value;
main() {
  var f = future<int>(0);
  print(f.runtimeType); // _Future
  f as Future<int>; // fails casting _Future<dynamic> to Future<int>
}

Is that behavior intended? That is what DDC (w/ Analyzer strong mode) is currently doing.

When determining the type T of the Future<T> to return from the async body, we look at the declared return type, and see if we can match Future<T>. If we can't, we default to T := dynamic. However the code sample above uses FutureOr<T>, which fails to match.

Seems like a soundness problem, because the value in f at runtime is not a subtype of its static type (FutureOr<int>). I believe we should change _getExpectedReturnType for DDC/DDK to match against FutureOr<T>. And probably add some testing for the reified type of async (and sync*, async*) function bodies.

Aside @stereotype441 -- it would be kinda nice if front_end/kernel made T accessible somehow; presumably the expected return type was already computed for inference purposes. For DDC we just had a copy of the Analyzer code, which wasn't ideal. Reason I mention this is dart2js/VM might have a similar problem as DDC for reified type of async function results. (edit: essentially we're redoing some inference in the back end. Fairly simple inference, but still.)

@stereotype441
Copy link
Member

@jmesserly As of 45b02f8 you can get access to the T by passing the closure's return type to TypeEnvironment.unfutureType. That should work regardless of whether the closure's return type is FutureOr<T> or Future<T>. Is that be sufficient for you or were you hoping that the type T would be stored directly in the kernel representation of the closure?

@lrhn
Copy link
Member Author

lrhn commented Jan 17, 2018

tl;dr: Use Future<flatten(declaredReturnType)> as the type of object actually returned.

For the above program, the returned value should be a Future<int> in Dart 2.

I don't know if strong mode specified this explicitly, but as I see it, If the declared return type of an async function designates a particular type of future, then that is the type of future to return. If not, then just use Future<Object> (or Future<dynamic>, but as runtime-types those are equivalent in everything but toString).

So, the declared return type -> returned future type mapping is:

  • void -> Future<Object>
  • Object -> Future<Object>
  • dynamic -> Future<Object>
  • FutureOr<T> -> Future<T>
  • Future<T> -> Future<T>

No other types are valid return types for async functions (the declared return type must be something that some Future<T> is a subtype of, and the actual return value type is the most general Future type satisfying that).

The first three cases have no hint to the actual type of the future, so it just picks a type that works for all values.

Which actually means that we can just use Future<flatten(declaredReturnType)> for all valid async function return types, since it doesn't matter if we use Future<Object> or Future<void> as the returned future type. So, let's just say that we do that.

@jmesserly
Copy link

thanks @lrhn, that sounds good to me! I will use that in DDC/K.

@whesse whesse closed this as completed in 8d48151 Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler
Projects
None yet
Development

No branches or pull requests

3 participants