Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2185223002:
Fix not throwing error when redefine eval or arguments in strict mode. (Closed)
DescriptionFix not throwing error when redefine eval or arguments in strict mode.
Currently when redefining eval or arguments in non-simple parameter list and
destructuring binding, V8 doesn't throw any error, this patch fixes it.
BUG=v8:5201
LOG=N
Committed: https://crrev.com/0c95efb7b79e6baec1dec2a75c5c3187b3a6df46
Cr-Commit-Position: refs/heads/master@{#38762}
Patch Set 1 #Patch Set 2 : add test #
Total comments: 6
Patch Set 3 : update #
Total comments: 5
Patch Set 4 : update #
Total comments: 1
Patch Set 5 : update&rebase #
Total comments: 6
Patch Set 6 : Clean up tests and rebase #
Total comments: 6
Patch Set 7 : Clean up tests #Messages
Total messages: 56 (38 generated)
The CQ bit was checked by lpy@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.
The CQ bit was checked by lpy@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.
lpy@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
PTAL.
https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1973: if (IsValidReferenceExpression(lhs) && peek() == Token::ASSIGN) { Does the insertion of IsValidReferenceExpression(lhs) here have any other effects observable in tests?
On 2016/07/28 00:13:38, Dan Ehrenberg wrote: > https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base.h > File src/parsing/parser-base.h (right): > > https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base... > src/parsing/parser-base.h:1973: if (IsValidReferenceExpression(lhs) && peek() == > Token::ASSIGN) { > Does the insertion of IsValidReferenceExpression(lhs) here have any other > effects observable in tests? I didn't see any other effects in tests.
So it looks like this is currently broken not just for parameter destructuring,
but for destructuring binding as well, e.g.:
var {eval} = {}
We somehow get the _right_ answer for destructuring assignment, i.e.:
({eval} = {})
There are tests for the latter in cctest/test-parsing.cc:
https://cs.chromium.org/chromium/src/v8/test/cctest/test-parsing.cc?rcl=0&l=6652
I think that file is the right place for tests for this feature, too, as it's
easy to write exhaustive tests there (and to test both the preparser and parser
paths).
https://codereview.chromium.org/2185223002/diff/20001/test/mjsunit/strict-mod...
File test/mjsunit/strict-mode-eval.js (right):
https://codereview.chromium.org/2185223002/diff/20001/test/mjsunit/strict-mod...
test/mjsunit/strict-mode-eval.js:34: var code5 = "(() => { function a({ eval =
false } = {}) {} })()";
Is the outer arrow function an important part of this test case?
https://codereview.chromium.org/2185223002/diff/20001/test/mjsunit/strict-mod...
test/mjsunit/strict-mode-eval.js:43: eval_alias(code4);
Can you add code5 here too?
https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1992: ReportMessageAt(Scanner::Location(next_beg_pos, next_end_pos), I think you should be able to detect this when parsing the identifier. See ParseAndClassifyIdentifier (and in particular its callers). ExpressionClassifier is the right way to handle this error; it's the thing we use to record "speculative" errors, before we know exactly what we're trying to parse. Anyway, in pointing to the destructuring assignment tests, I also meant to suggest looking at how the destructuring assignment case manages to get the right error (I'd use gdb or printf debugging for that :)
The CQ bit was checked by lpy@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.
Thanks for the feedback, I made a few updates. PTAL. https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/20001/src/parsing/parser-base... src/parsing/parser-base.h:1992: ReportMessageAt(Scanner::Location(next_beg_pos, next_end_pos), On 2016/07/28 18:46:55, adamk wrote: > I think you should be able to detect this when parsing the identifier. See > ParseAndClassifyIdentifier (and in particular its callers). > > ExpressionClassifier is the right way to handle this error; it's the thing we > use to record "speculative" errors, before we know exactly what we're trying to > parse. > > Anyway, in pointing to the destructuring assignment tests, I also meant to > suggest looking at how the destructuring assignment case manages to get the > right error (I'd use gdb or printf debugging for that :) Done. https://codereview.chromium.org/2185223002/diff/20001/test/mjsunit/strict-mod... File test/mjsunit/strict-mode-eval.js (right): https://codereview.chromium.org/2185223002/diff/20001/test/mjsunit/strict-mod... test/mjsunit/strict-mode-eval.js:43: eval_alias(code4); On 2016/07/28 18:33:44, adamk wrote: > Can you add code5 here too? Done.
Looking better. I'd really like to see tests for this in test-parsing.cc. https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:1910: if (this->IsAwait(*name)) { I actually don't know why we handle this here instead of in the caller...see below. https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:1912: } else if (this->IsEval(*name)) { What about "arguments"? Is there a reason check has to be done here, and not in the caller? It seems like we handle most of the other similar errors in ParsePropertyDefinition, in the block that handles the PropertyDefinition production. https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:1913: classifier->RecordStrictModeFormalParameterError( Not clear on why this is an error in sloppy mode. Can you add the test-parsing.cc tests we discussed? That'd let us more easily verify that we get the right behavior in strict vs sloppy.
The CQ bit was checked by lpy@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.
PTAL. The positive test for `arguments` is blocked by https://bugs.chromium.org/p/v8/issues/detail?id=4577 https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:1910: if (this->IsAwait(*name)) { On 2016/08/05 22:29:29, adamk wrote: > I actually don't know why we handle this here instead of in the caller...see > below. Done. https://codereview.chromium.org/2185223002/diff/40001/src/parsing/parser-base... src/parsing/parser-base.h:1912: } else if (this->IsEval(*name)) { My understanding is when parsing the property name, when a specific word encountered, it will record (for example, set is_await). But since `name` is an out parameter, I think we can do it in caller. Not sure if we should do this in a separate CL for await, I move the error report for eval and arguments to caller as suggested.
See below, I think this needs more test coverage because I think it doesn't
work.
As for the positive "arguments" case, you should be able to get this working
with a var declaration:
var {arguments};
since that won't conflict (since it won't try to create a lexical binding).
https://codereview.chromium.org/2185223002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/2185223002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6563: "eval",
I think this test case belongs with the other destructuring bind tests in
DestructuringPositiveTests
You'll want to add a section there for strict/sloppy, just like there is in this
one. But there's no need for a whole separate call for RunParserSyncTest.
Once you're there, you'll want to add a bunch more test cases. In particular, I
think this actually will fail, though it shouldn't:
{eval: x}
which means "bind the property named 'eval' to the variable 'x'". So more work
is needed in the code too.
The CQ bit was checked by lpy@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.
Description was changed from
==========
Fix not throwing when redefine eval in strict mode.
Redefining eval in strict mode using destructuring assignment doesn't throw,
this patch fixes it.
BUG=v8:5201
LOG=N
==========
to
==========
Fix not throwing error when redefine eval or arguments in strict mode.
Currently when redefining eval or arguments in non-simple parameter list and
destructuring binding, V8 doesn't throw any error, this patch fixes it.
BUG=v8:5201
LOG=N
==========
Thank you for the feedback, I updated the CL. PTAL. On 2016/08/09 17:00:51, adamk wrote: > You'll want to add a section there for strict/sloppy, just like there is in this > one. But there's no need for a whole separate call for RunParserSyncTest. What do you mean by no need for a whole separate call for RunParserSyncTest?
https://codereview.chromium.org/2185223002/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1998: classifier->RecordStrictModeFormalParameterError( Can you explain why this is needed? In particular, which tests fail without it? I don't see any added sloppy-mode error cases. https://codereview.chromium.org/2185223002/diff/80001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2185223002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6492: "{arguments}", Can you flesh out this test coverage while you're here? e.g., we test for "[eval]" but not "[arguments]". Similarly I'd like to see "{e : eval}", and similarly versions with defaults ("{eval = 0 }"). This test clearly thought it was covering these things previously and they were missed. https://codereview.chromium.org/2185223002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6897: // v8:5201 Isn't this block now redundant with the tests you added on line 6491?
The CQ bit was checked by lpy@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: Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by lpy@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...
Patchset #6 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I cleaned up the tests, ptal. https://codereview.chromium.org/2185223002/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2185223002/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1998: classifier->RecordStrictModeFormalParameterError( On 2016/08/17 19:32:51, adamk wrote: > Can you explain why this is needed? In particular, which tests fail without it? > I don't see any added sloppy-mode error cases. My mistakes. My understanding is when we enter PropertyDefinition, it means we are not using formal parameter error, instead we are using non-simple parameter list. So the RecordStrictModeFormalParameterError is not necessary here. https://codereview.chromium.org/2185223002/diff/80001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2185223002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6492: "{arguments}", On 2016/08/17 19:32:51, adamk wrote: > Can you flesh out this test coverage while you're here? e.g., we test for > "[eval]" but not "[arguments]". Similarly I'd like to see "{e : eval}", and > similarly versions with defaults ("{eval = 0 }"). This test clearly thought it > was covering these things previously and they were missed. Done. https://codereview.chromium.org/2185223002/diff/80001/test/cctest/test-parsin... test/cctest/test-parsing.cc:6897: // v8:5201 On 2016/08/17 19:32:51, adamk wrote: > Isn't this block now redundant with the tests you added on line 6491? Done.
Almost there, just a few more test changes please. https://codereview.chromium.org/2185223002/diff/120001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2185223002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6362: "function f(argument1, {eval} = {}) {}", Rather than enumerating different surrounding stuff here, I'd prefer if the context_data contained the outer bits and the tests themselves were just patterns, like the rest of the destructuring tests. For context, var declarations and functions are a good start, but I think you also want an arrow function example (I'd just copy the context_data for the positive tests and remove the "strict" ones). https://codereview.chromium.org/2185223002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6374: "var {eval: x} = {eval: 1};", These two tests can just go in the main block of test cases ("data"), as "{arguments: x}" and "{eval: x}" https://codereview.chromium.org/2185223002/diff/120001/test/mjsunit/strict-mo... File test/mjsunit/strict-mode-eval.js (right): https://codereview.chromium.org/2185223002/diff/120001/test/mjsunit/strict-mo... test/mjsunit/strict-mode-eval.js:34: var code5 = "(() => { function a({ eval = false } = {}) {} })()"; At this point I'd recommend just leaving out this test file, we're getting much more coverage from test-parser.cc.
The CQ bit was checked by lpy@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, I updated the CL, ptal. https://codereview.chromium.org/2185223002/diff/120001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2185223002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6362: "function f(argument1, {eval} = {}) {}", On 2016/08/19 18:14:56, adamk wrote: > Rather than enumerating different surrounding stuff here, I'd prefer if the > context_data contained the outer bits and the tests themselves were just > patterns, like the rest of the destructuring tests. > > For context, var declarations and functions are a good start, but I think you > also want an arrow function example (I'd just copy the context_data for the > positive tests and remove the "strict" ones). Done. https://codereview.chromium.org/2185223002/diff/120001/test/cctest/test-parsi... test/cctest/test-parsing.cc:6374: "var {eval: x} = {eval: 1};", On 2016/08/19 18:14:56, adamk wrote: > These two tests can just go in the main block of test cases ("data"), as > "{arguments: x}" and "{eval: x}" Done. https://codereview.chromium.org/2185223002/diff/120001/test/mjsunit/strict-mo... File test/mjsunit/strict-mode-eval.js (right): https://codereview.chromium.org/2185223002/diff/120001/test/mjsunit/strict-mo... test/mjsunit/strict-mode-eval.js:34: var code5 = "(() => { function a({ eval = false } = {}) {} })()"; On 2016/08/19 18:14:56, adamk wrote: > At this point I'd recommend just leaving out this test file, we're getting much > more coverage from test-parser.cc. Done.
lgtm
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 lpy@chromium.org
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
==========
Fix not throwing error when redefine eval or arguments in strict mode.
Currently when redefining eval or arguments in non-simple parameter list and
destructuring binding, V8 doesn't throw any error, this patch fixes it.
BUG=v8:5201
LOG=N
==========
to
==========
Fix not throwing error when redefine eval or arguments in strict mode.
Currently when redefining eval or arguments in non-simple parameter list and
destructuring binding, V8 doesn't throw any error, this patch fixes it.
BUG=v8:5201
LOG=N
==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Fix not throwing error when redefine eval or arguments in strict mode.
Currently when redefining eval or arguments in non-simple parameter list and
destructuring binding, V8 doesn't throw any error, this patch fixes it.
BUG=v8:5201
LOG=N
==========
to
==========
Fix not throwing error when redefine eval or arguments in strict mode.
Currently when redefining eval or arguments in non-simple parameter list and
destructuring binding, V8 doesn't throw any error, this patch fixes it.
BUG=v8:5201
LOG=N
Committed: https://crrev.com/0c95efb7b79e6baec1dec2a75c5c3187b3a6df46
Cr-Commit-Position: refs/heads/master@{#38762}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0c95efb7b79e6baec1dec2a75c5c3187b3a6df46 Cr-Commit-Position: refs/heads/master@{#38762} |
