Add runtime type check for 'await' in dart2js

[alexmarkov]

This is a dart2js part of #49396.

According to the spec, await e should check runtime type of e to be a Future<flatten(S)> where S is a static type of e before waiting for e (otherwise it should wait for Future.value(e)). See #49396 for details.

Kernel AST contains the type to check in AwaitExpression.runtimeCheckType.

@sigmundch @rakudrama @fishythefish

[eernstg]

@alexmarkov, @sigmundch, @rakudrama, @fishythefish, the definition of flatten turned out to be too aggressive (causing gratuitous loss of type information), so it was adjusted, cf. #49396 (comment). Sorry about the inconvenience this may cause!

[eernstg]

Friendly ping: All other configurations are passing in https://dart-current-results.web.app/#/filter=language/async/await_flatten_test. Is there something that prevents this soundness issue from being handled in dart2js?

[sigmundch]

Thanks for the ping Erik, I'll make sure we discuss it at our team sync later today.

[eernstg]

Thanks, that's great!

[fishythefish]

Iterating on a fix: https://dart-review.googlesource.com/c/sdk/+/316320

[lrhn]

Looks correct (to the untainted eye).
You don't actually have to call Future.value if you have another, shorter, way to insert an asynchronous delay before evaluating to the original value.
That would be what you currently do when the awaited value is not a future at all.

That is, in the code paths for

• not a future.
• a Future, but not a Future<S>.
• a Future<S>

the first two should behave the same.

I'm guessing the compilation of HAwait does something special to non-futures.
You shouldn't need to do two is Future checks, just one is Future<S> check, where the S can sometimes be omitted.

[eernstg]

It looks good! I think there is one issue ,though, and I added a drive-by comment in the CL.

[lrhn]

@eernstg If it matters whether we create a Future<S> (static type), Future<T> (flatten of S, after checking that the value is not a Future<T>), or just a Future<dynamic> where we ignore the following downcast, shouldn't matter.
If it does, we have a soundness issue.
It's a future which completes with a value of type T in any case, since T <: S.

[eernstg]

We certainly cannot expect that flatten(S) <: S for all S. For example, we do not have int <: Future<int>.

We do have flatten(S) <: S in the case where S is a type of the form FutureOr<U> for some U: In that case we have flatten(S) == U, and we have U <: FutureOr<U> for all U. Also, we have flatten(S) <: S in the case where flatten(S) == S (e.g., flatten(int) == int). Finally we have flatten(S) <: S in some special situations like class S extends Future<Never> {}.

But do we know for sure that we have one of these situations here, without exception?

In any case, it would be safe to use a Future<Object?> because we just need the scheduling effect, no user-written code is even going to get access to that future anyway. Would that be a performance improvement? It should not be a problem to use this in an implementation (and we don't need to change the specification) even though the specification says that we must await Future<T>.value(o), if the difference isn't observable.

[lrhn]

(Ack, we don't actually know T <: S. We know that S <: FutureOr<flatten(S)> for any S (that's how flatten is designed), so let T be flatten(S) and E be the runtime type of the value of e, then E <: S <: FutureOr<T>, and then also E <: T since we know it's not <: Future<T>. So both S and T would be valid types for the future that completes with the same value again.)

The optional implementation would avoid allocating a Future at all, and it would avoid checking for Future<T> here, then checking for Future again later.
If possible, the element type should be passed through to this point instead, so you only do one type check.

[fishythefish]

Thanks for the comments (both here and on the CL)!

I'm updating the CL to use T instead of S to match the spec. (I previously used S because while Future<flatten(S)> is already cached on the AST node, T = flatten(S) isn't. But it's not too hard to extract that or to have the CFE compute it from S.)

AFAIK, we have no mechanism to insert an async delay and evaluate to the original value. Even in async_patch, if _awaitOnObject is called on a non-Future, we construct a _Future and call _thenAwait.

The redundant test in async_patch is likely out of scope for this CL. That code is intended to be called with non-Futures as well (e.g. for async* support). We could probably disentangle it, but it's also non-trivial to pass the element type through. I'll leave a TODO, but the cost of a redundant is Future is anyway dwarfed by the cost of is Future<T> - the former is compiled to instanceof, so it's relatively fast.

[lrhn]

Ack.
A more relevant optimization would be to omit the type check in some cases, where it is not needed.
I don't know what the front end does to detect that.
For example, if the type to check is Future<Object?>, you can omit doing anything, since the later Future check is enough.
Or if the static type is a subtype of Future<T>, the test is also unnecessary, just in the other direction.

If the front end already does these checks, and only sets the check-type of a check is necessary, then you don't need to do anything, but that seems unlikely since it doesn't provide a way to know if it's an always-true or always-false check.

[fishythefish]

My understanding is that the CFE omits runtimeCheckType in the latter case, but not the former. I expect the former case to be optimized by dart2js already - whenever we have a type check using a generic type whose arguments are top types, we omit the type arguments from the check. That is, is Foo<Object?> should always be compiled as if it were is Foo.

@johnniwinther / @chloestefantsova: #49396 has a lot of discussion about when the runtime check can be safely elided. Which cases are implemented by the CFE?

[johnniwinther]

runtimeCheckType is set only when the static type of the operand is not a subtype of Future<T> where T is flatten of the runtime type:

sdk/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart

Lines 830 to 838 in bf9ea56

	DartType operandType = operandResult.inferredType;
	DartType flattenType = typeSchemaEnvironment.flatten(operandType);
	node.operand = operandResult.expression..parent = node;
	DartType runtimeCheckType = new InterfaceType(
	coreTypes.futureClass, libraryBuilder.nonNullable, [flattenType]);
	if (!typeSchemaEnvironment.isSubtypeOf(
	operandType, runtimeCheckType, SubtypeCheckMode.withNullabilities)) {
	node.runtimeCheckType = runtimeCheckType;
	}

[eernstg]

where T is flatten of the runtime type:

Sounds like that should be "flatten of the static type of the operand"? The point would be that, by soundness, there is no need to check e is U at run time if the static type of e is a subtype of U.

[johnniwinther]

It is. Don't know why I wrote "runtime type".

[eernstg]

🎉

[eernstg]

(make that await 🎉)
(Edit: 🎉 completed)

[fishythefish]

@lrhn: Future.value delegates to _Future.immediate. I notice that DDC uses _Future.value instead, which avoids the _asyncComplete and just sets the value. Is this sufficient? (I'm relatively clueless when it comes to async timing details.)

[lrhn]

It should be sufficient to use _Future.value.
It can change timing, but I don't think it will in this case. On the other hand, it avoids doing extra, unnecessary, type checks.

The Future<T>.value constructor takes a FutureOr<T>, calls _asyncComplete with it, which then does type-checks to see if it's a Future<T>, and then (in the value case) takes the non-future branch which schedules a microtask to call _completeWithValue.

That strategy, of scheduling a microtask immediately, ensures that if someone listens to the future before that microtask triggers (which is incredibly likely, the way we encourage futures to be used), then we've already scheduled a task to report the result to them. It minimizes latency by scheduling early, and if more than one listener is added to the same future, we only schedule one microtask, and accumulate listeners on the future until that microtask triggers.

Using _Future.value creates a future that is already completed with a value. That's the same as the state that Future.value ends up in after the microtask completes and existing listeners have been notified.
Every time a listener is added to an already completed future, a new microtask is scheduled, to report the value to the new listener in a later microtask.
If two listeners are added to an already completed future, we schedule two microtasks. (Maybe we should have an internal "future listener notification queue" they could be added to, and only schedule that once, but that's also more overhead when it's not needed.) And the microtasks aren't scheduled unitl you add the listener, which may be slightly later than when the future was created, so potentially more overhead for multiple listeners, and higher latency for later listeners.

But, if you add zero listeners, a completed future does nothing, whereas the Future.value-created one still schedules a microtask to complete it later.

In this case, you will have precisely one listener, added immediately, so I doubt there is any visible difference in timing between the two approaches. Using _Future.value should avoid doing an extra type check on the argument (is Future<T>, which you have just checked to be false), so if you don't inline the constructor, and _asyncComplete, you'll be doing two type checks.

[fishythefish]

Great, thanks, Lasse! This was very informative.