Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2258313002:
[async functions] Disallow 'await' in arrow params inside async functions (Closed)
Description[async functions] Disallow 'await' in arrow params inside async functions
The following code was previously accepted:
async function f() {
let g = (await) => {};
}
But per the spec, using 'await' is disallowed in arrow parameters
by an early error rule (just as 'yield' is disallowed in arrow
params inside generators).
There was special logic in ParseUnaryExpression which seems to have been
there only to allow that case. Having removed it, we get a SyntaxError in
the right cases anyway when ParseUnaryExpression chokes on whatever
illegal token follows 'await' in the cases this code previously handled.
Also removes the unnecessary AsyncBindingPatternProduction enum value.
R=caitp@igalia.com, littledan@chromium.org
BUG=v8:4483
Committed: https://crrev.com/232a33602bd6fff2cd381927d94ba9e86cf9a6e5
Cr-Commit-Position: refs/heads/master@{#38802}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Different approach with more tests #
Total comments: 2
Patch Set 3 : Better testing #Patch Set 4 : Rebased #
Messages
Total messages: 32 (18 generated)
The CQ bit was checked by adamk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2258313002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2258313002/diff/1/src/parsing/parser-base.h#o... src/parsing/parser-base.h:936: if (!classifier->is_valid_binding_pattern() || If this passes tests, it looks great. At some point, this was definitely needed to make `var { ... await ... }` / var [ ... await ... ]` throw within async functions. Maybe that's not true anymore. I guess there's no reason the normal BindingPattern handler can't do this. https://codereview.chromium.org/2258313002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2258313002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2786: classifier->RecordArrowFormalParametersError( good find!
https://codereview.chromium.org/2258313002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (left): https://codereview.chromium.org/2258313002/diff/1/src/parsing/parser-base.h#o... src/parsing/parser-base.h:936: if (!classifier->is_valid_binding_pattern() || On 2016/08/19 23:04:34, caitp wrote: > If this passes tests, it looks great. At some point, this was definitely needed > to make `var { ... await ... }` / var [ ... await ... ]` throw within async > functions. Maybe that's not true anymore. > > I guess there's no reason the normal BindingPattern handler can't do this. Yeah, that was exactly my thinking (and it seems to be true; I still need the call to RecordBindingPatternError, it just doesn't have to be separate from other binding pattern errors). The same may actually be true of ExpressionError vs CoverInitializedNameError, see discussion in gsathya's https://codereview.chromium.org/2255353002/. Some day I'll understand all the uses of ExpressionClassifier :)
The CQ bit was unchecked by adamk@chromium.org
The CQ bit was checked by adamk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by adamk@chromium.org
The CQ bit was checked by adamk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [async functions] Disallow 'await' in arrow params inside async functions Also remove unnecessary AsyncBindingPatternProduction enum and supporting code from ExpressionClassifier, as well as an unnecessary call to RecordExpressionError. R=caitp@igalia.com, littledan@chromium.org BUG=v8:4483 ========== to ========== [async functions] Disallow 'await' in arrow params inside async functions There was special logic in ParseUnaryExpression which seems to have been there only to allow that case. As it's illegal per the early errors in the spec, we can just remove it. We get a SyntaxError in the right cases anyway when ParseUnaryExpression chokes on whatever illegal token follows 'await' in the cases this code previously handled. Also removes the unnecessary AsyncBindingPatternProduction enum value. R=caitp@igalia.com, littledan@chromium.org BUG=v8:4483 ==========
Different approach with more test coverage, ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I find this version of the grammar counter-intuitive, and I'm having trouble finding which early error in the spec you are referring to. Could you provide a little more detail about why this is banned in the commit message?
lgtm Never mind, this is because CoverCallExpressionAndAsyncArrowHead is parametrized by [Await], which leads to banning await as an identifier in an async arrow head just like elsewhere. Easier to spec, easier to implement, seems fine since you're aware of await existing when you're parsing an arrow function within an async function. Code looks good. https://codereview.chromium.org/2258313002/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2258313002/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:7895: "async function f() { return (await = 42) }", Consider adding tests for other kinds of async functions, e.g. async concise bodies.
On 2016/08/22 01:35:20, Dan Ehrenberg wrote:
> I find this version of the grammar counter-intuitive, and I'm having trouble
> finding which early error in the spec you are referring to. Could you provide
a
> little more detail about why this is banned in the commit message?
The old code went out of its way to allow `await` in arrow formal parameters
within an async function
EG:
```
async function foo() {
var f = (await) => await; // Should be a SyntaxError
return f;
}
```
This is covered in the spec by these points:
-
https://tc39.github.io/ecmascript-asyncawait/#identifier-static-semantics-ear...
- https://tc39.github.io/ecmascript-asyncawait/#async-function-definitions
I agree though, the cl description could be more descriptive.
Description was changed from ========== [async functions] Disallow 'await' in arrow params inside async functions There was special logic in ParseUnaryExpression which seems to have been there only to allow that case. As it's illegal per the early errors in the spec, we can just remove it. We get a SyntaxError in the right cases anyway when ParseUnaryExpression chokes on whatever illegal token follows 'await' in the cases this code previously handled. Also removes the unnecessary AsyncBindingPatternProduction enum value. R=caitp@igalia.com, littledan@chromium.org BUG=v8:4483 ========== to ========== [async functions] Disallow 'await' in arrow params inside async functions The following code was previously accepted: async function f() { let g = (await) => {}; } But per the spec, using 'await' is disallowed in arrow parameters by an early error rule (just as 'yield' is disallowed in arrow params inside generators). There was special logic in ParseUnaryExpression which seems to have been there only to allow that case. Having removed it, we get a SyntaxError in the right cases anyway when ParseUnaryExpression chokes on whatever illegal token follows 'await' in the cases this code previously handled. Also removes the unnecessary AsyncBindingPatternProduction enum value. R=caitp@igalia.com, littledan@chromium.org BUG=v8:4483 ==========
https://codereview.chromium.org/2258313002/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2258313002/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:7895: "async function f() { return (await = 42) }", On 2016/08/22 01:48:53, Dan Ehrenberg wrote: > Consider adding tests for other kinds of async functions, e.g. async concise > bodies. I've refactored this test to have a section for statements inside a variety of async function forms, and put these tests in that section.
The CQ bit was checked by adamk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com, littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2258313002/#ps40001 (title: "Better testing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11367) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by adamk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com, littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2258313002/#ps60001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[async functions] Disallow 'await' in arrow params inside async functions
The following code was previously accepted:
async function f() {
let g = (await) => {};
}
But per the spec, using 'await' is disallowed in arrow parameters
by an early error rule (just as 'yield' is disallowed in arrow
params inside generators).
There was special logic in ParseUnaryExpression which seems to have been
there only to allow that case. Having removed it, we get a SyntaxError in
the right cases anyway when ParseUnaryExpression chokes on whatever
illegal token follows 'await' in the cases this code previously handled.
Also removes the unnecessary AsyncBindingPatternProduction enum value.
R=caitp@igalia.com, littledan@chromium.org
BUG=v8:4483
==========
to
==========
[async functions] Disallow 'await' in arrow params inside async functions
The following code was previously accepted:
async function f() {
let g = (await) => {};
}
But per the spec, using 'await' is disallowed in arrow parameters
by an early error rule (just as 'yield' is disallowed in arrow
params inside generators).
There was special logic in ParseUnaryExpression which seems to have been
there only to allow that case. Having removed it, we get a SyntaxError in
the right cases anyway when ParseUnaryExpression chokes on whatever
illegal token follows 'await' in the cases this code previously handled.
Also removes the unnecessary AsyncBindingPatternProduction enum value.
R=caitp@igalia.com, littledan@chromium.org
BUG=v8:4483
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
[async functions] Disallow 'await' in arrow params inside async functions
The following code was previously accepted:
async function f() {
let g = (await) => {};
}
But per the spec, using 'await' is disallowed in arrow parameters
by an early error rule (just as 'yield' is disallowed in arrow
params inside generators).
There was special logic in ParseUnaryExpression which seems to have been
there only to allow that case. Having removed it, we get a SyntaxError in
the right cases anyway when ParseUnaryExpression chokes on whatever
illegal token follows 'await' in the cases this code previously handled.
Also removes the unnecessary AsyncBindingPatternProduction enum value.
R=caitp@igalia.com, littledan@chromium.org
BUG=v8:4483
==========
to
==========
[async functions] Disallow 'await' in arrow params inside async functions
The following code was previously accepted:
async function f() {
let g = (await) => {};
}
But per the spec, using 'await' is disallowed in arrow parameters
by an early error rule (just as 'yield' is disallowed in arrow
params inside generators).
There was special logic in ParseUnaryExpression which seems to have been
there only to allow that case. Having removed it, we get a SyntaxError in
the right cases anyway when ParseUnaryExpression chokes on whatever
illegal token follows 'await' in the cases this code previously handled.
Also removes the unnecessary AsyncBindingPatternProduction enum value.
R=caitp@igalia.com, littledan@chromium.org
BUG=v8:4483
Committed: https://crrev.com/232a33602bd6fff2cd381927d94ba9e86cf9a6e5
Cr-Commit-Position: refs/heads/master@{#38802}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/232a33602bd6fff2cd381927d94ba9e86cf9a6e5 Cr-Commit-Position: refs/heads/master@{#38802} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
