Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091313002:
[parser] don't report error for CoverInitializedNames in async arrow formals (Closed)
|
Created:
4 years, 6 months ago by caitp (gmail) Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[parser] don't report error for CoverInitializedNames in async arrow formals
BUG=v8:4483, v8:5148
R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com, nikolaos@chromium.org
Committed: https://crrev.com/4bb1f70e66ef361f42fdb6082cd4db9def77befc
Cr-Commit-Position: refs/heads/master@{#37260}
Patch Set 1 #
Total comments: 8
Patch Set 2 : nits #Patch Set 3 : [parser] don't report error for CoverInitializedNames in async arrow formals #
Total comments: 1
Patch Set 4 : Fix compile error #Patch Set 5 : _Really_ fix compile error #
Messages
Total messages: 25 (8 generated)
PTAL, a fix for the bug I reported last night (re: CoverInitializedNames in async arrow formal parameters). I'm hoping this doesn't hurt performance for the common case too badly.
https://codereview.chromium.org/2091313002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2091313002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2203: if ((!maybe_arrow || peek() != Token::ARROW)) { Nit: remove extra parens https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7674: "async({ foo = 1 })", Maybe verify that await is banned in this context https://codereview.chromium.org/2091313002/diff/1/test/mjsunit/harmony/async-... File test/mjsunit/harmony/async-await-basic.js (right): https://codereview.chromium.org/2091313002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-await-basic.js:357: () => (async(foo, { a = NaN }) => foo + a)("1", { a: "0" })); If you're writing tests for defaults, maybe let a: undefined to see that?
https://codereview.chromium.org/2091313002/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2091313002/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:2203: if ((!maybe_arrow || peek() != Token::ARROW)) { On 2016/06/24 17:30:41, Dan Ehrenberg wrote: > Nit: remove extra parens Done https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7674: "async({ foo = 1 })", On 2016/06/24 17:30:41, Dan Ehrenberg wrote: > Maybe verify that await is banned in this context why would await be banned in this context, other than in a module? `async({ await = 1 }) => 1` would be an error, but is currently only reported as an error in the full parser, not the preparser, so I think that's a separate issue (similar to other cases where arrow functions don't produce early errors in the preparser). https://codereview.chromium.org/2091313002/diff/1/test/mjsunit/harmony/async-... File test/mjsunit/harmony/async-await-basic.js (right): https://codereview.chromium.org/2091313002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-await-basic.js:357: () => (async(foo, { a = NaN }) => foo + a)("1", { a: "0" })); On 2016/06/24 17:30:41, Dan Ehrenberg wrote: > If you're writing tests for defaults, maybe let a: undefined to see that? Done
https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7674: "async({ foo = 1 })", On 2016/06/24 at 18:05:40, caitp wrote: > On 2016/06/24 17:30:41, Dan Ehrenberg wrote: > > Maybe verify that await is banned in this context > > why would await be banned in this context, other than in a module? > > `async({ await = 1 }) => 1` would be an error, but is currently only reported as an error in the full parser, not the preparser, so I think that's a separate issue (similar to other cases where arrow functions don't produce early errors in the preparser). test-parsing.cc has a way of invoking just the parser. We should take advantage of that mode to test these things.
https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2091313002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7674: "async({ foo = 1 })", On 2016/06/24 18:30:14, Dan Ehrenberg wrote: > On 2016/06/24 at 18:05:40, caitp wrote: > > On 2016/06/24 17:30:41, Dan Ehrenberg wrote: > > > Maybe verify that await is banned in this context > > > > why would await be banned in this context, other than in a module? > > > > `async({ await = 1 }) => 1` would be an error, but is currently only reported > as an error in the full parser, not the preparser, so I think that's a separate > issue (similar to other cases where arrow functions don't produce early errors > in the preparser). > > test-parsing.cc has a way of invoking just the parser. We should take advantage > of that mode to test these things. I've added tests and a TODO, will deal with fixing the banned "await" parameter separately.
Thanks for the additional tests, but they seem to show that the patch is not ready yet. https://codereview.chromium.org/2091313002/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2091313002/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:7674: "async({ foo = 1 })", I don't think it's valid for this to be a syntax error. Functions called 'async' must be supported, especially since this feature is staged.
On 2016/06/24 21:31:00, Dan Ehrenberg wrote: > Thanks for the additional tests, but they seem to show that the patch is not > ready yet. > > https://codereview.chromium.org/2091313002/diff/40001/test/cctest/test-parsin... > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/2091313002/diff/40001/test/cctest/test-parsin... > test/cctest/test-parsing.cc:7674: "async({ foo = 1 })", > I don't think it's valid for this to be a syntax error. Functions called 'async' > must be supported, especially since this feature is staged. The additional tests are totally unrelated to this patch, this is fixing cover initialized names, nothing relating to a banned identifier
lgtm
Sorry for my confusion, of course ({foo = 1}) is a SyntaxError. This patch seems
good to me.
Any idea why we bother calling RewriteNonPattern on each iteration of the loop,
rather than just always at the end?
On 2016/06/24 22:31:30, Dan Ehrenberg wrote:
> lgtm
>
> Sorry for my confusion, of course ({foo = 1}) is a SyntaxError. This patch
seems
> good to me.
>
> Any idea why we bother calling RewriteNonPattern on each iteration of the
loop,
> rather than just always at the end?
I think it emits a more correct error, but it may not matter that much. Should
ask Nickie if theres another reason
On 2016/06/24 22:31:30, Dan Ehrenberg wrote:
> lgtm
>
> Sorry for my confusion, of course ({foo = 1}) is a SyntaxError. This patch
seems
> good to me.
>
> Any idea why we bother calling RewriteNonPattern on each iteration of the
loop,
> rather than just always at the end?
I think it emits a more correct error, but it may not matter that much. Should
ask Nickie if theres another reason
The CQ bit was checked by caitpotter88@gmail.com
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_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2091313002/#ps60001 (title: "Fix compile error")
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_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) 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 caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2091313002/#ps80001 (title: "_Really_ fix compile error")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [parser] don't report error for CoverInitializedNames in async arrow formals BUG=v8:4483, v8:5148 R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com, nikolaos@chromium.org ========== to ========== [parser] don't report error for CoverInitializedNames in async arrow formals BUG=v8:4483, v8:5148 R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com, nikolaos@chromium.org Committed: https://crrev.com/4bb1f70e66ef361f42fdb6082cd4db9def77befc Cr-Commit-Position: refs/heads/master@{#37260} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4bb1f70e66ef361f42fdb6082cd4db9def77befc Cr-Commit-Position: refs/heads/master@{#37260} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
