[parser] Disallow Expression in for..of statements

test/cctest/test-parsing.cc

Index: test/cctest/test-parsing.cc
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index 8573ce3391fa581fe60cfb5cf5e061027d93b3f5..20c82665354c3921a9b96119e20a3ed295c8111f 100644
--- a/test/cctest/test-parsing.cc
+++ b/test/cctest/test-parsing.cc
@@ -4909,6 +4909,18 @@ TEST(ConstParsingInForIn) {
 }
 
 
+TEST(StatementParsingInForIn) {
+  const char* context_data[][2] = {{"'use strict';", ""},

[adamk, 2016/01/20 00:35:57]

Can you add non-strict cases too? Seems like the mode shouldn't matter.

[mike3, 2016/01/20 17:55:40]

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!

[adamk, 2016/01/20 19:31:17]

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.

[mike3, 2016/01/20 19:54:08]

Thanks for the pointer! (It's been a while since I wrote C++; I really missed
the memory management puns.)

+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {"for(x in {}, {}) {}", "for(var x in {}, {}) {}",
+                        "for(let x in {}, {}) {}", "for(const x in {}, {}) {}",
+                        NULL};
+  RunParserSyncTest(context_data, data, kSuccess, nullptr, 0, nullptr, 0);

[adamk, 2016/01/20 00:35:57]

You can leave off the last 4 arguments here (they default to null and 0). Same
goes for all the calls below

[mike3, 2016/01/20 17:55:40]

Acknowledged.

+}
+
+
 TEST(ConstParsingInForInError) {
   const char* context_data[][2] = {{"'use strict';", ""},
                                    {"function foo(){ 'use strict';", "}"},
@@ -5082,6 +5094,57 @@ TEST(ForOfNoDeclarationsError) {
 }
 
 
+TEST(ForOfInOperator) {
+  const char* context_data[][2] = {{"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for(x of 'foo' in {}) {}", "for(var x of 'foo' in {}) {}",
+      "for(let x of 'foo' in {}) {}", "for(const x of 'foo' in {}) {}", NULL};
+  RunParserSyncTest(context_data, data, kSuccess, nullptr, 0, nullptr, 0);
+}
+
+
+TEST(ForOfYieldIdentifier) {
+  const char* context_data[][2] = {{NULL, NULL}};

[adamk, 2016/01/20 00:35:57]

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.

[mike3, 2016/01/20 17:55:40]

Acknowledged.

+
+  const char* data[] = {"for(x of yield) {}", "for(var x of yield) {}",

[adamk, 2016/01/20 00:38:09]

Also curious why you added tests for yield, that seems unrelated to your patch.

[mike3, 2016/01/20 17:55:40]

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.

[adamk, 2016/01/20 19:31:17]

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).

+                        "for(let x of yield) {}", "for(const x of yield) {}",
+                        NULL};
+  RunParserSyncTest(context_data, data, kSuccess, nullptr, 0, nullptr, 0);
+}
+
+
+TEST(ForOfYieldExpressionError) {

[adamk, 2016/01/20 00:35:57]

This name seems wrong.

[mike3, 2016/01/20 17:55:40]

Acknowledged.

+  const char* context_data[][2] = {{"'use strict';", ""},

[adamk, 2016/01/20 00:35:57]

Again, non-strict versions would be good too, same below.

+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {"(function* g() { for(x of [], []) {} }",
+                        "(function* g() { for(var x of [], []) {} }",
+                        "(function* g() { for(let x of [], []) {} }",
+                        "(function* g() { for(const x of [], []) {} }", NULL};
+  RunParserSyncTest(context_data, data, kError, nullptr, 0, nullptr, 0);
+}
+
+
+TEST(ForOfExpressionError) {
+  const char* context_data[][2] = {{"'use strict';", ""},
+                                   {"function foo(){ 'use strict';", "}"},
+                                   {NULL, NULL}};
+
+  const char* data[] = {
+      "for(x of [], []) {}", "for(var x of [], []) {}",
+      "for(let x of [], []) {}", "for(const x of [], []) {}",
+
+      // AssignmentExpression should be validated statically:
+      "for(x of { y = 23 }) {}", "for(var x of { y = 23 }) {}",
+      "for(let x of { y = 23 }) {}", "for(const x of { y = 23 }) {}", NULL};
+  RunParserSyncTest(context_data, data, kError, nullptr, 0, nullptr, 0);
+}
+
+
 TEST(InvalidUnicodeEscapes) {
   const char* context_data[][2] = {{"", ""},
                                    {"'use strict';", ""},