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

"Incompatible with await" and async return. #3599

Closed
lrhn opened this issue Jan 31, 2024 · 4 comments
Closed

"Incompatible with await" and async return. #3599

lrhn opened this issue Jan 31, 2024 · 4 comments

Comments

@lrhn
Copy link
Member

lrhn commented Jan 31, 2024

Proposal

For a return e in an async function, if the static type of e is incompatible with await, then the runtime semantics will never await. The static type of e must be directly assignable to the future value type of the function.

Background

We've decided that a extension type which does not implement Future is incompatbile with await, which makes it an error to await it (or things bounds by that or made nullable, as many times as we wan't to wrap types up).

It's not clear how that should affect returns in an async function which has an implicit (and optional) await.

Take just:

extension type Ext(Object? _) {}
Future<Ext> foo() async {
   /// ...
   return Ext(someValue);
}

The type inference and specified behavior is to infer Ext(someValue) with context type FutureOr<Ext>,
then at runtime check if the value implements Future<Ext>, and if so, await that future.

Here Ext is an extension type which doesn't implement Future, so awaiting it is precisely what we prohibit for await.
If someValue happens to implement Future, then using the currently specified behavior, it wll be awaited, blindy awaiting the representation value of an extension type which doesn't implement Future, which was what we wanted to prevent.

Then the function body of the example above will not need to check whether the value is a Future<Ext> (aka, Future<Object?> at runtime), and will not await anything if it is.

Implementation issues

This requires the compilers to know that the type is incompatible with await, and support not awaiting it.
For the former, I believe an async function return from the CFE currently carries the future value type to check for (the flatten of the static type of e).
If the getter for that future value type could be made nullable, a value of null would signal the compiler to not check or await in that particular return. Then the compiler won't need to do the "compatible with await" computation again.

(If the backends delegate the awaiting to _Future._complete on the returned future of the async function, they should use _Future._completeWithValue instead. Most likely the code already uses _completeWithValue, since the future type to check for may not be the same as the future value type of the returned future.)

Example

A concrete example where this could be relevant (similar to an extension type I have already written):

extension type Result<T>._(FutureOr<T> _) { ... }
// ...
class _ErrorHack implements Future<Never> { _ErrorHack(this.error, this.stackTrace; ... }
extension type Error<T>._(_ErrorHack _) implements Result<T> { 
  Error(Object error, StackTrace stack) : this._(_ErrorHack(error, stack));
 }

Future<Result<int>> foo() async { 
   ... return Error(e, s);
}

Using the current async return behavior would await the _ErrorHack "future", no matter which future type is checked for, beause it implements Future<Never>.

And indeed:

Future<Result<R>> captureAsync<R>(Future<R> computation) async {
  try {
    var value = await computation;
    return Result<R>.value(value);
  } catch (e, s) {
    return Error<R>(e, s);
  }
}

This function fails to capture an error fomr the computation into a value, because when it tries to return that value, it's immediately recognized as a future and implicitly awaited, raising the error again as an error.
The returned future is completed with the error, not with the Error value.

However, this is equally much a problem with using synchronous code and Completer.complete/Future.then, because those functions also does the implicit await on any valid future. (That's why returns did it originally, they were implemented using Completer.complete).
So, we can't actually prevent such a non-future extension type carrying a future value from being awaited in a lot of situations.

And because of that,

Future<Result<R>> captureSync<R>(Future<R> computation) {
  return computation.then(Result<R>.value, onError: Result<R>.error);
}

has precisely the same behavior as the async version.

We can, and probably should, improve the APIs to have Completer<T>.completeValue(T value), Future.map<R>(R Function(T), {R Function(Object, StackTrace)? onError}) and similar functions that ensure a value is passed through unmodified, even if it could potentially be a future. The FutureOr<T> type is a liability any time T can contain a value of type Future<T>.

For now, I think we should close the return loop-hole past "incompatbile with await" here, as described above. Using async functions is what most people do, and getting that right is important. People using Future methods and completers may have similar issues, but they are (even if just slightly) more sophisticated users, and may be able to understand the workarounds needed (which is currently wrapping the value in a future, which can be unwrapped instead of the future we want to preserve:

Future<Result<R>> captureSync<R>(Future<R> computation) {
  return computation.then(Result<R>.value, onError: (Object error, StackTrace stack) => Future<Result<R>>.value(Result<R>.error(error, stack)));
}

Then work towards #870, to get rid of the implicit await, and add helper methods to Future and Completer which makes it possible to complete with a Future value that won't be awaited.

@lrhn lrhn added request Requests to resolve a particular developer problem extension-types and removed request Requests to resolve a particular developer problem labels Jan 31, 2024
@lrhn
Copy link
Member Author

lrhn commented Jan 31, 2024

@johnniwinther @mraleph @rakudrama @osa1

The proposal is that an return e in an async function, where the static type of e is "incompatible with await", will not await the expression. It will be returned as-is, and eventually be used to complete the returned future using _Future._completeWithValue (unless intercepted by a finally).

How complicated do you think this would be to implement, should we choose to go for it?

We're very, very late in the release process, so it has to be really trivial to make it before the first patch.
We are only implementing the "incompatible with await" restriction now, but that should be only front-end code (it just introduces a compile-time error, no runtime behavior changes).

My suggestion is that the CFE marks the return as "not awaited" somehow. (Maybe a null value for the type of future to check for before awaiting, which I believe the return carries.)

Then backends need to recognize this signal and skip the code to check-and-await a future value.
Which should be easy if that is just code inserted at the return, simply not putting it in would be enough, but could be harder if it just calls out to some general helper function which does the awaiting, and doesn't understand not awaiting.

@johnniwinther
Copy link
Member

@chloestefantsova How easy do you think the suggested CFE change will be?

@chloestefantsova
Copy link
Contributor

@johnniwinther We've just discussed this issue with @lrhn and concluded that the most work needs to be done by the backends. Currently, we don't instruct the backends to await on the return expressions, and we don't have any way to communicate to them that the await should be skipped. We certainly can add a boolean flag to the return statement that will inform the backends that the await can be skipped though, to simplify their part of work a bit. Do you think we should do that?

@lrhn
Copy link
Member Author

lrhn commented Jan 31, 2024

Looking at the backend code with my untrained eyes, I think the ones I've checked may not be inserting any await code, and are relying on the flattening of Completer.complete or a similar function, which gets invoked only at the exit of the function, independently of where the return came from.

That would make it hard to implement a per-return "don't await". (Or easy, it just have to pre-wrap in a Future<ExpressionType>(value), which will then be the future that is unwrapped.
(The dart2js async lowerings are more worrisome, since they desugart some async functions to synchronous functions returninng Futures, and then call Future.sync with those functions, which gives some number of unwrappings that apparently add up correctly.)

Which all comes back to dart-lang/sdk#44395 - the implicit await is not implemented correctly, it happens after the return, not at the return.
And that probably makes it impossible to do anything about this issue, and #870 is the only thing that will work.

So, probably not going to do this.

@lrhn lrhn closed this as not planned Won't fix, can't repro, duplicate, stale Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants