Skip to content
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

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

Closed
rrousselGit opened this issue Dec 11, 2023 · 22 comments
Closed

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

rrousselGit opened this issue Dec 11, 2023 · 22 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@rrousselGit
Copy link

rrousselGit commented Dec 11, 2023

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
Copy link
Member

eernstg commented Dec 12, 2023

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 eernstg added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Dec 12, 2023
@eernstg
Copy link
Member

eernstg commented Dec 12, 2023

@chloestefantsova, @johnniwinther, WDYT?

@busslina
Copy link

busslina commented Dec 12, 2023

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
Copy link
Member

eernstg commented Dec 12, 2023

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
Copy link
Member

lrhn commented Dec 12, 2023

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.

@eernstg eernstg added area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Dec 12, 2023
@johnniwinther
Copy link
Member

The CFE generates the right "runtimeCheckType":

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

@eernstg
Copy link
Member

eernstg commented Dec 12, 2023

@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
Copy link
Member

lrhn commented Dec 13, 2023

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

  • The value type of the returned future, the function returns a Future<V>.
  • The context type of return statement expressions inside the body, which have context type FutureOr<V>.

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
Copy link
Member

eernstg commented Dec 13, 2023

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
Copy link
Member

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
Copy link
Member

lrhn commented Dec 13, 2023

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
Copy link
Member

@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
Copy link
Member

lrhn commented Dec 13, 2023

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:

  • dynamic. Dynamic invocations and implicit downcast only apply to the type dynamic, not to fx FutureOr<dynamic>, which normalizes to dynamic. (But I'm pretty sure we eagerly normalize dynamic? to dynamic and allow dynamic behavior for that, which is probably the exact right thing to do. So we're inconsistent, in the most user friendly way. I don't know if that's a happy accident of representation, or if we did it deliberately.)
  • void. The restriction against accessing members on an expression typed as void does not apply to FutureOr<void>, which normalizes to void. (And again we treat void? differently, and that's a good thing.) And the rule against returning a value from a void-return typed function also only works for the precise void type.

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
Copy link
Contributor

codesculpture commented Dec 17, 2023

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
Copy link
Member

lrhn commented Dec 17, 2023

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 dart-lang/language#870, which we hope to get done eventually.

@codesculpture
Copy link
Contributor

Well, this issue may solved by this dart-lang/language#870 ?

copybara-service bot pushed a commit that referenced this issue Dec 19, 2023
…c change

Calculation of element type for the value returned from
async/async*/sync* functions was recently adjusted
in the spec to address soundness issue
(dart-lang/language#3218).

This change adjusts Dart VM to conform to the new spec.

TEST=language/async/regression_54311_test
TEST=co19/Language/Functions/element_type_*

Fixes #54316
Issue #54311
Issue #54159

Change-Id: I4c51e7cba704d034350519375210bdb2086c5432
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/342082
Reviewed-by: Alexander Aprelev <aam@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>
@codesculpture
Copy link
Contributor

I think this is fixed here

6b12473

So can be closed?

@eernstg
Copy link
Member

eernstg commented Jan 11, 2024

Well, this issue may solved by this dart-lang/language#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
Copy link
Member

eernstg commented Jan 11, 2024

So can be closed?

#54318 is still marked as open, that is, DDC still needs to be updated.

@codesculpture
Copy link
Contributor

codesculpture commented Jan 11, 2024

Well, this issue may solved by this dart-lang/language#870 ?

You are right @eernstg
At the time of writing this comment, i thought dart-lang/language#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 6b12473
This solves the soundness issue, for this issue

@eernstg
Copy link
Member

eernstg commented Jan 11, 2024

Right, 6b12473 is a VM update that targets this spec change.

@sigmundch
Copy link
Member

(closing now that the DDC issue is also closed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

8 participants