[parser] Enforce module-specific identifier restriction

test/cctest/test-parsing.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1531 matching lines @@
 void TestParserSyncWithFlags(i::Handle<i::String> source,
                              i::EnumSet<ParserFlag> flags,
                              ParserSyncTestResult result,
                              bool is_module = false) {
   i::Isolate* isolate = CcTest::i_isolate();
   i::Factory* factory = isolate->factory();
 
   uintptr_t stack_limit = isolate->stack_guard()->real_climit();
   int preparser_materialized_literals = -1;
   int parser_materialized_literals = -2;
-  bool test_preparser = !is_module;
 
   // Preparse the data.
   i::CompleteParserRecorder log;
-  if (test_preparser) {

[adamk, 2016/04/04 22:33:46]

Does this test still pass after you remove these ifs? My understanding was that
Caitlin added these in order to make it easier to test code that wanted to
exercise modules, so that the individual tests didn't have to duplicate the
logic here.

[mike3, 2016/04/13 20:06:16]

After applying this patch, all tests in test-parsing.cc pass, as far as I can
tell. I'm still pretty new around here, so it may be that I am not triggering
*all* test runs. Here is how I verify:

    $ ./tools/run-tests.py --arch-and-mode=x64.release cctest/test-parsing/*
    >>> running presubmit tests
    Running C++ lint check...
    No changes in files detected. Skipping cpplint check.
    Running copyright header, trailing whitespaces and two empty lines between
declarations check...
    Total violating files: 0
    >>> Running tests for x64.release
    No connection to distribution server; running tests locally.
    [00:05|% 100|+ 526|-   0]: Done
    $ echo $?
    0

As I understand it, the net effect of removing the conditional check for
`test_preparser` (which is the inverse of `is_module`) is that invocations of
RunModuleParserSyncTest (of which there are two in `master` today) will now
also test the PreParser. It seems like this change would just make the existing
test more strict without effecting code reusability.

I may be wrong, though. I'll ask Caitlin to take a look.

On 2016/04/04 22:33:46, adamk wrote:

+  {
     i::Scanner scanner(isolate->unicode_cache());
     i::GenericStringUtf16CharacterStream stream(source, 0, source->length());
     i::Zone zone;
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->heap()->HashSeed());
     i::PreParser preparser(&zone, &scanner, &ast_value_factory, &log,
                            stack_limit);
     SetParserFlags(&preparser, flags);
     scanner.Initialize(&stream);
-    i::PreParser::PreParseResult result = preparser.PreParseProgram(
-        &preparser_materialized_literals);
+    i::PreParser::PreParseResult result =
+        preparser.PreParseProgram(&preparser_materialized_literals, is_module);
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
   }
   bool preparse_error = log.HasError();
 
   // Parse the data
   i::FunctionLiteral* function;
   {
     i::Handle<i::Script> script = factory->NewScript(source);
     i::Zone zone;
     i::ParseInfo info(&zone, script);
@@ skipping 25 matching lines @@
       v8::base::OS::Print(
           "Parser failed on:\n"
           "\t%s\n"
           "with error:\n"
           "\t%s\n"
           "However, we expected no error.",
           source->ToCString().get(), message_string->ToCString().get());
       CHECK(false);
     }
 
-    if (test_preparser && !preparse_error) {
+    if (!preparse_error) {
       v8::base::OS::Print(
           "Parser failed on:\n"
           "\t%s\n"
           "with error:\n"
           "\t%s\n"
           "However, the preparser succeeded",
           source->ToCString().get(), message_string->ToCString().get());
       CHECK(false);
     }
     // Check that preparser and parser produce the same error.
-    if (test_preparser) {
+    {
       i::Handle<i::String> preparser_message =
           FormatMessage(log.ErrorMessageData());
       if (!i::String::Equals(message_string, preparser_message)) {
         v8::base::OS::Print(
             "Expected parser and preparser to produce the same error on:\n"
             "\t%s\n"
             "However, found the following error messages\n"
             "\tparser:    %s\n"
             "\tpreparser: %s\n",
             source->ToCString().get(), message_string->ToCString().get(),
             preparser_message->ToCString().get());
         CHECK(false);
       }
     }
-  } else if (test_preparser && preparse_error) {
+  } else if (preparse_error) {
     v8::base::OS::Print(
         "Preparser failed on:\n"
         "\t%s\n"
         "with error:\n"
         "\t%s\n"
         "However, the parser succeeded",
         source->ToCString().get(),
         FormatMessage(log.ErrorMessageData())->ToCString().get());
     CHECK(false);
   } else if (result == kError) {
     v8::base::OS::Print(
         "Expected error on:\n"
         "\t%s\n"
         "However, parser and preparser succeeded",
         source->ToCString().get());
     CHECK(false);
-  } else if (test_preparser &&
-             preparser_materialized_literals != parser_materialized_literals) {
+  } else if (preparser_materialized_literals != parser_materialized_literals) {
     v8::base::OS::Print(
         "Preparser materialized literals (%d) differ from Parser materialized "
         "literals (%d) on:\n"
         "\t%s\n"
         "However, parser and preparser succeeded",
         preparser_materialized_literals, parser_materialized_literals,
         source->ToCString().get());
     CHECK(false);
   }
 }
@@ skipping 3918 matching lines @@
       "var x; export default x = 7",
       "export { Q } from 'somemodule.js';",
       "export * from 'somemodule.js';",
       "var foo; export { foo as for };",
       "export { arguments } from 'm.js';",
       "export { for } from 'm.js';",
       "export { yield } from 'm.js'",
       "export { static } from 'm.js'",
       "export { let } from 'm.js'",
       "var a; export { a as b, a as c };",
+      "var a; export { a as await };",
+      "var a; export { a as enum };",
 
       "import 'somemodule.js';",
       "import { } from 'm.js';",
       "import { a } from 'm.js';",
       "import { a, b as d, c, } from 'm.js';",
       "import * as thing from 'm.js';",
       "import thing from 'm.js';",
       "import thing, * as rest from 'm.js';",
       "import thing, { a, b, c } from 'm.js';",
       "import { arguments as a } from 'm.js';",
@@ skipping 105 matching lines @@
       "import a { b, c } from 'm.js';",
       "import arguments from 'm.js';",
       "import eval from 'm.js';",
       "import { arguments } from 'm.js';",
       "import { eval } from 'm.js';",
       "import { a as arguments } from 'm.js';",
       "import { for } from 'm.js';",
       "import { y as yield } from 'm.js'",
       "import { s as static } from 'm.js'",
       "import { l as let } from 'm.js'",
+      "import { a as await } from 'm.js';",
+      "import { a as enum } from 'm.js';",
       "import { x }, def from 'm.js';",
       "import def, def2 from 'm.js';",
       "import * as x, def from 'm.js';",
       "import * as x, * as y from 'm.js';",
       "import {x}, {y} from 'm.js';",
       "import * as x, {y} from 'm.js';",
   };
   // clang-format on
 
   i::Isolate* isolate = CcTest::i_isolate();
@@ skipping 12 matching lines @@
 
     i::Handle<i::Script> script = factory->NewScript(source);
     i::Zone zone;
     i::ParseInfo info(&zone, script);
     i::Parser parser(&info);
     info.set_module();
     CHECK(!parser.Parse(&info));
   }
 }
 
+TEST(ModuleAwaitReserved) {
+  // clang-format off
+  const char* kErrorSources[] = {
+      "await;",
+      "await: ;",
+      "var await;",
+      "var [await] = [];",
+      "var { await } = {};",
+      "var { x: await } = {};",
+      "{ var await; }",
+      "let await;",
+      "let [await] = [];",
+      "let { await } = {};",
+      "let { x: await } = {};",
+      "{ let await; }",
+      "const await = null;",
+      "const [await] = [];",
+      "const { await } = {};",
+      "const { x: await } = {};",
+      "{ const await = null; }",
+      "function await() {}",
+      "function f(await) {}",
+      "function* await() {}",
+      "function* g(await) {}",
+      "(function await() {});",
+      "(function (await) {});",
+      "(function* await() {});",
+      "(function* (await) {});",
+      "(await) => {};",
+      "await => {};",
+      "class await {}",
+      "class C { constructor(await) {} }",
+      "class C { m(await) {} }",
+      "class C { static m(await) {} }",
+      "class C { *m(await) {} }",
+      "class C { static *m(await) {} }",
+      "(class await {})",
+      "(class { constructor(await) {} });",
+      "(class { m(await) {} });",
+      "(class { static m(await) {} });",
+      "(class { *m(await) {} });",
+      "(class { static *m(await) {} });",
+      "({ m(await) {} });",
+      "({ *m(await) {} });",
+      "({ set p(await) {} });",
+      "try {} catch (await) {}",
+      "try {} catch (await) {} finally {}"
+  };

[adamk, 2016/04/04 22:33:46]

Please add a "// clang-format on" after this line.

[mike3, 2016/04/13 20:06:16]

Sure thing! I omitted it in another test, so I'll correct that instance, too.

On 2016/04/04 22:33:46, adamk wrote:

+
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::Factory* factory = isolate->factory();
+
+  v8::HandleScope handles(CcTest::isolate());
+  v8::Local<v8::Context> context = v8::Context::New(CcTest::isolate());
+  v8::Context::Scope context_scope(context);
+
+  isolate->stack_guard()->SetStackLimit(i::GetCurrentStackPosition() -
+                                        128 * 1024);
+
+  for (unsigned i = 0; i < arraysize(kErrorSources); ++i) {
+    i::Handle<i::String> source =
+        factory->NewStringFromAsciiChecked(kErrorSources[i]);
+
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::Zone zone;
+    i::ParseInfo info(&zone, script);
+    i::Parser parser(&info);
+    info.set_module();
+    CHECK(!parser.Parse(&info));
+  }
+  for (unsigned i = 0; i < arraysize(kErrorSources); ++i) {
+    i::Handle<i::String> source =
+        factory->NewStringFromAsciiChecked(kErrorSources[i]);
+
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::Zone zone;
+    i::ParseInfo info(&zone, script);
+    i::Parser parser(&info);
+    CHECK(parser.Parse(&info));
+  }
+}
+
+
+TEST(ModuleAwaitReservedPreParse) {
+  const char* context_data[][2] = {
+    {"", ""},
+    {NULL, NULL}
+  };
+  const char* error_data[] = {
+    "function f() { var await = 0; }",
+    NULL
+  };
+
+  RunModuleParserSyncTest(context_data, error_data, kError, NULL, 0, NULL, 0);
+}
+
+
+TEST(ModuleAwaitPermitted) {
+  // clang-format off
+  const char* kErrorSources[] = {
+    "({}).await;",
+    "({ await: null });",
+    "({ await() {} });",
+    "({ get await() {} });",
+    "({ set await(x) {} });",
+    "(class { await() {} });",
+    "(class { static await() {} });",
+    "(class { *await() {} });",
+    "(class { static *await() {} });"
+  };
+  // clang-format on
+
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::Factory* factory = isolate->factory();
+
+  v8::HandleScope handles(CcTest::isolate());
+  v8::Local<v8::Context> context = v8::Context::New(CcTest::isolate());
+  v8::Context::Scope context_scope(context);
+
+  isolate->stack_guard()->SetStackLimit(i::GetCurrentStackPosition() -
+                                        128 * 1024);
+
+  for (unsigned i = 0; i < arraysize(kErrorSources); ++i) {
+    i::Handle<i::String> source =
+        factory->NewStringFromAsciiChecked(kErrorSources[i]);
+
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::Zone zone;
+    i::ParseInfo info(&zone, script);
+    i::Parser parser(&info);
+    info.set_module();
+    CHECK(parser.Parse(&info));
+  }
+}
+
+TEST(EnumReserved) {
+  // clang-format off
+  const char* kErrorSources[] = {
+      "enum;",
+      "enum: ;",
+      "var enum;",
+      "var [enum] = [];",
+      "var { enum } = {};",
+      "var { x: enum } = {};",
+      "{ var enum; }",
+      "let enum;",
+      "let [enum] = [];",
+      "let { enum } = {};",
+      "let { x: enum } = {};",
+      "{ let enum; }",
+      "const enum = null;",
+      "const [enum] = [];",
+      "const { enum } = {};",
+      "const { x: enum } = {};",
+      "{ const enum = null; }",
+      "function enum() {}",
+      "function f(enum) {}",
+      "function* enum() {}",
+      "function* g(enum) {}",
+      "(function enum() {});",
+      "(function (enum) {});",
+      "(function* enum() {});",
+      "(function* (enum) {});",
+      "(enum) => {};",
+      "enum => {};",
+      "class enum {}",
+      "class C { constructor(enum) {} }",
+      "class C { m(enum) {} }",
+      "class C { static m(enum) {} }",
+      "class C { *m(enum) {} }",
+      "class C { static *m(enum) {} }",
+      "(class enum {})",
+      "(class { constructor(enum) {} });",
+      "(class { m(enum) {} });",
+      "(class { static m(enum) {} });",
+      "(class { *m(enum) {} });",
+      "(class { static *m(enum) {} });",
+      "({ m(enum) {} });",
+      "({ *m(enum) {} });",
+      "({ set p(enum) {} });",
+      "try {} catch (enum) {}",
+      "try {} catch (enum) {} finally {}"
+  };
+
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::Factory* factory = isolate->factory();
+
+  v8::HandleScope handles(CcTest::isolate());
+  v8::Local<v8::Context> context = v8::Context::New(CcTest::isolate());
+  v8::Context::Scope context_scope(context);
+
+  isolate->stack_guard()->SetStackLimit(i::GetCurrentStackPosition() -
+                                        128 * 1024);
+
+  for (unsigned i = 0; i < arraysize(kErrorSources); ++i) {
+    i::Handle<i::String> source =
+        factory->NewStringFromAsciiChecked(kErrorSources[i]);
+
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::Zone zone;
+    i::ParseInfo info(&zone, script);
+    i::Parser parser(&info);
+    info.set_module();
+    CHECK(!parser.Parse(&info));
+  }
+  for (unsigned i = 0; i < arraysize(kErrorSources); ++i) {
+    i::Handle<i::String> source =
+        factory->NewStringFromAsciiChecked(kErrorSources[i]);
+
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::Zone zone;
+    i::ParseInfo info(&zone, script);
+    i::Parser parser(&info);
+    CHECK(!parser.Parse(&info));
+  }
+}
+
 
 TEST(ModuleParsingInternals) {
   i::Isolate* isolate = CcTest::i_isolate();
   i::Factory* factory = isolate->factory();
   v8::HandleScope handles(CcTest::isolate());
   v8::Local<v8::Context> context = v8::Context::New(CcTest::isolate());
   v8::Context::Scope context_scope(context);
   isolate->stack_guard()->SetStackLimit(i::GetCurrentStackPosition() -
                                         128 * 1024);
 
@@ skipping 1617 matching lines @@
     // "Array() **= 10",
     NULL
   };
   // clang-format on
 
   static const ParserFlag always_flags[] = {
       kAllowHarmonyExponentiationOperator};
   RunParserSyncTest(context_data, error_data, kError, NULL, 0, always_flags,
                     arraysize(always_flags));
 }