Wrong async return semantics

[eernstg]

[Edit: This issue is blocked on dart-lang/language#870; see this comment for details.]

Consider the following program:

Future<void> f() async {
  throw 'f';
}

Future<void> g() async {
  try {
    return f();
  } catch (e) {
    print('Caught "$e"');
  }
}

void main() async {
  await g(); // Should print 'Caught "f"'.
}

Execution of this program with dart (from 74be667, and 2.12.0-96.0.dev) gives rise to the following output:

Unhandled exception:
f
#0      f (file:///usr/local/google/home/eernst/lang/dart/scratch/202012/n011.dart:2:3)
dart-lang/sdk#57147      g (file:///usr/local/google/home/eernst/lang/dart/scratch/202012/n011.dart:7:12)
dart-lang/sdk#57148      main (file:///usr/local/google/home/eernst/lang/dart/scratch/202012/n011.dart:14:9)
dart-lang/sdk#57149      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:283:19)
dart-lang/sdk#57150      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

However, the semantics of return f() should have been as follows:

When $f$ is an asynchronous non-generator with future value type $T_v$
(\ref{functions}), evaluation proceeds as follows:

($f$ is the function whose body contains the return statement)

The expression $e$ is evaluated to an object $o$.

(In this case, $o$ is an instance of a subtype of Future whose type argument at Future is void.)

If the run-time type of $o$ is a subtype of \code{Future<$T_v$>},

($T_v$ is the future value type of $f$, that is, void. And the subtype relation does hold.)

let \code{v} be a fresh variable bound to $o$ and
evaluate \code{\AWAIT{} v} to an object $r$;
otherwise let $r$ be $o$.
...

The evaluation of said await v will throw, and hence we should proceed to execute the print statement and return from g by completing the body normally (and implicitly completing the returned future with the null object).

The actual behavior indicates that no such evaluation of await v occurs, and the returned future is completed with the thrown exception, so await g() in main throws.

In legacy code the await occurs at the same point, with similar spec language, since commit 5af9844 of Sep 17, 2018.

[johnniwinther]

Given that the expected behavior is depending on the runtime type this is likely a backend issue.

The AST generated from CFE is

library /*isNonNullableByDefault*/;
import self as self;
import "dart:async" as asy;
import "dart:core" as core;

static method f() → asy::Future<void> async {
  throw "f";
}
static method g() → asy::Future<void> async {
  try {
    return self::f();
  }
  on core::Object catch(final core::Object e) {
    core::print("Caught \"${e}\"");
  }
}
static method main() → void async {
  await self::g();
}

and I don't think we should do anything differently.

[eernstg]

I actually get a different output, based on

> FRONT_END_DIR=$HOME/devel/dart/work/sdk/pkg/front_end
> $FRONT_END_DIR/tool/fasta compile --dump-ir n011.dart

The complete output looks like this.

library /*isNonNullableByDefault*/;
import self as self;
import "dart:async" as asy;
import "dart:core" as core;
import "dart:_internal" as _in;

static method f() → asy::Future<void> /* originally async */ {
  final asy::_Future<void> :async_future = new asy::_Future::•<void>();
  core::bool* :is_sync = false;
  FutureOr<void>? :return_value;
  (dynamic) → dynamic :async_op_then;
  (core::Object, core::StackTrace) → dynamic :async_op_error;
  core::int :await_jump_var = 0;
  dynamic :await_ctx_var;
  function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
    try {
      #L1:
      {
        throw "f";
      }
      asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
      return;
    }
    on dynamic catch(dynamic exception, core::StackTrace stack_trace) {
      asy::_completeOnAsyncError(:async_future, exception, stack_trace, :is_sync);
    }
  :async_op_then = asy::_asyncThenWrapperHelper(:async_op);
  :async_op_error = asy::_asyncErrorWrapperHelper(:async_op);
  :async_op.call();
  :is_sync = true;
  return :async_future;
}
static method g() → asy::Future<void> /* originally async */ {
  final asy::_Future<void> :async_future = new asy::_Future::•<void>();
  core::bool* :is_sync = false;
  FutureOr<void>? :return_value;
  (dynamic) → dynamic :async_op_then;
  (core::Object, core::StackTrace) → dynamic :async_op_error;
  core::int :await_jump_var = 0;
  dynamic :await_ctx_var;
  function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
    try {
      #L2:
      {
        try {
          :return_value = self::f();
          break #L2;
        }
        on core::Object catch(final core::Object e) {
          core::print("Caught \"${e}\"");
        }
      }
      asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
      return;
    }
    on dynamic catch(dynamic exception, core::StackTrace stack_trace) {
      asy::_completeOnAsyncError(:async_future, exception, stack_trace, :is_sync);
    }
  :async_op_then = asy::_asyncThenWrapperHelper(:async_op);
  :async_op_error = asy::_asyncErrorWrapperHelper(:async_op);
  :async_op.call();
  :is_sync = true;
  return :async_future;
}
static method main() → void /* originally async */ {
  final asy::_Future<dynamic> :async_future = new asy::_Future::•<dynamic>();
  core::bool* :is_sync = false;
  FutureOr<dynamic>? :return_value;
  (dynamic) → dynamic :async_op_then;
  (core::Object, core::StackTrace) → dynamic :async_op_error;
  core::int :await_jump_var = 0;
  dynamic :await_ctx_var;
  dynamic :saved_try_context_var0;
  function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
    try {
      #L3:
      {
        [yield] let dynamic #t1 = asy::_awaitHelper(self::g(), :async_op_then, :async_op_error, :async_op) in null;
        _in::unsafeCast<void>(:result);
      }
      asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
      return;
    }
    on dynamic catch(dynamic exception, core::StackTrace stack_trace) {
      asy::_completeOnAsyncError(:async_future, exception, stack_trace, :is_sync);
    }
  :async_op_then = asy::_asyncThenWrapperHelper(:async_op);
  :async_op_error = asy::_asyncErrorWrapperHelper(:async_op);
  :async_op.call();
  :is_sync = true;
  return :async_future;
}

Here is the crucial part, as I see it:

    try {
      #L2:
      {
        try {
          :return_value = self::f();
          break #L2;
        }
        on core::Object catch(final core::Object e) {
          core::print("Caught \"${e}\"");
        }
      }
      asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
      return;
    }
    on dynamic catch(dynamic exception, core::StackTrace stack_trace) {
      asy::_completeOnAsyncError(:async_future, exception, stack_trace, :is_sync);
    }

The point is that :return_value is assigned the value of self::f() which is the Future<void>, and then _completeOnAsyncReturn will complete :async_future with the exception, rather than throwing at the return statement and catching 4 lines further down.

[johnniwinther]

The difference is due to the VM transformation applied. My example was with --target=none which is without the VM transformation and which matches the AST provided to dart2js and DDC. I checked dart2js also has the wrong semantics, so the problem doesn't lie in the VM transformation.

Would be valid to replace self::f() with await self:f(), and if so, should we do that in the CFE based on static types?

[eernstg]

It's is typically valid to replace self::f() by await self::f(), but the await should not be executed when self::f() returns a future whose type argument does not ensure that the value is a type-correct return value. E.g.:

Future<int> f() {
  return Future<num>.value(1) as dynamic;
}

This should evaluate the returned expression, detect that it is not a subtype of Future<int>, and then encounter a dynamic error (so it should not blindly await the 1 and succeed, because the returned expression yields a future).

So in the case where the returned expression has a type that guarantees that it will be a Future<T> where T is the future value type of the enclosing function, it would be safe to add the wait, but I don't know whether that's useful when it doesn't work in all cases.

I checked dart2js also has the wrong semantics, so the problem
doesn't lie in the VM transformation.

Perhaps it could be a problem in the VM transformation as well as in the back-end processing done by dart2js?

[johnniwinther]

Given that it doesn't work in all cases we need backends to fix this.

I think we should file issue for each backend and make this a meta issue.

[eernstg]

OK!

[johnniwinther]

I haven't checked whether DDC handles this correctly.

[mkustermann]

@lrhn Is this (i.e. is auto-insertion of await for returned values) actually intended? Addressing this change in semantics might make performance worse.

[eernstg]

We always had an await for returned values in async functions, but it used to kick in for every future. With null safety we only await futures whose type argument guarantees that they will be completed with an object which is of the required type.

(Edit: This change was actually not introduced with null safety, it was already Sep 17 2018, in commit 5af9844.)

But in this case the issue is that the await "leaks out of" the try block, so we don't catch the exception as required. The total number of awaits shouldn't change.

[eernstg]

Test out for review here.

[lrhn]

Yes, it's intended because the alternative, leaking an error past a try/catch, is too confusing.

Looking at the type of the future was something we changed for Dart 2.0, when we stopped recursively flattening the type, because otherwise we had an unsound type system. It might not have been implemented at the time.

There shouldn't be any change needed for functions where the return is not inside a try/catch, and where the type of the returned expression is either T or Future<T> already (unless Future<T> is a subtype of T).

(I really, really want to deprecate the ability to return anything but a T from an async function with return type of Future<T>, removing the implicit await and type check at the return. All the extra type checking we do is horribly complicated and doesn't do anything the user can't get by inserting an await when necessary. Then we won't do it when it's not necessary.)

[natebosch]

(I really, really want to deprecate the ability to return anything but a T from an async function with return type of Future<T>, removing the implicit await and type check at the return. All the extra type checking we do is horribly complicated and doesn't do anything the user can't get by inserting an await when necessary. Then we won't do it when it's not necessary.)

That is dart-lang/language#870 right?

I'm also in favor of that. Would it be worth pushing on that instead of changing backend behavior? We could manage the breaking change with language versioning if we require the await. If we implicitly await when we didn't used to, it may break some unsuspecting code that relied on the bug. I think that risk is low though, so it would also be fine to change backends now and make the await required later.

[leafpetersen]

@natebosch @lrhn @johnniwinther the fix for this caused breakage internally. I think it's being fixed forward for now, but that does suggest that we may see additional runtime breakage in the future when folks opt in. Externally, opting in a package may be breaking in a way that folks are expecting. Do we still think this is sufficiently valuable (and sufficiently non-breaking) to try to land? If so, we should probably call this out explicitly in the CHANGELOG and possibly in a breaking change announcement.

[natebosch]

The impact wasn't very widespread - but more happened than I would have guessed. This also looks like it can break things in a really subtle way that is hard to track down.

I lean towards reverting this and getter a better understanding of the impact before relanding or choosing to do something different.

[natebosch]

This got reverted and I think backends are still inconsistent with the spec, and we have tests in the SDK repo for the specced behavior.

@lrhn @eernstg - is this something we should still push on? Should we put more attention towards dart-lang/language#870 instead?

[lrhn]

I can specify the current behavior, I just don't want to 😁.
(A return e; is valid if the value implements FutureOr<F> where F is the future value type of the function. It completes by returning that value. When an async method body completes by returning a value, we then check if that value is a future (of the right type) and if so, waits for it before completing the async* function invocation's associated future with the same result. If it's not such a future, we complete the associated future with the value.)

Doesn't matter, because it's bad semantics, and we should fix it.

[eernstg]

@lrhn wrote:

we should fix it.

👍

[eernstg]

@natebosch wrote (about adopting dart-lang/language#870):

If we think it's worth the statically user visible breaking change at some
point, it might be easier to do that directly rather than first do a runtime
only breaking change and later the static breaking change.

I think dart-lang/language#870 is attractive because it allows us to simplify type inference for returned expressions, which may well provide a better developer experience (because type inference could succeed in some situations where it currently fails).

However, dart-lang/language#870 is not only a change that will require the addition of await on a statically known set of return statements, so we can't really say that it is only a 'static breaking change'.

In particular, any return e; in an async function body where e has type dynamic will remain compilable, but it could start throwing now and then (because e could evaluate to a future, and that future cannot be returned any more). We can't just statically find all those locations and change them to return await e;, because that may change the semantics: We would have to check and only perform the await when the given object is a Future<T> where T is the future value type of the enclosing function.

It is true that dart-lang/language#870 would eliminate the situation where a return e; has an e with static type Future<T> (or a subtype thereof) where T is the future value type. That's the case where we can statically determine that there must be an implicit await, and with dart-lang/language#870 this must be an explicit await.

However, that would give rise to exactly the behavioral changes that the fix to this issue caused.

In other words, the difficulties that arose when 14032a6 was landed must be handled in any case as part of the migration to dart-lang/language#870, so it's not like doing dart-lang/language#870 makes that issue go away.

[natebosch]

must be handled in any case as part of the migration to dart-lang/language#870, so it's not like doing dart-lang/language#870 makes that issue go away.

It would let us make those fixes incrementally instead of forcing them to be fixed before we release the change.

Fixing the existing semantics without the static breaking change should be fine too. If we plan on doing that we might want to track on our OKRs.

[nshahan]

I have added a skip for this test on the DDC firefox bot because it was flaking at a rate that was avoiding flake detection but consistently turning the bot red and requiring new approval d6db39e. We should remove the skip if we reland the change in the CFE or some other implemenation.

[mraleph]

@leafpetersen @eernstg @lrhn This needs an owner. We have a test which is failing for 3 years and nobody owns it.

As I understand it - fixing this is not a problem (@johnniwinther had a patch), but rolling it out is complicated. So we need to make a decision here: we need to either commit to fixing it at some point or commit to never fixing it and change the spec instead. The current equilibrium where we just have a failing test and ignore it (because we can't see it) is not a good state.

[eernstg]

We could also introduce a warning for the situation where the wrong semantics occurs, instructing developers to replace return e; by return await e;.

The wrong semantics only occurs when the return statement is located inside a try/catch statement, and the evaluation of e yields an object whose type implements Future<T> for a T which is a subtype of the future value type of the enclosing function, and that future is completed with an error.

We don't immediately have a safe approximation of this situation based on the static type of e, but it might be OK to emit that warning with some false positives.

This approach would make the semantics modification explicit for each location where it occurs, and developers would be in a rather good position to detect that the change from return e; to return await e; is the culprit, if some bad behavior emerges after this change.

With this change in place (in "almost" all serious code out there), we're free to change the implementation such that the behavior is as specified. Note that return await e; works the same before and after we do that. That is, we can change the implementations to behave as specified at these return statements, and it won't break anything.

By the way, we are also free to say that "this is wonderful! now we're halfway done with dart-lang/language#870!", because developers have already done some of the migration to that proposal, and in particular they've already done the kind of migration that is most likely to cause subtle bugs (which is exactly the insertion of await at these particular return statements).

[lrhn]

Much as I hate to bring it up, inserting await it's not a complete no-op since it can change the context type of the following expression.
A return Future(dynamicValue); with a future value type of T infers s Future<T> (I hope, based on a context type of FutureOr<T>).
Adding an extra await will, I believe, change the context type to FutureOr<FutureOr<T>>.

(But I'd also prefer to completely remove the await in async returns, and, honestly, change the behavior of await in general. Then this problem should go away, and hopefully a few more. But that's harder. Until then, just guiding people away from the problematic implementation should be easy.)

[natebosch]

But I'd also prefer to completely remove the await in async returns

I prefer to require return await as in dart-lang/linter#870 than to remove the keyword from returns.

[mraleph]

Just want to check if we have concrete plan on how we would like to move this forward?

[eernstg]

It is on the agenda of the language team for tomorrow (it was there last week, too, but we should actually get around to it this time).

[eernstg]

The language team just decided that we'd like to explore the proposal in dart-lang/language#870, which implies that it will no longer be possible to return a future which must be implicitly awaited. (That is, return e; will need to be changed to return await e; when e must be awaited.) We do not promise that dart-lang/language#870 will be accepted into the language, but it is likely.

With that in mind, it has low priority to eliminate the wrong semantics reported in this issue (that is: don't do it ;-).

I've marked this issue as blocked (on dart-lang/language#870) to give yet another hint that it is dormant.

Please use dart-lang/language#870 to follow the exploration of that proposal.

This issue stays open in case the above mentioned exploration runs into serious difficulties. If dart-lang/language#870 works well then this issue will be moot and we can close it.

[gmpassos]

Please, consider allowing the return of Future (or anything) without await for a method returning FutureOr to avoid disrupting optimizations.

For use cases, see:
https://pub.dev/packages/async_extension

[lrhn]

The current behavior of return future is specified as not introducing an extra delay for a non-future value. Unlike plain await, it's not specified to wrap the value in a future and then await that.
I have no idea whether that is implemented (because this bug is about not implementing the specified semantics).
That doesn't guarantee that the returned future is completed synchronously by the return, though. It might be, and it might not be, the specification doesn't say either way.

It's quite possible that there is a synchronous completion of the returned future with a value, because that's what we do for normal value returns, because we can, but it's never been specified that that a return of a value from an async function will complete the returned future synchronously, and it's always been optimistic to rely on that.
All that's promised is that the returned future will "complete at some later time" (maybe even in some later microtask).
It's possibly implemented so that it's the very next microtask, as long as the return is after an await.

So, don't assume synchronous completion, not even for returning a plain value. It's not guaranteed.
Optimizations that can be so easily disrupted, are dangerous.

[iapicca]

I suspect fixing this issue may solve

• Warn when returning a function invocation when within a try block in an async function #58279