dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.59k forks source link

`await futureOr` may return a Future when the generic is `Object` #54311

Closed rrousselGit closed 6 months ago

rrousselGit commented 11 months ago

Consider the following code:

import 'dart:async';

void main() async {
  final value = await fn();
  print(value);
}

FutureOr<Object> fn() async {
  return Future<Object>.value(42);
}

This should print 42 but actually prints Instance of '_Future<dynamic>'

Returning anything other than FutureOr<Object> in fn fixes the issue. This includes changing Object to int.

eernstg commented 11 months ago

This is a somewhat twisted corner of the language, and it does indeed show an issue with a type function called futureValueType.

The async function fn has return type FutureOr<Object> which means that it has a so-called future value type of Object.

The intuition is that the future value type is the "real" returned value for an async function, that is, the kind of object that you will receive by awaiting the Future that the async function returns if it is completed by a value (and not by an error). An async function always returns a Future, also in the case where you declare the return type to be, say, FutureOr<...> or void.

In other words, if the async function returns Future<S> for some S then the future value type is S, and you'll get an S by awaiting the return value from an invocation of that function (or it throws). The declared return type could be any of several different types: Future<S>, Future<S>?, FutureOr<S>, void (in which case S is also void), and more, but it would have to be a supertype of Future<S>, and the declared type is used to compute S in the first place.

So the invocation of fn should return a Future<Object>. In DartPad it actually returns a _Future<dynamic> (where _Future is a class that implements Future). So that's a bug.

[Edit: I tried out the program on the command line, and dart, dart compile js, and dart compile exe all have this behavior.]

Just to finish the story about what should have happened, assume that it returns a Future<Object> (the following works similarly with the Future<dynamic> that we actually get):

The return statement in fn yields an object whose run-time type is Future<Object>. So the return statement must await this future, obtain 42, and use 42 to complete the returned future. So print(value) in main should indeed print '42'.

(We do get that behavior if the return type of fn is changed to Future<Object>.)

So the conclusion is that fn returns a future whose run-time type implements Future<dynamic>; this seems to imply that the future value type is considered to be dynamic (it should have been Object). On the other hand, the return statement in fn does not await the given future, even though it is a Future<Object> which is a subtype of Future<flatten(FutureOr<Object>)> == Future<Object>; this seems to imply that flatten(FutureOr<Object>) is considered to be some type T which is not a supertype of Object, which means that it can't be dynamic after all.

eernstg commented 11 months ago

@chloestefantsova, @johnniwinther, WDYT?

busslina commented 11 months ago

It fails too with this simpler code:

import 'dart:async';

void main() async {
  final value = await fn();
  print(value);
}

FutureOr<Object> fn() async => 42;

But not with:

import 'dart:async';

void main() async {
  final value = await fn();
  print(value);
}

Future<Object> fn() async => 42;

I can suppose that Dart coders would expect same behaviour on both cases.

(This is in line with what @eernstg said)

eernstg commented 11 months ago

Interesting, @busslina! Clearly the return type FutureOr<Object> creates some confusion during compilation. When two async functions have return types Future<Object> and FutureOr<Object>, both of them should have future value type Object, and the future value type should govern the type of the returned future (when we know the future value type we don't have to know anything else about the declared return type), and that should in turn govern the semantics of return statements. I can't think of any way those two functions could behave differently (apart from the static type of invocations, which would be taken directly from the declared return type).

lrhn commented 11 months ago

The most likely reason is that the "future value type" uses the old unsafe approach, and becomes dynamic for FutureOr<Object>. Then the returned future is a Future<Object?>, and the is Future<Object> test in the await rejects it and passes the future through as an Object.

johnniwinther commented 11 months ago

The CFE generates the right "runtimeCheckType":

  final core::Object value = await self::fn() /* runtimeCheckType= asy::Future<core::Object> */ ;
  core::print(value);
eernstg commented 11 months ago

@johnniwinther just checked the generated code, and it does contain the expected future value type. It looks like there's a need to double check what the backends are doing. I'm creating issues for that.

Subtasks:

lrhn commented 11 months ago

The runtimeCheckType above is not the relevant type. The relevant type is the "future value type" of the function declaration, which is used as:

A quick check shows that the context type of the return type of return expressions is correct:

import "dart:async";
void main() {
  printType(foo(1));
  printType(o(1));
}

String lastName = "";
Object o(Object? value) async { 
  lastName = "Object";
  return captureContext(value);
}
FutureOr<Object> foo(Object? value) async { 
  lastName = "FutureOr<Object>";
  return captureContext(value);
}

Type lastContext = Null;
T captureContext<T>(Object? o) {
  lastContext = T;
  // print("Context type: $T");
  return o as T;
}

void printType<T>(T value) {
  // Prints value, runtime type and static type captured in type variable.
  print("$lastName: $value");
  print(" RuntimeType: ${value.runtimeType}");
  print(" Static type: $T");
  print(" Async context type: $lastContext");
}

(Run with dart2js in DartPad, any branch.)

The FutureOr<Object> case is wrong. (I tested other types too, it's the only one which is wrong). The printed context type of Object is correct, because it's Norm(FutureOr<Object>). The important part is that the context type is not dynamic or Object?, so the implementation knows the correct future-value-type, and knows it's not a top type.

However, the returned future is a _Future<dynamic>, the same as if the return type had been FutureOr<void> or FutureOr<Object?>, or just Object, which is incorrect for FutureOr<Object>, and causes a later await at static type FutureOr<Object> to not await the future.

My hunch is that the code re-computes the future-value-type, instead of using the type provided by the CFE and used for the return statements, but does so on a normalized (by Norm) return type. The normalization of FutureOr<Object> is Object. That approach is incorrect, because the future-value-type should be used on un-normalized types, and it's one function which does not give the same result for all types that are Norm-equivalent. (Most of the language's functions on types are compatible with normalization, so that Norm(f(T)) == Norm(f(Norm(T))), often without needing the last Norm on the right side. Future-value-type is not.)

But it's suspicious that it's consistent across implementations. (@johnniwinther Are we sure the type of future created is not provided by the CFE?).

eernstg commented 11 months ago

The runtimeCheckType above is not the relevant type.

True, it's the futureValueType which is relevant here. We can see it in the Kernel which is produced from the original example, which shows that fn has a future value type of Object:

library;
import self as self;
import "dart:core" as core;
import "dart:async" as asy;

import "dart:async";

static method main() → void async /* futureValueType= void */ {
  final core::Object value = await self::fn() /* runtimeCheckType= asy::Future<core::Object> */ ;
  core::print(value);
}
static method fn() → FutureOr<core::Object> async /* futureValueType= core::Object */ {
  return asy::Future::value<core::Object>(42);
}

@johnniwinther and I discussed this IRL, and I didn't notice that the Kernel code shown so far only contained the runtimeCheckType.

One possible explanation would be that the magic comment /* futureValueType= core::Object */ is somehow misinterpreted by the backends, and I agree that it could involve normalization. In any case, we have a clearly wrong behavior and a very small example producing it, so it should be well on track.

fishythefish commented 11 months ago

FWIW, the fix to dart2js is landing here. Outside of an experimental/disabled kernel transformation, dart2js simply never used the futureValueType anywhere prior to this change. Instead, we were pattern matching on the return type (modulo nullability) - we were extracting T from FutureOr<T> and Future<T>, and using dynamic otherwise. The type representation we use internally normalizes at construction, which leads to the issue @lrhn described.

lrhn commented 11 months ago

If we had normalized FutureOr<Future<Object>> to Future<Object> (which is a mutual subtype), then we would likely have had a soundness issue with FutureOr<Future<Object>> foo() async { return Object(); } too. But we don't. Phew. (We don't generally normalize A | B to one of the types when it's a supertype of the other, we only special when one of them is a top type, Object, Null or Never.)

fishythefish commented 11 months ago

@lrhn: You mentioned that there are other functions which must operate on non-normalized types. Is this list documented anywhere? It's been convenient for us to eagerly normalize types at construction in our internal representations, but we may have to rethink this design.

lrhn commented 11 months ago

There is no list.

I don't know where it matters, but generally anything in the language semantics is defined in terms of un-normalized types unless it explicitly says otherwise. All the type functions, Up, flatten, the subtype relation itself, etc., assumes that the input can be non-normalized. And some uses of them require it. Or at least this one.

The place I can remember that normalization is mentioned is Type.operator==, where two type objects are equal if their corresponding types normalize to the same type.

Since any type is a mutual subtype with its normalization, any rules based entirely on subtyping shouldn't be affected by normalization.

On the other hand, any place where a type is special-cased independently of subtyping, it's important to not normalize. That includes:

These are the two most special-cased types, and their interaction with union types is where we can most see the effect.

Generally we seem to eagerly remove ? when the underlying type is already nullable, and not eagerly remove FutureOr when the value type is a supertype of Object. That matches the users' mental model, where nullability is idempotent, dynamic? is just dynamic again, and that's why it's ok that our model can't even represent int??. On the other hand, adding FutureOr adds intent and information, even if there is no difference in subtyping. Sure, Object can be a Future, but we know it isn't, because then it would have said so!

That's why this problem comes from FutureOr and future-value-type, a place where we work on un-normalized types. It matters whether the return type is declared as Object or FutureOr<Object>, because only one of them tells us which kind of future is intended to be returned, even if exactly the same values can be returned.

I don't think flatten has the same issue, because we've designed it to give the same result before and after normalization. (If we hadn't, and had used the same principle as for future-value-type, flatten(Object) should have been Object?.)

So if there are other cases, then they're likely around FutureOr and some use-case where it matters if the FutureOr is removed or not.

(All of this from memory, on my phone. I can take a closer look tomorrow.)

codesculpture commented 11 months ago

Hi, i have a small question.


Future<int> fn() async() {
         return Future<int>.value(42);
}

I felt something wrong here, since the actual return type of fn() would (needs to) be Future<Future<int>>

but its being resolved to Future. Is that expected?

lrhn commented 11 months ago

Yes, that is currently specified behavior. The return statements of async functions have an implicit await built in.

Making that no longer be the case is https://github.com/dart-lang/language/issues/870, which we hope to get done eventually.

codesculpture commented 11 months ago

Well, this issue may solved by this https://github.com/dart-lang/language/issues/870 ?

codesculpture commented 10 months ago

I think this is fixed here

https://github.com/dart-lang/sdk/commit/6b12473f75788fbf7cdca52c68b7bd99609ba221

So can be closed?

eernstg commented 10 months ago

Well, this issue may solved by this https://github.com/dart-lang/language/issues/870 ?

No, dart-lang/language#870 makes it an error to have return e; when the static type of e isn't assignable to the future value type T of the enclosing async function (today, that static type must be assignable to T or a subtype of Future<T>).

This issue is about the actual type of future which is being returned when an async function has a non-standard return type (like FutureOr<Object>), that is, it is about the dynamic semantics of such functions, and 870 does not propose any changes to the dynamic semantics.

eernstg commented 10 months ago

So can be closed?

https://github.com/dart-lang/sdk/issues/54318 is still marked as open, that is, DDC still needs to be updated.

codesculpture commented 10 months ago

Well, this issue may solved by this https://github.com/dart-lang/language/issues/870 ?

You are right @eernstg At the time of writing this comment, i thought https://github.com/dart-lang/language/issues/870 would solve this because that returning Future<T> for any async function which has return type of Future<T> could make this error while doing static analysing but soon i realise this is different from what i thought. Sorry for that. Also "now" i belive this is fixed by https://github.com/dart-lang/sdk/commit/6b12473f75788fbf7cdca52c68b7bd99609ba221 This solves the soundness issue, for this issue

eernstg commented 10 months ago

Right, https://github.com/dart-lang/sdk/commit/6b12473f75788fbf7cdca52c68b7bd99609ba221 is a VM update that targets this spec change.

sigmundch commented 6 months ago

(closing now that the DDC issue is also closed)