Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695973003:
ParserBase should accept ESCAPED_STRICT_RESERVED_WORD as an identifier (Closed)
|
Created:
3 years, 10 months ago by vabr (Chromium) Modified:
3 years, 10 months ago Reviewers:
adamk CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionParserBase should accept ESCAPED_STRICT_RESERVED_WORD as an identifier
ParserBase::is_any_identifier currently does not recognise
Token::ESCAPED_STRICT_RESERVED_WORD as an identifier. This seems different
from what ParserBase::ParseIdentifierName does, and also prevents
"l\u0065t", unlike "let", from becoming a label.
This CL extends is_any_identifier to also accept ESCAPED_STRICT_RESERVED_WORD.
BUG=v8:5692
Review-Url: https://codereview.chromium.org/2695973003
Cr-Commit-Position: refs/heads/master@{#43204}
Committed: https://chromium.googlesource.com/v8/v8/+/e3d761d94b2d899e4c13e57e5f1a75364082cd53
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add cctest, improve mjsunit #
Total comments: 2
Patch Set 3 : cctest fixed and ParseIdentifierOrStrictReservedWord recognises ESCAPED_STRICT_RESERVED_WORD #
Total comments: 4
Patch Set 4 : Address comments and add NULL to statement_data #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by vabr@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vabr@chromium.org changed reviewers: + adamk@chromium.org
Hi Adam, Could you please review? Thanks! Vaclav
The change looks right to me. I'd like to see some more tests in test-parsing.cc
around this, though, as I see several places where peek_any_identifier() is
currently used:
Function declaration: "function l\u0065t() { }"
Function expression: "(function l\u0065t() { })"
(also async versions of the above two)
Async arrow function: "async l\u0065t => 42"
All of these currently throw a SyntaxError, but I suspect they should all be
allowed (and will be with this CL).
https://codereview.chromium.org/2695973003/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-v8-5692.js (right):
https://codereview.chromium.org/2695973003/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-v8-5692.js:8: if (true) l\u0065t: ;
Can you make this test functional, by actually breaking to this label?
The CQ bit was checked by vabr@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...
Thanks, Adam! I did my best to address your comments, but apparently I need some advice (see below). Thanks, Vaclav https://codereview.chromium.org/2695973003/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-v8-5692.js (right): https://codereview.chromium.org/2695973003/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-v8-5692.js:8: if (true) l\u0065t: ; On 2017/02/14 22:00:53, adamk wrote: > Can you make this test functional, by actually breaking to this label? Done. https://codereview.chromium.org/2695973003/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2695973003/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:9465: v8_compile(script); I'm not at all sure that I'm doing the right thing here. If I place some obvious failure to |scripts|, e.g., "var var -", the test fails by reaching the CHECK below and printing the message. However, it passes for all |scripts| above, despite d8 showing errors: d8> function l\u0065t() { } (d8):1: SyntaxError: Keyword must not contain escaped characters function l\u0065t() { } ^^^^^^^^ SyntaxError: Keyword must not contain escaped characters So while my fix is obviously not complete yet, I'm not even sure how to test it properly. Any advice?
https://codereview.chromium.org/2695973003/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2695973003/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:1717: TEST(NoErrorsEvalAndArgumentsSloppy) { This is an example of the kind of test I had in mind. It tests parsing behavior only, while testing both the Parser and the PreParser to make sure they have the same behavior. As for the fact that "function l\u0065t() { }" still fails with your patch, that may be ok (it'd be good to know why, though). Hopefully you'll at least be able to see it fail easily by using this style of test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vabr@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...
Thanks, Adam! I changed the cctest according to your suggestion. It now agrees with manual testing in d8. Looking at the spec grammar [1], function identifiers are identifiers and can contain unicode escape sequences as well as "let" (the latter in non-strict mode). The same goes for the binding identifier in the arrow function. I also found the reason why I saw the error mentioned last time: ParseIdentifierOrStrictReservedWord needs a similar decision to what is_any_identifier does, but split into two branches. So I added ESCAPED_STRICT_RESERVED_WORD there as well. Now the test passes and all seems to be in accordance with the spec. Please review patch set 3. Thanks! Vaclav [1] https://tc39.github.io/ecma262/#sec-function-definitions
Please leave the "v8" out of the test filename, just "regress-5692.js" is the naming convention for v8 bugs. This lgtm once this and the comments below are addressed. Thanks for the fixes! https://codereview.chromium.org/2695973003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2695973003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:9461: "(async function l\u0065t() { })", "async l\u0065t => 42"}; For completeness, can you add a non-async arrow function here? I think it already worked, but it makes sense to include it in this test case. https://codereview.chromium.org/2695973003/diff/40001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-v8-5692.js (right): https://codereview.chromium.org/2695973003/diff/40001/test/mjsunit/regress/re... test/mjsunit/regress/regress-v8-5692.js:16: assertEquals(false, wasTouched); assertFalse will work here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
Thanks for the review and helpful advice! I addressed your comments, fixed a typo I made in the cctest (terminating NULL for statement_data) and am sending this to the CQ now. Cheers, Vaclav https://codereview.chromium.org/2695973003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2695973003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:9461: "(async function l\u0065t() { })", "async l\u0065t => 42"}; On 2017/02/15 01:25:39, adamk wrote: > For completeness, can you add a non-async arrow function here? I think it > already worked, but it makes sense to include it in this test case. Done. https://codereview.chromium.org/2695973003/diff/40001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-v8-5692.js (right): https://codereview.chromium.org/2695973003/diff/40001/test/mjsunit/regress/re... test/mjsunit/regress/regress-v8-5692.js:16: assertEquals(false, wasTouched); On 2017/02/15 01:25:39, adamk wrote: > assertFalse will work here. Done.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2695973003/#ps60001 (title: "Address comments and add NULL to statement_data")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487122465889450,
"parent_rev": "8a54a471758d0c732ac30b9d4026717b7c60020c", "commit_rev":
"e3d761d94b2d899e4c13e57e5f1a75364082cd53"}
Message was sent while issue was closed.
Description was changed from ========== ParserBase should accept ESCAPED_STRICT_RESERVED_WORD as an identifier ParserBase::is_any_identifier currently does not recognise Token::ESCAPED_STRICT_RESERVED_WORD as an identifier. This seems different from what ParserBase::ParseIdentifierName does, and also prevents "l\u0065t", unlike "let", from becoming a label. This CL extends is_any_identifier to also accept ESCAPED_STRICT_RESERVED_WORD. BUG=v8:5692 ========== to ========== ParserBase should accept ESCAPED_STRICT_RESERVED_WORD as an identifier ParserBase::is_any_identifier currently does not recognise Token::ESCAPED_STRICT_RESERVED_WORD as an identifier. This seems different from what ParserBase::ParseIdentifierName does, and also prevents "l\u0065t", unlike "let", from becoming a label. This CL extends is_any_identifier to also accept ESCAPED_STRICT_RESERVED_WORD. BUG=v8:5692 Review-Url: https://codereview.chromium.org/2695973003 Cr-Commit-Position: refs/heads/master@{#43204} Committed: https://chromium.googlesource.com/v8/v8/+/e3d761d94b2d899e4c13e57e5f1a7536408... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/e3d761d94b2d899e4c13e57e5f1a7536408... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
