Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1602823003:
[parser] Disallow Expression in for..of statements (Closed)
Description[parser] Disallow Expression in for..of statements
Although the `for..in` statement allows Expressions to define the
iterator, only an AssignmentExpression may occupy this position in the
`for..of` statement.
BUG=v8:4692
LOG=N
R=adamk@chromium.org
Committed: https://crrev.com/f7263b6a3f35f677169c77cdd332794d1b3b63ca
Cr-Commit-Position: refs/heads/master@{#33420}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Modified to include expression validation and more extensive tests #Patch Set 3 : Correcting a typo #
Total comments: 18
Patch Set 4 : Incorporating latest review feedback #
Messages
Total messages: 15 (2 generated)
https://codereview.chromium.org/1602823003/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1602823003/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:3682: enumerable = ParseAssignmentExpression(true, &classifier, CHECK_OK); Why no ValidateExpression() call here? https://codereview.chromium.org/1602823003/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:3803: enumerable = ParseAssignmentExpression(true, &classifier, CHECK_OK); Same question down here? https://codereview.chromium.org/1602823003/diff/1/test/mjsunit/es6/iteration-... File test/mjsunit/es6/iteration-syntax.js (right): https://codereview.chromium.org/1602823003/diff/1/test/mjsunit/es6/iteration-... test/mjsunit/es6/iteration-syntax.js:69: StrictSyntaxError("function f() { for (x of y, z) { } }"); Rather than adding tests here (which limits you not just to the parser but also to strict mode), please add a new test case to test/cctest/test-parsing.cc Take a look at TEST(ConstParsingForInError) for a good starting point.
https://codereview.chromium.org/1602823003/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1602823003/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:3683: ParserTraits::RewriteNonPattern(enumerable, &classifier, CHECK_OK); I think the calling convention here is to assign it back to the original (as I said, this is brand new to me): enumerable = RewriteNonPattern(...) https://codereview.chromium.org/1602823003/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:3805: ParserTraits::RewriteNonPattern(enumerable, &classifier, CHECK_OK); Same note here. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4913: const char* context_data[][2] = {{"'use strict';", ""}, Can you add non-strict cases too? Seems like the mode shouldn't matter. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4920: RunParserSyncTest(context_data, data, kSuccess, nullptr, 0, nullptr, 0); You can leave off the last 4 arguments here (they default to null and 0). Same goes for all the calls below https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5110: const char* context_data[][2] = {{NULL, NULL}}; This won't actually run any tests, since you haven't provided any context_data. See the outer for loop in RunParserSyncTest for how this works. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5119: TEST(ForOfYieldExpressionError) { This name seems wrong. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5120: const char* context_data[][2] = {{"'use strict';", ""}, Again, non-strict versions would be good too, same below.
https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5112: const char* data[] = {"for(x of yield) {}", "for(var x of yield) {}", Also curious why you added tests for yield, that seems unrelated to your patch.
https://codereview.chromium.org/1602823003/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1602823003/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:3683: ParserTraits::RewriteNonPattern(enumerable, &classifier, CHECK_OK); On 2016/01/20 00:35:57, adamk wrote: > I think the calling convention here is to assign it back to the original (as I > said, this is brand new to me): > > enumerable = RewriteNonPattern(...) Acknowledged. https://codereview.chromium.org/1602823003/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:3805: ParserTraits::RewriteNonPattern(enumerable, &classifier, CHECK_OK); On 2016/01/20 00:35:57, adamk wrote: > Same note here. Acknowledged. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4913: const char* context_data[][2] = {{"'use strict';", ""}, On 2016/01/20 00:35:57, adamk wrote: > Can you add non-strict cases too? Seems like the mode shouldn't matter. I've omitted strict mode because I was following the pattern set by ConstParsingForInError. I didn't realize this until attempting to enable non-strict mode versions that V8 requires strict mode for block-scoped declarations. If you prefer, I could split these tests in two: one set that has `for(x of y)` and `for(var x of y)` (which V8 will parse outside of strict mode) and a separate test for `for(let x of y)` and `for(const x of y)`. Let me know! https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4920: RunParserSyncTest(context_data, data, kSuccess, nullptr, 0, nullptr, 0); On 2016/01/20 00:35:57, adamk wrote: > You can leave off the last 4 arguments here (they default to null and 0). Same > goes for all the calls below Acknowledged. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5110: const char* context_data[][2] = {{NULL, NULL}}; On 2016/01/20 00:35:57, adamk wrote: > This won't actually run any tests, since you haven't provided any context_data. > See the outer for loop in RunParserSyncTest for how this works. Acknowledged. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5112: const char* data[] = {"for(x of yield) {}", "for(var x of yield) {}", On 2016/01/20 00:38:09, adamk wrote: > Also curious why you added tests for yield, that seems unrelated to your patch. I was thinking it would be good to be thorough, since the expression I modified has a [?Yield] production parameter, and there are current no tests to assert that is observed. You're right that my change doesn't implement this behavior, though, so I'm happy to remove these tests if you'd prefer. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5119: TEST(ForOfYieldExpressionError) { On 2016/01/20 00:35:57, adamk wrote: > This name seems wrong. Acknowledged.
https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4913: const char* context_data[][2] = {{"'use strict';", ""}, On 2016/01/20 17:55:40, mike3 wrote: > On 2016/01/20 00:35:57, adamk wrote: > > Can you add non-strict cases too? Seems like the mode shouldn't matter. > > I've omitted strict mode because I was following the pattern set by > ConstParsingForInError. I didn't realize this until attempting to enable > non-strict mode versions that V8 requires strict mode for block-scoped > declarations. > > If you prefer, I could split these tests in two: one set that has `for(x of y)` > and `for(var x of y)` (which V8 will parse outside of strict mode) and a > separate test for `for(let x of y)` and `for(const x of y)`. Let me know! Outside strict mode, v8 will parse lexical declarations if the proper flags are set: kAllowHarmonySloppy, kAllowHarmonySloppyLet, and kNoLegacyConst. See the "LetSloppyOnly" test for an example that passes those flags to RunParserSyncTest. https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:5112: const char* data[] = {"for(x of yield) {}", "for(var x of yield) {}", On 2016/01/20 17:55:40, mike3 wrote: > On 2016/01/20 00:38:09, adamk wrote: > > Also curious why you added tests for yield, that seems unrelated to your > patch. > > I was thinking it would be good to be thorough, since the expression I modified > has a [?Yield] production parameter, and there are current no tests to assert > that is observed. You're right that my change doesn't implement this behavior, > though, so I'm happy to remove these tests if you'd prefer. It's fine to leave them in, thanks for the explanation of why they got added here (we can always use more coverage of yield parsing).
Can't we just emit a SyntaxError if the iterable is a BinaryExpression with Token::COMMA in the for-of case? Seems more concise in my mind
https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1602823003/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:4913: const char* context_data[][2] = {{"'use strict';", ""}, On 2016/01/20 19:31:17, adamk wrote: > On 2016/01/20 17:55:40, mike3 wrote: > > On 2016/01/20 00:35:57, adamk wrote: > > > Can you add non-strict cases too? Seems like the mode shouldn't matter. > > > > I've omitted strict mode because I was following the pattern set by > > ConstParsingForInError. I didn't realize this until attempting to enable > > non-strict mode versions that V8 requires strict mode for block-scoped > > declarations. > > > > If you prefer, I could split these tests in two: one set that has `for(x of > y)` > > and `for(var x of y)` (which V8 will parse outside of strict mode) and a > > separate test for `for(let x of y)` and `for(const x of y)`. Let me know! > > Outside strict mode, v8 will parse lexical declarations if the proper flags are > set: kAllowHarmonySloppy, kAllowHarmonySloppyLet, and kNoLegacyConst. See the > "LetSloppyOnly" test for an example that passes those flags to > RunParserSyncTest. Thanks for the pointer! (It's been a while since I wrote C++; I really missed the memory management puns.)
On 2016/01/20 19:42:19, caitp wrote: > Can't we just emit a SyntaxError if the iterable is a BinaryExpression with > Token::COMMA in the for-of case? Seems more concise in my mind That sounds straightforward, although I know next-to-nothing about V8's internals. My concern would be how V8's "BinaryExpression" differs from ES2015's Expression (in case it allowed some comma-less form that was not an AssignmentExpression). But I'm sure this question was directed at Adamk, anyway :P
On 2016/01/20 20:00:17, mike3 wrote: > On 2016/01/20 19:42:19, caitp wrote: > > Can't we just emit a SyntaxError if the iterable is a BinaryExpression with > > Token::COMMA in the for-of case? Seems more concise in my mind > > That sounds straightforward, although I know next-to-nothing about V8's > internals. My concern would be how V8's "BinaryExpression" differs from ES2015's > Expression (in case it allowed some comma-less form that was not an > AssignmentExpression). > > But I'm sure this question was directed at Adamk, anyway :P I feel like this approach is more readable, especially when comparing to the spec language. It doesn't need a comment, whereas a check for BinaryExpression/Token::COMMA would, even if that code would be shorter. lgtm, we're right at the beginning of the branch point so this is as good a time as any to see if this restrictive syntax will cause any problems in the wild.
The CQ bit was checked by mike@mikepennisi.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1602823003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1602823003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [parser] Disallow Expression in for..of statements Although the `for..in` statement allows Expressions to define the iterator, only an AssignmentExpression may occupy this position in the `for..of` statement. BUG=v8:4692 LOG=N R=adamk@chromium.org ========== to ========== [parser] Disallow Expression in for..of statements Although the `for..in` statement allows Expressions to define the iterator, only an AssignmentExpression may occupy this position in the `for..of` statement. BUG=v8:4692 LOG=N R=adamk@chromium.org Committed: https://crrev.com/f7263b6a3f35f677169c77cdd332794d1b3b63ca Cr-Commit-Position: refs/heads/master@{#33420} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f7263b6a3f35f677169c77cdd332794d1b3b63ca Cr-Commit-Position: refs/heads/master@{#33420} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
