Skip to content

Soundness issue with await #49396

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
eernstg opened this issue Jul 5, 2022 · 40 comments
Closed

Soundness issue with await #49396

eernstg opened this issue Jul 5, 2022 · 40 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) soundness

Comments

@eernstg
Copy link
Member

eernstg commented Jul 5, 2022

[Edit: This issue has evolved into an implementation issue, hence the 'area-meta' and 'implementation' labels.]

Subissues:


Consider the following program:

import 'dart:async';

void main() async {
  final FutureOr<Object> fut = Future<Object?>.value();
  var x = await fut;
  // int i = x; shows that `x` has type `Object`.
  print(x); // 'null'.

  final Object o = Future<Object?>.value();
  var y = await o;
  // int i = y; shows that `y` has type `Object`.
  print(y); // 'null'.
}

This program is accepted by the dart analyze, dart, and dart compile js without any diagnostic messages, and it prints null twice with dart and dart compile js. When the commented-out declarations are commented in, all tools report a compile-time error, confirming that the inferred type is Object for both x and y.

This means that the state of x and y is a soundness violation, because the type is Object and the value is null.

So we need to take into account that when the static type of e is Object or FutureOr<Object>, the expression await e can evaluate to any object including null.

[Edit, based on further discussion in dart-lang/language#2326]:

The soundness issue comes up with several other types than Object and FutureOr<Object>, cf. dart-lang/language#2310 as well.

Consider await e, where e has static type S. In this situation, the evaluation of await e has the following steps: Evaluate e to an object o. Check whether the dynamic type of o is a subtype of Future<flatten(S)>. If yes, then await o; otherwise await Future<S>.value(o). The soundness issue arises because the check on the dynamic type of o does not occur (at least not always).

In the example above, S is FutureOr<Object> respectively Object, flatten(S) is Object, and the dynamic type of the value of e is (a type that implements) Future<Object?>. So we should await Future<Object>.value(o), but the actual behavior is to await o itself.

This issue is still blocked on discussions in dart-lang/language#2326.

@eernstg eernstg added legacy-area-analyzer Use area-devexp instead. status-blocked Blocked from making progress by another (referenced) issue soundness legacy-area-front-end Legacy: Use area-dart-model instead. labels Jul 5, 2022
@eernstg eernstg changed the title Soundness issue with await and Object Soundness issue with await Jul 5, 2022
@johnniwinther
Copy link
Member

cc @chloestefantsova

@eernstg
Copy link
Member Author

eernstg commented Jul 6, 2022

This issue is blocked because discussions about the intended semantics are ongoing (dart-lang/language#2326, dart-lang/language#2310). I've created this issue because those discussions revealed that the current implementations of dart and dart compile js do have a soundness issue, and we will definitely need to address that.

But we can't do that yet: The soundness issue arises because a certain type check isn't performed during evaluation of an <awaitExpression> (or perhaps the type check is being performed, but it isn't used to determine what happens next). The fix could be to make sure that the type check is performed, and that it does determine subsequent behavior as specified, but another possible fix could be to change the static type of certain await expressions.

One possible explanation for the currently implemented behavior with await e is that there is a dynamic type check, determining whether the value of e is a future or not (that is, whether it is Future<Object?>); but the check should have been whether said value is a Future<flatten(staticTypeOf(e))>.

eernstg pushed a commit to dart-lang/language that referenced this issue Jul 7, 2022
This PR specifies a breaking change to await, and an update to flatten which includes null safety. In particular, **flatten** is changed such that it is defined for type variables and intersection types. It changes `await` to throw when provided a `Future` value of a type that could provide an unsound value if awaited. This differs from the current specification, which instead treats the future as a value, so `await someFuture` can evaluate to `someFuture`. It also differs from the current implementation, which awaits `someFuture` to its value, whether the result is sound or not.

See also dart-lang/sdk#49396, #2326.
@eernstg
Copy link
Member Author

eernstg commented Jul 7, 2022

The language team favored a third option (not mentioned above), as specified in dart-lang/language#2333.

The main point is that there is a dynamic error during evaluation of await e, where e has static type S and T is flatten(S), if e evaluates to an instance whose dynamic type implements Future<R> for some R which is not a subtype of T.

For example:

void f() async {
  FutureOr<Object> fut = Future<Object?>.value();
  var x = await fut; // Throws.
}

In this example, the value of fut is a future of Object? (which is an Object, so there's no soundness issue with the initialization of fut). It is evaluated and it is detected that it is not an instance with type Future<Object>, but it is a Future<Object?>, and then it throws.

The point is, intuitively, that flatten(FutureOr<Object>) is Object, so we expect an Object or a Future<Object>. But when we actually receive a Future<Object?> then we can't await it (the result is not guaranteed to have type Object), and we shouldn't treat it just like "an Object" (because it is a future), so we throw.

NB: The issue is still blocked, discussions haven't been settled entirely!

@pq pq added the P3 A lower priority bug or feature request label Jul 25, 2022
@eernstg
Copy link
Member Author

eernstg commented Nov 2, 2022

@sigmundch, @rakudrama, @nshahan, @mkustermann, @johnniwinther, the language team discussed this issue today, (Nov 2, 2022). One reasonable way ahead, in many ways, would be to implement the specified behavior:

Evaluation of an await expression $a$ of the form \code{\AWAIT{} $e$} where $e$ has
static type $S$ proceeds as follows:
First, the expression $e$ is evaluated to an object $o$. Let $T$ be \flatten{S}.
It is a dynamic type error if the run-time type of $o$ is not a subtype of \code{FutureOr<$T$>}.

If the run-time type of $o$ is a subtype of \code{Future<$T$>}, then let $f$ be $o$;
otherwise let $f$ be the result of creating a new object using the constructor
\code{Future<$T$>.value()} with $o$ as its argument.

The language team wants to clarify the situation, and proceed to do this unless there are concerns that we have overlooked.

Based on the current behavior, and in particular the example here which illustrates the soundness violation, the situation appears to be the following:

At the point where we should test If the run-time type of $o$ is a subtype of \code{Future<$T$>}, the implementations seem to test whether the run-time type of $o$ is a subtype of Future<dynamic>, and the unsoundness follows from getting true from that test in cases where we should have had false.

At the language team meeting, concerns were raised about the implications of implementing this change, in particular with respect to the code size, for web configurations.

It seems that a test for Future<T> may be more expensive than a test for Future<dynamic>, and it might cause type arguments to be preserved at run time in situations where they might otherwise be eliminated.

On the other hand, it should be noted that there is never a need to perform this dynamic check when the static type of $e$ is a subtype of Future<T> for any T, which would be a very common case with await expressions. Similarly, await null does not need any dynamic checks. The remaining cases are in particular when the static type of $e$ is FutureOr<T> for some T, or it is a non-future type (say, MyClass, where we cannot completely rule out the possibility that the given object has a run-time type which is also a subtype of Future<Object?>).

What is your impression of the situation? Is there a danger that this change to the implementation could cause programs to get larger or run more slowly, or that it would cause a considerable effort to implement it?

@eernstg
Copy link
Member Author

eernstg commented Nov 2, 2022

A language test available in https://dart-review.git.corp.google.com/c/sdk/+/267422.

@johnniwinther
Copy link
Member

From the CFE perspective, we probably want to add properties to the AwaitExpression node, such that determining whether or not a runtime check needs to be performed, and if so, what type it should against, is straight forward for the backends.

@eernstg
Copy link
Member Author

eernstg commented Nov 8, 2022

CFE ... add properties ... straight forward for the backends

Sounds good! Here's a summary of the situation:

  • The dynamic type test is never needed when the static type of e in await e is of the form Future<T>. By soundness, it will evaluate to an object whose run-time type implements Future<S> for some S <: T, and then the completion of that future must be awaited.
  • In the case where the expression has an exact type which does not implement Future (say, null or some other literal, or a constructor invocation), if the concept of an exact type is available anywhere in the CFE, it is guaranteed statically that the value of e, let's call it o, should not be awaited (which could mean that Future.value(o) must be awaited, depending on how it is implemented).
  • In the case where the static type of e is FutureOr<T> for some T, the dynamic test is probably always required.
  • In the case where the static type of e is some type T which is not a supertype nor a subtype of Future<U> for any U, the dynamic test is probably always required (because the given object could implement Future).
  • In the case where the static type of e is a top type, a simpler dynamic type test can be used: If e evaluates to a future (no need to check the type argument) then it must be awaited, otherwise Future.value(o) must be awaited.

@alexmarkov
Copy link
Contributor

Did we ever consider to disallow await e if e has a static type which is not a subtype of Future? That would simplify things a lot and avoid unnecessary runtime checks. My understanding is that await is only really useful for futures, so awaiting anything else is likely either a no-op or a hack to yield to micro-tasks. Disallowing await for non-futures can be done step-by-step, using a lint to gradually enforce this and maybe handle this in dart fix to migrate such code automatically. After lint is enforced everywhere and turned on by default, we can disallow this in the language. WDYT?

@eernstg
Copy link
Member Author

eernstg commented Nov 8, 2022

disallow await e if e has a static type which is not a subtype of Future?

Yes, that's very tempting, but we never got around to a decision to actually do that.

We should probably allow await e when the static type of e is a type that implements Future or the type is dynamic, and when e is null (to allow developers to say "wait a bit", without doing anything extra like creating an object). The type FutureOr<T> is on the fence: Should we require that developers discriminate this type to Future<T> and T manually, or is await e OK, and the await will do the test? The rest can be an error: It's really not very useful to have await 14, await myDateTime, and such.

But we should know more about how breaking this would be.

@alexmarkov
Copy link
Contributor

If we decide to go this route, then it would be nice to have static type requirements for the argument of await to be Future similarly to how we require logical expressions to be bool. If static type of the await argument is dynamic, then it is implicitly casted to Future<dynamic> and type error is thrown if it isn't future at runtime.

As for await null case, I think it is not guaranteed to yield to other microtasks according to the spec, so it is basically a hack to use it in such a way. Instead, it would be cleaner to introduce a proper API in the core library for that purpose, such as Future<void> completePendingMicrotasks() in dart:async - somewhere near existing scheduleMicrotask API. We can use automated tooling to migrate all occurrences of await null to the new API.

@eernstg
Copy link
Member Author

eernstg commented Nov 8, 2022

As for await null case, ...

It is specified such that await null is executed by creating a Future<Null>.value(null) and suspending the execution awaiting the completion of that future.

The specification never says 'microtask', but I can't see how this can be implemented in a way that doesn't suspend execution at await null until it is restarted by the scheduler. So it should be OK to use await null in order to obtain a suspension. This also matches an old phrase which was supposed to be true: "await will await".

On the other hand, we could of course make await null (and similar expressions) an error, and then support the suspension in some other way. Anyone who doesn't know what await null is doing would probably have a hard time discovering the semantics and purpose of that construct, but completePendingMicrotasks() is more self-explanatory, and may be linked to a DartDoc paragraph.

In any case, we should fix the soundness issue now; the syntax used to get a plain suspension may then be dealt with as a separate effort.

@nshahan
Copy link
Contributor

nshahan commented Nov 8, 2022

In the DDC implementation we don't have any special casts or type tests of the expression in the await at runtime. We simply lower to a JavaScript yield expression and let the native implementation handle the value. That means anything we add will be more work than what is performed currently. It should be fairly easy to insert a cast, but the best case scenario would be to have the CFE identify only the cases where a runtime check is needed to preserve soundness.

@leafpetersen
Copy link
Member

In discussion with @nshahan , we noted that it's a bit odd that in the second example:

final Object o = Future<Object?>.value();
  var y = await o;
  // int i = y; shows that `y` has type `Object`.
  print(y); // 'null'.

we are proposing to turn this into a runtime error (a cast to Object will be inserted which will fail). It's a bit odd to infer Object here when we could say Object?.

I wonder if we should make flatten(Object) be Object?? Or if this is not the right thing to do for all uses of flatten, we could either use futureValueType for await instead of flatten, or define a new operator?

@lrhn
Copy link
Member

lrhn commented Nov 8, 2022

We are not proposing to insert a cast to Object.
Rather, we are requiring a test of is Future<Object> to guard the await, so a Future<Object?> will not be awaited.
The result will be that y has type Object and contains the Future<Object?> of o, which has not been awaited.

The flatten operator is what we use for await, it's what it was designed for. We introduced the "future-value-type" because flatten was not correct for figuring out what to return from an async function, because it was designed for await.
If flatten is not doing what we want in await, we should change it.

Changing flatten to return Object? on Object (or on any non-Future type, really, because a List<int> can implement Future<Object?> too) is possible, but it still won't help us with FutureOr<Object>. It'll be too disruptive to make flatten of that be Object?. If we do that, we might as well disallow awaiting anything but Future.
(We considered those alternatives back when we changed the specification to the current phrasing.)

Basically:

  • if you await something which is a Future<T>, then we know that flatten is T, and that's safe.

  • if you await a FutureOr<T>, then we can have an issue if T can itself be a Future of a type other than T, like T being Object. That's where we need to do an is Future<T> check first, to destructure the union type, before awaiting only the Future part of it.

  • if you await something which is neither, we cannot really know if the object ends up implementing Future anyway.
    What we (are specified to) do then is to treat the non-Future/FutureOr static type T as if it was FutureOr<T>, check if the value is a Future<T>, and and await it if so.

(I'd be fine with simply not awaiting anything but the static types Future<T>, FutureOr<T> and dynamic, but that's not what we do or specify today. With await, I mean wait for the future to complete. Even without that, we still introduce an async delay before evaluating to the Future).

Now, if we changed the spec to insert an as flatten(T) after the await (optimizable away when not needed), instead of an is Future<flatten(T)> check before it, then

  • I'd assume dart2js would just omit the check. Just another unsound optimizations.
  • We'd get some new runtime errors that you can't really statically check for. Those are probably good errors, in that it was unlikely that the author intended to have a Future<Object?> in a place that has type FutureOr<Object>, or even just Object (use FutureOr if you think it might be a future, and you probably thought that since you used await). And you probably didn't expect to do an await and end up with a Future value anyway. The await_only_futures lint should help too.
  • But it is runtime errors, rather than static errors. We like static errors.
  • But it's probably faster. We like fast too.

If we are going to make changes, I'd still prefer to just not await anything if the static type is not Future, FutureOr or dynamic. Only the FutureOr needs any check, whether before or after.

@leafpetersen
Copy link
Member

We are not proposing to insert a cast to Object.
Rather, we are requiring a test of is Future<Object> to guard the await, so a Future<Object?> will not be awaited.
The result will be that y has type Object and contains the Future<Object?> of o, which has not been awaited.

Sorry, yes I'm not sure how I got confused on this (maybe the cast was the change that got reverted and I read an old version of the spec? Or maybe I just misread it?).

The behavior then is perhaps less disruptive, but nonetheless pretty surprising. It feels like a foot gun that:

final Object o = Future<Object?>.value(3);
var y = await o;
print(y); 

will print "Instance of 'Future<Object?>'"

but

final Object? o = Future<Object?>.value(3);
var y = await o;
print(y); 

will print "3".

@eernstg
Copy link
Member Author

eernstg commented Nov 9, 2022

@leafpetersen wrote:

I wonder if we should make flatten(Object) be Object??

Sounds like this is a proposal to avoid the run-time type test for Future<flatten(StaticTypeOfAwaitedExpression)>?

I'm afraid we would then need to make flatten(T) be Object? for almost every T (the exception being the case where T is a subtype of Future<S> for some S):

import 'dart:async';

class A {}

class B extends A implements Future<Object?> {
  Future<Object?> fut = Future.value(CompletelyUnrelated());
  asStream() => fut.asStream();
  catchError(error, {test}) => fut.catchError(error, test: test);
  then<R>(onValue, {onError}) => fut.then(onValue, onError: onError);
  timeout(timeLimit, {onTimeout}) => fut.timeout(timeLimit, onTimeout: onTimeout);
  whenComplete(action) => fut.whenComplete(action);
}

class CompletelyUnrelated {}

A aFunc() => B();

void main() async {
  A a = aFunc();
  var x = await a;
  print(x.runtimeType); // 'CompletelyUnrelated'.
}

@leafpetersen
Copy link
Member

Sounds like this is a proposal to avoid the run-time type test for Future<flatten(StaticTypeOfAwaitedExpression)>?

No, not really. It's to avoid the odd behavior that I describe above where we have something which has a type which is known to be a super type of any possible Future, and yet we choose to make await skip awaiting on half of them (the ones with nullable arguments).

So I think I would keep the runtime behavior as specified, but just change what we infer for var x = await e when e has type Object.

I'm afraid we would then need to make flatten(T) be Object? for almost every T (the exception being the case where T is a subtype of Future<S> for some S):

I definitely wouldn't want that. I think we can say that types come in three categories:

  • Known to be a supertype of Future, but not known to implement Future (Object, Object?)
  • Known to implement Future (Future, or B from your example above)
  • Known not to implement Future (e.g. int)
  • Might implement Future, might not. (e.g. A from your example above)

It's true that your proposal also means that we will skip awaiting an A, but that's pretty esoteric, and doesn't surprise me that much. It does surprise me that changing the type from Object to Object? in my example changes the behavior.

I'm not dead set on this or anything, just want to explore the options here.

@eernstg
Copy link
Member Author

eernstg commented Nov 10, 2022

So I think I would keep the runtime behavior as specified

OK, so we would change the specification of flatten such that flatten(Object) is Object?, but we would leave the specification of the run-time semantics of await unchanged (except for the fact that it uses flatten, which is now different). This should still be sound, because we're awaiting a future in&only-in the case where it must be completed with an object that has the static type of the await expression.

We would presumably change FutureOr<Object> to be Object? as well (this type is also statically known to include every future), as well as FutureOr<FutureOr<Object>>, etc.

There could then be a certain amount of breakage, because some expressions with static type Object would now have static type Object?.

@lrhn
Copy link
Member

lrhn commented Nov 10, 2022

It feels a little special-casey to just do it for Object, but not for FutureOr<Object> (which is a mutual supertype of Object).
We definitely don't want to change the type of FutureOr<Object>, because that's precisely where you want to await and get an Object, so the special-case is necessary.

If we want to change the behavior, then I think we should try to look at it from a use-case perspective, instead of a soundness-perspective.
Currently we are only looking at soundness. The is Future<T> check ensures soundness, but risks ending up with a Future after doing await. That's a very unlikely intended use-case.

If we assume that someone writing await intends to actually either

  • Just delay. await 0 or await null are patterns used to introduce a microtask deleay.
  • Await something which is definitely a future.
  • Or await the future of something which might be a future.

Then we can try to imagine which use case different scenarios fall into.

If you await e and e has type T:

  • If T has a type which implements Future<X>. Definitely await!
  • If T is FutureOr<T>, conditionally await! If the value is not a Future<T>, don't await it, otherwise do.
  • T is not a supertype of Future: Probably just a delay. If the value is an int or null, definitely an await. Maybe not check if it's a future at all.
  • T is dynamic, conditionally await (if it's a future of any kind, await it).
  • T is another supertype of Future. Probably not a delay, there are easier ways to write those. So, likely a conditional await. Not with a known future type. Making it have type Object? isn't unreasonable.

If we assume that you never have a Future<Future<X>>, we can even say that it's an error if a FutureOr<Object> is a Future<Object?>. There is no use-case for that.

All in all, it only differs from the current behavior in non-top types that are supertypes of Future (and not FutureOr), where the type becomes a top type, and in types that are unrelated to Future, where it doesn't await at all.
There are still runtime checks for FutureOr, but that's the only one.

@leafpetersen
Copy link
Member

t feels a little special-casey to just do it for Object, but not for FutureOr<Object> (which is a mutual supertype of Object).

I think this change would definitely apply to FutureOr<Object> as well, if we did it; otherwise all sorts of stuff gets weird. Basically, I would propose to add a clause to flatten singling out Object. The recursive invocation of flatten on FutureOr<T> would then naturally result in flatten(FutureOr<Object>) being Object?

We definitely don't want to change the type of FutureOr<Object>, because that's precisely where you want to await and get an Object, so the special-case is necessary.

Hmm. When do you expect to get a specific Object when awaiting a FutureOr<Object>? I guess the argument is that you expect this to only await on the Future<Object> case. So if this thing turns out to be a Future<int?>, you expect it to fall into the Object` case and not await? I don't know, I don't have good intuition as to what people are doing when they are awaiting something like this.

@lrhn
Copy link
Member

lrhn commented Nov 10, 2022

We don't recursively flatten on FutureOr (or in general). The result of flatten(FutureOr<T>) is just T.

If I have a value with a static type of type FutureOr<Object>, I would expect it to be either a Future<Object> or an Object. I'd not expect the Object to be a Future<Object?>, or Future<int?>. If it is, it's likely a bug.
So, if I await that FutureOr<Object>, I'd expect to get an Object back - by awaiting the Future<Object> and keeping the Object.
If we change the static type of that await to be Object?, I'd be both confused and annoyed. It'd get in the way of the common and correct use.

I'd probably end up writing (await futureOrObject)! because surely the value won't be null. And I'd be right, unless there is a bug.
Or I'd end up doing T value = futureOr is Future<T> ? await futureOr : (futureOr as T);, because that's what I want.
(We allow you to await a FuturOr, with the type we give it today, precisely because we don't want you to have to first figure out which of those two union types it is.)

I don't want to change the typing of await on a FutureOr<X> to be anything but X.
Any other typing would be strictly worse.

For the behavior, it's only really the case where the Object is Future<T> and not a Future<X> where we have wiggle-room.
That means "awaiting such a future and throw if the value is not an X" could also an option, but because nested futures are valid, that would give incorrect behavior for:

FutureOr<Future<int>> ffi = Future<int>.value(1);
var fi = await ffi;

The type of fi will be Future<int>, anything else is inconsistent.
We cannot in good conscience await the Future<int> first, and then afterwards throw because an int is not a Future<int>.

So, the behavior of await e on FutureOr is pretty much a given:

  • If e has static type FutureOr<X>
  • Then flatten(FutureOr<X>) is X.
  • Then await e has static type X
  • At runtime, evaluate e to v
  • If v is a Future<X>, let f be v
  • Otherwise v has type X. Let f be Future<X>.value(v).
  • Then wait for f to complete, and complete with its result.

I can't find any way to reasonably give await of a FutureOr any other consitent type or behavior, without making it so broken that you'd be better of doing the is Future<X> yourself.

The behavior of await on a Future<X> is also given.

That leaves us with the non-Future/non-FutureOr type, which is where flatten currently gives the type back.
I'd be fine with changing that to Object?. If you await a non-future type, then either it's not going to be a future at runtime, and you are just delaying. Then the value most likely doesn't matter.
Or you are expecting that maybe it is a future of some kind, but you don't know which. And neither does the compiler, so expecting an Object? back isn't wrong.

(Except for dynamic, where we should give dynamic again.)

@eernstg
Copy link
Member Author

eernstg commented Nov 11, 2022

So what are the consequences of these proposals?

  1. Change the implemented behavior to match the current specification: No compile-time breakage, and the unsoundness at run time is eliminated. Some examples may be counterintuitive.
  2. Change flatten such that flatten(Object) == Object?, and change the implemented behavior accordingly (the Future<SomeType> check at runtime is still needed for soundness in cases like FutureOr<Future<int>> where class A implements Future<SomeType>). This can introduce some static breakage (some expressions will have static type Object?, and they used to have type Object), but the unsoundness is eliminated. The example from the previous bullet point may now be more intuitive, but since Object == FutureOr<Object> == FutureOr<FutureOr<Object>> == ... it may seem inconsistent that they don't get the same treatment.
  3. Change flatten such that flatten(T) == Object? for every T where T <: Object <: T. This introduces the static breakage like the previous bullet point, and then some, but the unsoundness is again eliminated. This may be counterintuitive in the opposite direction: With await e where e has the static type FutureOr<Object>, it may be expected that a Future<Object> will be awaited, and no other object will be awaited (including Future<Object?>).

Note that the implementations of flatten ignore one more thing: flatten(X) yields X for a type variable X extends B. It should yield flatten(B), as specified, and for soundness. Yielding X can violate soundness, e.g., if X has the value Object and the value of e in await e has run-time type Future<Object?>.

What do we want?

I tend to prefer the currently specified behavior (1).

@mkustermann
Copy link
Member

As @eernstg pointed out, it is (considerably) more expensive to perform a is Future<T> compared to a is Future. It may lead to considerably slower code in some rare cases due to the VM's current implementation of such is checks (see #48235) - which we should arguably optimize.

The fact that we can rely on static types to avoid the dynamic check in many cases (e.g. in await Future<X>) is comforting. The cases remaining seem to be await X (where subtype of X may implement Future) and await FutureOr<X>. For AOT, our global analysis could tell us that there's no subtype of X implementing Future and as such avoid the dynamic check as well (:+1: to adding needs-runtime-check flag to Kernel AST node)

IIRC we do have g3 customer code that uses protobuf-based RPC services where the generated service classes's methods use FutureOr<X> as return type (probably intended as an optimization for cases where the service interface implementation can answer synchronously)

@lrhn
Copy link
Member

lrhn commented Nov 18, 2022

Generally, we need the typed check (an is Future<X> instead of is Future) when awaiting something of static type T if a value of that type can implement Future<X> for some X which is not a subtype of flatten(T).

In more detail:

  • The restriction on only implementing one version of a generic interface, plus covariant subtyping for generics, means we never need a type check at all for a type that itself implements Future<X>, not even is Future.
  • The subtype restriction means we never need a typed check if flatten(T) is a top type.
  • For any other non-Future/non-FutureOr type T = flatten(T). We only need a typed check if T can have a value which implements Future.
    • If T is a sealed type (int, bool, etc.), it can't implement a future.
    • If T is a function type, it can't implement a future.
    • If T is an enum type, and that enum type doesn't implement Future itself, it can't have any value which does.
    • (Ditto for record types when we get them. Generally if no class can subtype both T and Future<X>, it's safe,
      and all types which cannot be implemented and are not supertypes of Future satisfy that. No check is needed at all)
    • If a type does not have any (non-abstract) subtype in the program (as decided by whole-program analysis) which implements Future, it cannot have a value doing so, and no check is needed at all.
    • (Or if there are subtypes, but they all implement Future<X> with X a subtype of T, then it's still safe to only do an is Future).
  • For T = FutureOr<X>, with flatten(T) = X, we only need a typed check if X can be a future with a value type that is not a subtype of X.
    • If X is a top-type, no typed check is need.
    • If X is any of the types above which cannot be futures, no typed check is needed. (E.g. FutureOr<int> is either a Future<int> or an int, and neither can be a FuturewhereXis not a subtype ifint`.)

That does leave Object and FutureOr<Object> as probably common types which need typed checking of the future.

Any type Foo which has a subtype implementing Future<Bar> where Bar is not a subtype of Foo will need a typed check. Such types are unlikely to exist at all, but we do need to check that they don't.

(Maybe the front-end could put a flag on every (non-FutureOr) type which doesn't implement Future, but has a subtype which does. It'll likely be Object and top types only. Maybe Mock too, in tests, if someone mocks a future, which they shouldn't!)

copybara-service bot pushed a commit that referenced this issue Nov 22, 2022
Add AwaitExpression.runtimeTypeCheck to support easy backend
implementation of runtime type check.

In response to #49396

TEST=pkg/front_end/testcases/general/issue49396.dart

Change-Id: I13b9b14566ebc34cdb0811c16a262421417b68e7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/270723
Commit-Queue: Johnni Winther <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
copybara-service bot pushed a commit that referenced this issue Dec 7, 2022
'await e' should check that e is a Future<flatten(S)>, where S is a
static type of e before awaiting e. If e is not a Future<flatten(S)>,
then 'await e' should await Future.value(e) instead of e. So futures
of incompatible type are not awaited and soundness is not violated.

TEST=tests/language/async/await_type_check_test.dart
(Based on https://dart-review.git.corp.google.com/c/sdk/+/267422.)

Fixes #50529
Part of #49396

Change-Id: Ia418db1be6736710abc9be87d95584c50cbc677e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/273002
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
@eernstg
Copy link
Member Author

eernstg commented Dec 8, 2022

The definition of flatten was extended in dart-lang/language@a3b45ab2 to include a case for type variables. That change turned out to give rise to a loss of precision in the cases where a type variable does not have Future<...> or FutureOr<...> as a supertype. This PR changes flatten such that flatten(X) == X when X is a type variable with that property (for example, a type variable with no bound).

@eernstg
Copy link
Member Author

eernstg commented Dec 8, 2022

Said PR, dart-lang/language#2696, has now been landed.

@eernstg
Copy link
Member Author

eernstg commented Dec 9, 2022

https://dart-review.googlesource.com/c/sdk/+/274682 updates the 49396 test to expect the behavior of the updated flatten. Sorry about the inconvenience that may be caused by a change at this time!

@eernstg eernstg added the implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) label Dec 9, 2022
@eernstg
Copy link
Member Author

eernstg commented Dec 9, 2022

Note that https://dart-review.googlesource.com/c/sdk/+/274682 renames the test to await_flatten_test.dart.

@eernstg eernstg added area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). and removed legacy-area-analyzer Use area-devexp instead. legacy-area-front-end Legacy: Use area-dart-model instead. labels Dec 9, 2022
@eernstg
Copy link
Member Author

eernstg commented Dec 9, 2022

@scheglov, is there an 'area-analyzer' issue for this? The value of flatten(X) where X is a type variable was changed in dart-lang/language#2696.

@eernstg eernstg removed the P3 A lower priority bug or feature request label Dec 9, 2022
@scheglov
Copy link
Contributor

scheglov commented Dec 9, 2022

I opened #50672

@eernstg
Copy link
Member Author

eernstg commented Dec 11, 2022

Flatten has been adjusted again in order to handle intersection types, cf. dart-lang/language#2713.

copybara-service bot pushed a commit that referenced this issue Dec 12, 2022
Some expressions have a static type that can lead to a possible
soundness issue when awaited. For those expressions an additional
runtime check is performed.

Issue: #49396
Fixes: #50602
Change-Id: Ief25fbe8c38330cca0c17be4d411780a20ab87a0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/274729
Reviewed-by: Mark Zhou <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>
@eernstg
Copy link
Member Author

eernstg commented Dec 13, 2022

Updated tests have landed, https://dart-review.googlesource.com/c/sdk/+/274682.

@eernstg
Copy link
Member Author

eernstg commented Dec 13, 2022

The language specification updates have landed, cf. dart-lang/language@0cff768.

copybara-service bot pushed a commit that referenced this issue Aug 8, 2023
See #49396 for details.

Fixes: #50601
Change-Id: Ie89130cffe642b3e4935d7d02fe2e34f7fc8b12e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316320
Commit-Queue: Mayank Patke <[email protected]>
Reviewed-by: Stephen Adams <[email protected]>
@eernstg
Copy link
Member Author

eernstg commented Aug 9, 2023

🎉

@eernstg eernstg closed this as completed Aug 9, 2023
copybara-service bot pushed a commit that referenced this issue Aug 9, 2023
This reverts commit c81711b.

Reason for revert: `Internal Error: Runtime type information not available for type_variable_local` - b/295131730

Original change's description:
> [dart2js] Add runtime type check for `await`.
>
> See #49396 for details.
>
> Fixes: #50601
> Change-Id: Ie89130cffe642b3e4935d7d02fe2e34f7fc8b12e
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316320
> Commit-Queue: Mayank Patke <[email protected]>
> Reviewed-by: Stephen Adams <[email protected]>

Change-Id: I481b119b6569d1bc9cf2ab80d997a3eb6d06f674
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319421
Reviewed-by: Alexander Thomas <[email protected]>
Auto-Submit: Oleh Prypin <[email protected]>
Commit-Queue: Oleh Prypin <[email protected]>
copybara-service bot pushed a commit that referenced this issue Aug 9, 2023
This is a reland of commit c81711b

Original change's description:
> [dart2js] Add runtime type check for `await`.
>
> See #49396 for details.
>
> Fixes: #50601
> Change-Id: Ie89130cffe642b3e4935d7d02fe2e34f7fc8b12e
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316320
> Commit-Queue: Mayank Patke <[email protected]>
> Reviewed-by: Stephen Adams <[email protected]>

Change-Id: Ida3258ee3768e8bff0161019511647db8b161473
Bug: #50601
Bug: b/295131730
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319462
Commit-Queue: Mayank Patke <[email protected]>
Reviewed-by: Stephen Adams <[email protected]>
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, ...). implementation Track the implementation of a specific feature (use on area-meta issue, not issues for each tool) soundness
Projects
None yet
Development

No branches or pull requests