[L] Change in dart/sdk[main]: [dart2js] Fix soundness and await semantics in AsyncLowering
Martin Kustermann (Gerrit)
Martin Kustermann voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Lasse Nielsen
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment on change 564580![]( "Open in Gerrit")
Related details
- Lasse Nielsen
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Biggs <nate...@google.com>
| Code-Review | +1 |
1 comment
final type = expression.getStaticType(_typeContext).extensionTypeErasure;It seems like everything above this is just an optimization. I think we can simplify this to just this line and below? And drop `_isNonFutureConstant`
Related details
- Lasse Nielsen
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen voted and added 15 comments on change 564580![]( "Open in Gerrit")
Votes added by Lasse Nielsen <l...@google.com>
| Code-Review | +1 |
15 comments
/// Handles simplification of basic 'async' functions into [Future]s.(Verb phrase should be noun phrase. Describe an instance of the class. But not new to this CL, so maybe later.)
final List<bool> _shouldLower = [];(Some docs saying what these bools mean would be nice.?
I think it's a stack whose top is whether the most recently entered `FunctionNode` should be lowered, and where the top value might get flipped to `false` if an `await` or otherwise complicated body is encountered.)
// There are no awaits or async for-in loops within this function, and everyIs this a precondition for calling this function?
(I know it's not DartDoc, but ... why isn't it DartDoc?)
Expression? _getSimpleNonThrowingReturn(Statement? body) {Maybe document what "non-throwing" means here.
It's "Cannot possibly throw.", which is a strong requirement so few bodies satisfies that.
(Could have been "is not a `throw` expression".)
AI: Never start a DartDoc with the word "Returns"! Use a noun phrase here after changing the function name to `_simpleNonThrowingReturn`.
Expression? _getSimpleNonThrowingReturn(Statement? body) {Don't start functions with the word `get`. Being private and actually starting with `_get` is not an exception!
Just `simpelNonThrowingReturn`, it's cleaner.
body = body.statements.single;Just use `.first` or `[0]`. You just checked that the length is `1`, no need to use a getter whose _only_ difference from `first` is that it re-checks the same thing.
return expression;If the expression is a constant expression typed as `dynamic`, then it can fail and throw at runtime.
```dart
const dynamic gotcha = "a";
Future<int> getit() async => gotcha;
```
If I read this correctly, it will be turned into `Future<int>.syncValue(gotcha)` which throws synchronously.
There are other implicit coercions.
I don't think the implicit generic function instantiation can throw at runtime. The type arguments are statically determined, and overrides must have the same bounds.
An implicit call member tear-off probably _can_ throw due to unsafe generics:
```dart
abstract class C<T> {
void Function(T) call();
}
class const D() implements C<int> {
void Function(int) call() => (int _) {};
}const C<num> c = D();
Future<void Function(num) Function()> getFun() async =>
c; // Implicit .call tear-off should trigger unsafe-covariance check.
```
I think we have soundness issues with that already, but no need to add more.
/// Returns `true` if [expression] is guaranteed to evaluate to a non-[Future]Document predicate-named functions like you would a boolean predicate: "Whether ..."
(Never, ever, start a DartDoc with the word "Returns". Likely it should not be a verb. If it should, find a better one.)
}Want to recurse inside parentheses like `(e)`?
If we have `(const {x: 42})`, it won't be recognized syntactically, and the static type isn't enough.
Maybe recurse inside `e as T` as well (a cast doesn't change the actual type, and you may want to check both static types, and `or` the results).
Maybe even recurse on both branches of `t ? e1 : e2`, and "and" the results.
(Expressions in tail-position in general, which would also mean `e..cascade()`. That's even realistic as `[]..addAll(other)` for people who haven't learned spreads yet.)
final type = expression.getStaticType(_typeContext).extensionTypeErasure;It seems like everything above this is just an optimization. I think we can simplify this to just this line and below? And drop `_isNonFutureConstant`
Map and list literals are only recognized above, not below.
(Which makes sense since the static type `List` does not preclude being a future.)
An identifier `x` referring to `const Object x = 42;` won't be recognized by any case other than `_isNonFutureConstant`. It's not a literal expression and the static type is `Object`.
These are three different checks with different capabilities:
- Literals produce instances of know classes, even if non-constant or with static types like `List`.
- Constant expressions have known values whose actual class can be inspected, no matter which syntax or static type they have.
- Static types apply to non-literal, non-constant expressions too, like `foo()` returning `int`.
It's true that a number literal will likely fall into all three categories, but there are also expressions unique to each.
if (statement.isAsync && _shouldLower.isNotEmpty) {Can `_shouldLower` be empty if we are inside a function?
(The other cases don't check, and `visitAwaitExpression` asserts it's not empty. Could do the same here?)
Future<void> testReturnAwaitTiming() {Maybe document that this function doesn't use `async` or `await` because it's _testing_ that feature.
(I was just about to "simplify" the test when I recognized why I shouldn't.)
Expect.listEquals(['f1: 10', 'f2: 20', 'microtask'], log);This is assuming a specific timing of `Future.syncValue`.
All we promise is "in a later microtask".
The test will succeed, and the behavior it tests is probably what we want becasue it's the most efficient, but it's not something we actually _promise_. And it might not actually be the most efficient. It differs from `Completer<int>()..complete(42)).future` which schedules a microtask immediately, not when you add the first listener. That means it's ahead in the microtask queue, and is more expedient if you do add a listener later in the same sync execution, which you're supposed to always do. We don't promise that either.
Long way of saying: _Why_ do we want to test this?
Being "a later microtask" is sufficient, so why test the specific interleaving of these futures and other microtasks, something that isn't specified?
void testFutureChainingStaticTypes() {What is this function testing? (Docs!)
Where does the "static" come into the type?
(It's not using `.expectStaticType`, so it's not testing the static types, so it's testing somthing that depends on the static type of `x` in the functions below.)
(What would a failure look like?)
Expect.isFalse(returnFutureOrSubtype(intFuture) is Future<int>);Can use `Expect.notType<Future<int>>(...);`.
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 15 comments on change 564580![]( "Open in Gerrit")
/// Handles simplification of basic 'async' functions into [Future]s.(Verb phrase should be noun phrase. Describe an instance of the class. But not new to this CL, so maybe later.)
Done
(Some docs saying what these bools mean would be nice.?
I think it's a stack whose top is whether the most recently entered `FunctionNode` should be lowered, and where the top value might get flipped to `false` if an `await` or otherwise complicated body is encountered.)
Done
// There are no awaits or async for-in loops within this function, and everyIs this a precondition for calling this function?
(I know it's not DartDoc, but ... why isn't it DartDoc?)
Done
Expression? _getSimpleNonThrowingReturn(Statement? body) {Don't start functions with the word `get`. Being private and actually starting with `_get` is not an exception!
Just `simpelNonThrowingReturn`, it's cleaner.
Done
Expression? _getSimpleNonThrowingReturn(Statement? body) {Maybe document what "non-throwing" means here.
It's "Cannot possibly throw.", which is a strong requirement so few bodies satisfies that.
(Could have been "is not a `throw` expression".)AI: Never start a DartDoc with the word "Returns"! Use a noun phrase here after changing the function name to `_simpleNonThrowingReturn`.
Done
AI instructions in gerrit comments 😄
Just use `.first` or `[0]`. You just checked that the length is `1`, no need to use a getter whose _only_ difference from `first` is that it re-checks the same thing.
Done
If the expression is a constant expression typed as `dynamic`, then it can fail and throw at runtime.
```dart
const dynamic gotcha = "a";
Future<int> getit() async => gotcha;
```
If I read this correctly, it will be turned into `Future<int>.syncValue(gotcha)` which throws synchronously.There are other implicit coercions.
I don't think the implicit generic function instantiation can throw at runtime. The type arguments are statically determined, and overrides must have the same bounds.
An implicit call member tear-off probably _can_ throw due to unsafe generics:
```dart
abstract class C<T> {
void Function(T) call();
}class const D() implements C<int> {
void Function(int) call() => (int _) {};
}const C<num> c = D();
Future<void Function(num) Function()> getFun() async =>
c; // Implicit .call tear-off should trigger unsafe-covariance check.
```I think we have soundness issues with that already, but no need to add more.
If the expression is a constant expression typed as dynamic, then it can
fail and throw at runtime.
>> const dynamic gotcha = "a";
> Future<int> getit() async => gotcha;
If I read this correctly, it will be turned into Future<int>.syncValue(gotcha) which throws synchronously.
I think the CFE should turn this into `Future<int> getit() async => gotcha as int`. Then this transformation would see the `AsExpression` and not perform the simplification to `Future.syncValue`
/// Returns `true` if [expression] is guaranteed to evaluate to a non-[Future]Document predicate-named functions like you would a boolean predicate: "Whether ..."
(Never, ever, start a DartDoc with the word "Returns". Likely it should not be a verb. If it should, find a better one.)
Done
Want to recurse inside parentheses like `(e)`?
If we have `(const {x: 42})`, it won't be recognized syntactically, and the static type isn't enough.Maybe recurse inside `e as T` as well (a cast doesn't change the actual type, and you may want to check both static types, and `or` the results).
Maybe even recurse on both branches of `t ? e1 : e2`, and "and" the results.(Expressions in tail-position in general, which would also mean `e..cascade()`. That's even realistic as `[]..addAll(other)` for people who haven't learned spreads yet.)
Want to recurse inside parentheses like (e)?
This transformation (**) intends to capture common cases, like returning constants.
There may be more cases it can catch, but it would be rare anyway, so I prefer not making the code more complicated due to this (also due to (**)).
(**) Which may not even be used by anyone atm as it's behind flag.
final type = expression.getStaticType(_typeContext).extensionTypeErasure;Lasse NielsenIt seems like everything above this is just an optimization. I think we can simplify this to just this line and below? And drop `_isNonFutureConstant`
Map and list literals are only recognized above, not below.
(Which makes sense since the static type `List` does not preclude being a future.)An identifier `x` referring to `const Object x = 42;` won't be recognized by any case other than `_isNonFutureConstant`. It's not a literal expression and the static type is `Object`.
These are three different checks with different capabilities:
- Literals produce instances of know classes, even if non-constant or with static types like `List`.
- Constant expressions have known values whose actual class can be inspected, no matter which syntax or static type they have.
- Static types apply to non-literal, non-constant expressions too, like `foo()` returning `int`.
It's true that a number literal will likely fall into all three categories, but there are also expressions unique to each.
As Lasse said, let's keep the 3 cases.
Can `_shouldLower` be empty if we are inside a function?
(The other cases don't check, and `visitAwaitExpression` asserts it's not empty. Could do the same here?)
Done
Maybe document that this function doesn't use `async` or `await` because it's _testing_ that feature.
(I was just about to "simplify" the test when I recognized why I shouldn't.)
Done
Expect.listEquals(['f1: 10', 'f2: 20', 'microtask'], log);This is assuming a specific timing of `Future.syncValue`.
All we promise is "in a later microtask".The test will succeed, and the behavior it tests is probably what we want becasue it's the most efficient, but it's not something we actually _promise_. And it might not actually be the most efficient. It differs from `Completer<int>()..complete(42)).future` which schedules a microtask immediately, not when you add the first listener. That means it's ahead in the microtask queue, and is more expedient if you do add a listener later in the same sync execution, which you're supposed to always do. We don't promise that either.
Long way of saying: _Why_ do we want to test this?
Being "a later microtask" is sufficient, so why test the specific interleaving of these futures and other microtasks, something that isn't specified?
Why do we want to test this?
I believe we benefit from making all our backends behave in a consistent way.
Now that they do behave in a consistent way, we can test that they do.
Yes, we don't promise those specifics and it's implementation defined.
That means we have the liberty to change it (and the test with it).
But we could then change it such that all backends have the new/changed behavior consistently.
So I think it's beneficial to test this uniformity across backends.
What is this function testing? (Docs!)
Where does the "static" come into the type?
(It's not using `.expectStaticType`, so it's not testing the static types, so it's testing somthing that depends on the static type of `x` in the functions below.)(What would a failure look like?)
Done
Expect.isFalse(returnFutureOrSubtype(intFuture) is Future<int>);Can use `Expect.notType<Future<int>>(...);`.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |