[parser] Enforce module-specific identifier restriction

[parser] Enforce module-specific identifier restriction

Restrict the use of the `await` token as an identifier when parsing
source text as module code.

From
http://www.ecma-international.org/ecma-262/6.0/#sec-future-reserved-words:

> 11.6.2.2 Future Reserved Words
>
> The following tokens are reserved for used as keywords in future
> language extensions.
>
> Syntax
>
>     FutureReservedWord ::
>         enum
>         await
>
> await is only treated as a FutureReservedWord when Module is the goal
> symbol of the syntactic grammar.

BUG=v8:4767
LOG=N
R=adamk@chromium.org

Committed: https://crrev.com/efe5b72d021f4b099e445bf1118b71df065d6e8c
Cr-Commit-Position: refs/heads/master@{#35914}

[mike3, 2016-02-23 19:29:44]

https://codereview.chromium.org/1723313002/diff/1/src/parsing/preparser.cc
File src/parsing/preparser.cc (left):

https://codereview.chromium.org/1723313002/diff/1/src/parsing/preparser.cc#ol...
src/parsing/preparser.cc:697: DCHECK(!expr.AsIdentifier().IsFutureReserved());
I'm not convinced that removing this check outright (instead of updating it) is
valid, but I haven't been able to think of a test that proves otherwise.

https://codereview.chromium.org/1723313002/diff/1/src/parsing/token.h
File src/parsing/token.h (right):

https://codereview.chromium.org/1723313002/diff/1/src/parsing/token.h#newcode199
src/parsing/token.h:199: bool is_generator, bool is_module) {
This method is starting to succumb to the boolean trap. I think we could
refactor this method's signature to accept simply accept a token and a Scope
reference, e.g.:

    static bool IsIdentifier(Value tok, Scope* scope)

...but that may violate a separation of concerns principle.

[adamk, 2016-02-23 22:37:16]

https://codereview.chromium.org/1723313002/diff/1/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/1723313002/diff/1/src/ast/scopes.h#newcode356
src/ast/scopes.h:356: bool inside_module() const { return scope_inside_module_;
}
Scope isn't the right place to put this, since it's not used at all in any Scope
analysis logic.

Can't this just be on Parser? There's no way to parse a module inside a module,
so for any given parser invocation, the options are:

1. We're parsing a module
2. We're parsing some other top-level program
3. We're parsing a function lazily, in which case it must have parsed
successfully the first time.

https://codereview.chromium.org/1723313002/diff/1/src/parsing/preparser.cc
File src/parsing/preparser.cc (left):

https://codereview.chromium.org/1723313002/diff/1/src/parsing/preparser.cc#ol...
src/parsing/preparser.cc:697: DCHECK(!expr.AsIdentifier().IsFutureReserved());
On 2016/02/23 19:29:44, mike3 wrote:
> I'm not convinced that removing this check outright (instead of updating it)
is
> valid, but I haven't been able to think of a test that proves otherwise.

"DCHECK" is the V8 equivalent of "assert": it's not meant to be something that a
known test case could be written for, it's stating assumptions about invariants
at this part of the code. So I'd prefer it be updated rather than removed.

[mike3, 2016-02-24 16:20:54]

https://codereview.chromium.org/1723313002/diff/1/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/1723313002/diff/1/src/ast/scopes.h#newcode356
src/ast/scopes.h:356: bool inside_module() const { return scope_inside_module_;
}
On 2016/02/23 22:37:15, adamk wrote:
> Scope isn't the right place to put this, since it's not used at all in any
Scope
> analysis logic.
> 
> Can't this just be on Parser? There's no way to parse a module inside a
module,
> so for any given parser invocation, the options are:
> 
> 1. We're parsing a module
> 2. We're parsing some other top-level program
> 3. We're parsing a function lazily, in which case it must have parsed
> successfully the first time.
> 

Acknowledged.

https://codereview.chromium.org/1723313002/diff/1/src/parsing/preparser.cc
File src/parsing/preparser.cc (left):

https://codereview.chromium.org/1723313002/diff/1/src/parsing/preparser.cc#ol...
src/parsing/preparser.cc:697: DCHECK(!expr.AsIdentifier().IsFutureReserved());
On 2016/02/23 22:37:15, adamk wrote:
> On 2016/02/23 19:29:44, mike3 wrote:
> > I'm not convinced that removing this check outright (instead of updating it)
> is
> > valid, but I haven't been able to think of a test that proves otherwise.
> 
> "DCHECK" is the V8 equivalent of "assert": it's not meant to be something that
a
> known test case could be written for, it's stating assumptions about
invariants
> at this part of the code. So I'd prefer it be updated rather than removed.

Acknowledged.

[mike3, 2016-02-24 16:26:25]

Thanks for the review, Adam! I've extended ParserBase with a `parsing_module_`
flag. I expect this isn't quite right either, but I'm hoping it's closer to what
you had in mind.

https://codereview.chromium.org/1723313002/diff/20001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/1723313002/diff/20001/src/parsing/preparser.c...
src/parsing/preparser.cc:699: DCHECK(!expr.AsIdentifier().IsEnum());
This check is equivalent to the previously-existing assertion. I haven't
extended it to include considerations for `await` because I don't think the
PreParser is able to determine whether it is parsing a module or not. That
information is provided when the Parser's `parse` method is invoked.

[adamk, 2016-02-24 19:13:25]

https://codereview.chromium.org/1723313002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1723313002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:938: bool parsing_module_;
Please initialize this to false in the constructor.

https://codereview.chromium.org/1723313002/diff/20001/src/parsing/preparser.cc
File src/parsing/preparser.cc (right):

https://codereview.chromium.org/1723313002/diff/20001/src/parsing/preparser.c...
src/parsing/preparser.cc:699: DCHECK(!expr.AsIdentifier().IsEnum());
On 2016/02/24 16:26:25, mike3 wrote:
> This check is equivalent to the previously-existing assertion. I haven't
> extended it to include considerations for `await` because I don't think the
> PreParser is able to determine whether it is parsing a module or not. That
> information is provided when the Parser's `parse` method is invoked.

I think this means you need to add code to pass the parsing_module_ bit to
PreParseLazyFunction(), which is how the PreParser is invoked from
Parser::ParseLazyFunctionBodyWithPreParser.

I'll think about the right way to design a test case to cover that particular
behavior; it may not be possible at the moment. We never parse the top level of
a module with the PreParser.

[adamk, 2016-02-24 19:37:01]

On 2016/02/24 19:13:25, adamk wrote:
>
https://codereview.chromium.org/1723313002/diff/20001/src/parsing/parser-base.h
> File src/parsing/parser-base.h (right):
> 
>
https://codereview.chromium.org/1723313002/diff/20001/src/parsing/parser-base...
> src/parsing/parser-base.h:938: bool parsing_module_;
> Please initialize this to false in the constructor.
> 
> https://codereview.chromium.org/1723313002/diff/20001/src/parsing/preparser.cc
> File src/parsing/preparser.cc (right):
> 
>
https://codereview.chromium.org/1723313002/diff/20001/src/parsing/preparser.c...
> src/parsing/preparser.cc:699: DCHECK(!expr.AsIdentifier().IsEnum());
> On 2016/02/24 16:26:25, mike3 wrote:
> > This check is equivalent to the previously-existing assertion. I haven't
> > extended it to include considerations for `await` because I don't think the
> > PreParser is able to determine whether it is parsing a module or not. That
> > information is provided when the Parser's `parse` method is invoked.
> 
> I think this means you need to add code to pass the parsing_module_ bit to
> PreParseLazyFunction(), which is how the PreParser is invoked from
> Parser::ParseLazyFunctionBodyWithPreParser.
> 
> I'll think about the right way to design a test case to cover that particular
> behavior; it may not be possible at the moment. We never parse the top level
of
> a module with the PreParser.

I think you should be able to trigger this by setting

i::FLAG_min_preparse_length = 0

before calling Parser::Parser in test-parser.cc, and then having a module like:

function f() { var await = 0 }

and verifying that it doesn't parse.

[mike3, 2016-03-02 18:06:24]

I've updated the PreParser to be aware of whether the input is module code or
not. This allowed me to re-write the original `DCHECK` statement with two
equivalent statements.

I was having a difficult time testing the pre-parser and comparing results with
the parser until Cait pointed me to RunModuleParserSyncTest. It seems as though
if we are going to test pre-parsing for module code, this is the place to do
it. As implemented, the function skips pre-parser checks, so I've updated it to
include those as well. I'm certain that this is not sufficient:

- the boolean `is_module` flag tacked on to PreParser::PreParseProgram doesn't
  seem right to me
- that method unconditionally calls `ParseStatementList`, even though module
  code should be parsed at the top level with ModuleItemList as the goal
  symbol

Since I'm not sure how to address these issues, I thought I would submit my
work-in-progress for feedback. I may be going down a rabbit hole here, though.
Do you think it would make more sense to land the identifier restriction as-is
and follow up with a separate patch to address lazily-parsed functions in
module code?

Thanks!

[adamk, 2016-03-02 18:27:45]

Haven't looked at the new code yet, but see responses inline.

On 2016/03/02 18:06:24, mike3 wrote:
> I've updated the PreParser to be aware of whether the input is module code or
> not. This allowed me to re-write the original `DCHECK` statement with two
> equivalent statements.
> 
> I was having a difficult time testing the pre-parser and comparing results
with
> the parser until Cait pointed me to RunModuleParserSyncTest. It seems as
though
> if we are going to test pre-parsing for module code, this is the place to do
> it. As implemented, the function skips pre-parser checks, so I've updated it
to
> include those as well. I'm certain that this is not sufficient:
> 
> - the boolean `is_module` flag tacked on to PreParser::PreParseProgram doesn't
>   seem right to me
> - that method unconditionally calls `ParseStatementList`, even though module
>   code should be parsed at the top level with ModuleItemList as the goal
>   symbol

There seems to be continuing confusion here (for which I apologize, I realize
it's not at all clear from the code): PreParser::PreParseProgram is a code-path
that's used only by test-parsing.cc. Modules are "special" in that they're not
supported in PreParseProgram, basically to avoid duplicating code that'd only be
used for testing. In production, we only use the PreParser to skip over function
bodies, and since import and export declarations are supported only at the top
level it didn't seem worth adding PreParser support for modules at all. With
this additional identifier restriction, we have our first bit that's necessary
to be threaded through the PreParser for modules.

> Since I'm not sure how to address these issues, I thought I would submit my
> work-in-progress for feedback. I may be going down a rabbit hole here, though.
> Do you think it would make more sense to land the identifier restriction as-is
> and follow up with a separate patch to address lazily-parsed functions in
> module code?

I too am worried about the rabbithole-ness of this patch; it may be that your
initial instinct that modules were a little too early for this work was correct.
I'd be fine with adding a simple restriction along with a TODO (and an open bug)
noting that it's currently broken for preparsing (this is fine until modules are
close to shipping).

> Thanks!

[mike3, 2016-03-31 23:08:39]

I just spent some time catching up with this patch, and I *think* the current
testing approach is sufficient. Here's why:

This patch updates the Parser's invocation of `PreParser::PreParseLazyFunction`
to forward its `parsing_module_` flag, which the PreParser adopts. The
PreParser does not itself define any methods that reference this flag (except
for those DTRACE calls), instead relying on the methods inherited from
ParserBase to honor the value of its own `parsing_module_` flag.

In PreParser::PreParseProgram, we're setting the `parsing_module_` flag as
appropriate. This mimics the behavior of the Parser in production. That means
that verifying consistent behavior here boils down to: "was the flag set
correctly?". As written, the current tests do indeed prove this.

The only way I can think to demonstrate the efficacy of a passing test is to
explain the failure that results from intentionally breaking the source code.
So here goes:

Removing the line where the PreParser "adopts" the flag from the Parser:

     PreParser::PreParseResult PreParser::PreParseLazyFunction(
         LanguageMode language_mode, FunctionKind kind, bool
has_simple_parameters,
         bool parsing_module, ParserRecorder* log,
         Scanner::BookmarkScope* bookmark) {
    -  parsing_module_ = parsing_module;
       log_ = log;
       // Lazy functions always have trivial outer scopes (no with/catch
scopes).
       Scope* top_scope = NewScope(scope_, SCRIPT_SCOPE);
       PreParserFactory top_factory(NULL);

Results in the following failure in cctest:

    Preparser failed on:
      function f() { var await; }
    with error:
      Unexpected reserved word
    However, the parser succeeded

This report demonstrates the validity of using StatementList in the
test-environment-specific PreParser::PreParseProgram because the PreParser
correctly reported the error. The fact that the Parser does *not* trigger an
error shows that the Parser under test is definitely using the PreParser for
function literals. So by re-inserting that line and confirming that the tests
pass again, we can say with confidence that the test demonstrates:

1. correct behavior by the PreParser as invoked in `PreParser::PreParseProgram`
   (which we can both agree is an artificial use case; necessary but not
   sufficient)
2. correct usage of the PreParser by the Parser in production settings

Although (and as always) I may be mistaken. Whatever the case, I very much
appreciate your guidance through this process!

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

I like the look of the code now, but I'm not sure the tests work. See below.

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (left):

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:1556: if (test_preparser) {
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.

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5802: };
Please add a "// clang-format on" after this line.

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

Caitlin: could you let us know if I'm correctly modifying
TestParserSyncWithFlags? (My interpretation is in an in-line comment below.)

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (left):

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:1556: if (test_preparser) {
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:
> 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.

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:5802: };
Sure thing! I omitted it in another test, so I'll correct that instance, too.

On 2016/04/04 22:33:46, adamk wrote:
> Please add a "// clang-format on" after this line.

[caitp (gmail), 2016-04-13 20:35:37]

On 2016/04/13 20:06:16, mike3 wrote:
> Caitlin: could you let us know if I'm correctly modifying
> TestParserSyncWithFlags? (My interpretation is in an in-line comment below.)
> 
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> File test/cctest/test-parsing.cc (left):
> 
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> test/cctest/test-parsing.cc:1556: if (test_preparser) {
> 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:
> > 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.
> 
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> File test/cctest/test-parsing.cc (right):
> 
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> test/cctest/test-parsing.cc:5802: };
> Sure thing! I omitted it in another test, so I'll correct that instance, too.
> 
> On 2016/04/04 22:33:46, adamk wrote:
> > Please add a "// clang-format on" after this line.

If those tests pass with this change, then great

However, the new tests you're adding are written in a way that does not check
consistency between the parser and preparser at all --- so you should change
them to use the module helpers

[mike3, 2016-04-13 20:50:17]

On 2016/04/13 20:35:37, caitp wrote:
> On 2016/04/13 20:06:16, mike3 wrote:
> > Caitlin: could you let us know if I'm correctly modifying
> > TestParserSyncWithFlags? (My interpretation is in an in-line comment below.)
> > 
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > File test/cctest/test-parsing.cc (left):
> > 
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > test/cctest/test-parsing.cc:1556: if (test_preparser) {
> > 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:
> > > 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.
> > 
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > File test/cctest/test-parsing.cc (right):
> > 
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > test/cctest/test-parsing.cc:5802: };
> > Sure thing! I omitted it in another test, so I'll correct that instance,
too.
> > 
> > On 2016/04/04 22:33:46, adamk wrote:
> > > Please add a "// clang-format on" after this line.
> 
> If those tests pass with this change, then great
> 
> However, the new tests you're adding are written in a way that does not check
> consistency between the parser and preparser at all --- so you should change
> them to use the module helpers

I included a single "negative" test that uses `await` within the body of a
function
literal, and used RunModuleParserSyncTest for that test.

Adam tells me that the top level of a module is never visited by the PreParser,
so I
thought this would be sufficient. I think that in order to meaningfully test the
PreParser, here, I would need to re-write the tests to *all* occur within a
function
literal. Is that what you had in mind?

[caitp (gmail), 2016-04-13 20:54:42]

On 2016/04/13 20:50:17, mike3 wrote:
> On 2016/04/13 20:35:37, caitp wrote:
> > On 2016/04/13 20:06:16, mike3 wrote:
> > > Caitlin: could you let us know if I'm correctly modifying
> > > TestParserSyncWithFlags? (My interpretation is in an in-line comment
below.)
> > > 
> > >
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > > File test/cctest/test-parsing.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > > test/cctest/test-parsing.cc:1556: if (test_preparser) {
> > > 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:
> > > > 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.
> > > 
> > >
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > > File test/cctest/test-parsing.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1723313002/diff/60001/test/cctest/test-parsin...
> > > test/cctest/test-parsing.cc:5802: };
> > > Sure thing! I omitted it in another test, so I'll correct that instance,
> too.
> > > 
> > > On 2016/04/04 22:33:46, adamk wrote:
> > > > Please add a "// clang-format on" after this line.
> > 
> > If those tests pass with this change, then great
> > 
> > However, the new tests you're adding are written in a way that does not
check
> > consistency between the parser and preparser at all --- so you should change
> > them to use the module helpers
> 
> I included a single "negative" test that uses `await` within the body of a
> function
> literal, and used RunModuleParserSyncTest for that test.
> 
> Adam tells me that the top level of a module is never visited by the
PreParser,
> so I
> thought this would be sufficient. I think that in order to meaningfully test
the
> PreParser, here, I would need to re-write the tests to *all* occur within a
> function
> literal. Is that what you had in mind?

use the helper functions, theyre there for a reason

[mike3, 2016-04-14 16:40:19]

This latest patch incorporates Caitlin's suggestion to consistently use the
RunModuleParserSyncTest for all the new tests. Besides making the test cases
far more readable, this also uncovered a bug in the test logic. As the diff
shows, I needed to explicitly set the `language_mode` for module code in
PreParseProgram. (Before, the Parser and PreParser were both passing/failing
for the same input, but they were doing so for different reasons. The helper
function identified the discrepancy.)

I'm a little concerned to find a bug this late in the review process, but I
take some comfort in that it only effected test code (since the pre-parser does
not traverse module bodies in production).

The change does suggest a more general improvement, though--one that effects
the business logic as well. Currently, it is possible to create a "module"
scope that is not in strict mode. This is a technically invalid state that must
be explicitly rectified by callers (as in my latest change to PreParseProgram).

Scope::SetDefaults currently has this line:

    language_mode_ = outer_scope != NULL ? outer_scope->language_mode_ : SLOPPY;

If we updated that to instead read:

    language_mode_ = outer_scope != NULL ? outer_scope->language_mode_
                                         : (is_module_scope() ? STRICT :
SLOPPY);

...then we would ensure that invalid "sloppy module" scopes could not be
created, and alleviate callers of this concern.

Practically speaking, that would let us remove the following line from
Parser::ParseModuleItemList:

    RaiseLanguageMode(STRICT);

Does this sound like a good idea to you, Adam? If so, should I address it in
this change set, or follow up with a re-factoring?

[adamk, 2016-04-20 19:35:32]

On 2016/04/14 16:40:19, mike3 wrote:
> This latest patch incorporates Caitlin's suggestion to consistently use the
> RunModuleParserSyncTest for all the new tests. Besides making the test cases
> far more readable, this also uncovered a bug in the test logic. As the diff
> shows, I needed to explicitly set the `language_mode` for module code in
> PreParseProgram. (Before, the Parser and PreParser were both passing/failing
> for the same input, but they were doing so for different reasons. The helper
> function identified the discrepancy.)
> 
> I'm a little concerned to find a bug this late in the review process, but I
> take some comfort in that it only effected test code (since the pre-parser
does
> not traverse module bodies in production).
> 
> The change does suggest a more general improvement, though--one that effects
> the business logic as well. Currently, it is possible to create a "module"
> scope that is not in strict mode. This is a technically invalid state that
must
> be explicitly rectified by callers (as in my latest change to
PreParseProgram).
> 
> Scope::SetDefaults currently has this line:
> 
>     language_mode_ = outer_scope != NULL ? outer_scope->language_mode_ :
SLOPPY;
> 
> If we updated that to instead read:
> 
>     language_mode_ = outer_scope != NULL ? outer_scope->language_mode_
>                                          : (is_module_scope() ? STRICT :
> SLOPPY);
> 
> ...then we would ensure that invalid "sloppy module" scopes could not be
> created, and alleviate callers of this concern.
> 
> Practically speaking, that would let us remove the following line from
> Parser::ParseModuleItemList:
> 
>     RaiseLanguageMode(STRICT);
> 
> Does this sound like a good idea to you, Adam? If so, should I address it in
> this change set, or follow up with a re-factoring?

Finally getting back to this, sorry for the extensive delays...

Perhaps such a refactoring could be done as a precursor to this patch?

[mike3, 2016-04-21 19:11:03]

On 2016/04/20 19:35:32, adamk wrote:
> Finally getting back to this, sorry for the extensive delays...
> 
> Perhaps such a refactoring could be done as a precursor to this patch?

Sure thing

Issue: https://bugs.chromium.org/p/v8/issues/detail?id=4941
Patch: https://codereview.chromium.org/1906923002/

[mike3, 2016-04-22 20:41:56]

Hi Adam. I've uploaded a new patch that takes advantage of
https://codereview.chromium.org/1906923002/

[adamk, 2016-04-22 22:03:04]

Almost there, just some style things.

https://codereview.chromium.org/1723313002/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1723313002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:1359: !Token::IsIdentifier(name_tok, STRICT, false, true))
{
Please just pass parsing_module_ for readability here

https://codereview.chromium.org/1723313002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:1410: if (!Token::IsIdentifier(scanner()->current_token(),
STRICT, false, true)) {
And here

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5722: RunModuleParserSyncTest(context_data,
kErrorSources, kError, NULL, 0, NULL,
No need to pass the last four arguments, they default to NULL and 0.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5730: RunModuleParserSyncTest(context_data,
error_data, kError, NULL, 0, NULL, 0);
Same here.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5750: RunModuleParserSyncTest(context_data,
kValidSources, kSuccess, NULL, 0, NULL,
And here.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5806: RunModuleParserSyncTest(context_data,
kErrorSources, kError, NULL, 0, NULL,
And finally, here.

[mike3, 2016-04-22 23:05:58]

Thanks for the feedback, Adam! I've incorporated it all in this latest patch.

https://codereview.chromium.org/1723313002/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1723313002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:1359: !Token::IsIdentifier(name_tok, STRICT, false, true))
{
On 2016/04/22 22:03:04, adamk wrote:
> Please just pass parsing_module_ for readability here

Acknowledged.

https://codereview.chromium.org/1723313002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:1410: if (!Token::IsIdentifier(scanner()->current_token(),
STRICT, false, true)) {
On 2016/04/22 22:03:04, adamk wrote:
> And here

Acknowledged.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5722: RunModuleParserSyncTest(context_data,
kErrorSources, kError, NULL, 0, NULL,
On 2016/04/22 22:03:04, adamk wrote:
> No need to pass the last four arguments, they default to NULL and 0.

Acknowledged.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5730: RunModuleParserSyncTest(context_data,
error_data, kError, NULL, 0, NULL, 0);
On 2016/04/22 22:03:04, adamk wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5750: RunModuleParserSyncTest(context_data,
kValidSources, kSuccess, NULL, 0, NULL,
On 2016/04/22 22:03:04, adamk wrote:
> And here.

Acknowledged.

https://codereview.chromium.org/1723313002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:5806: RunModuleParserSyncTest(context_data,
kErrorSources, kError, NULL, 0, NULL,
On 2016/04/22 22:03:04, adamk wrote:
> And finally, here.

Acknowledged.

[adamk, 2016-04-22 23:11:00]

Oops, one more missing thing (C++ is silly).

https://codereview.chromium.org/1723313002/diff/140001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1723313002/diff/140001/src/parsing/parser-bas...
src/parsing/parser-base.h:108: stack_limit_(stack_limit),
Please initialize parsing_module_ to false here.

[mike3, 2016-04-22 23:30:33]

Got it. I've just uploaded a patch with the initializer

https://codereview.chromium.org/1723313002/diff/140001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1723313002/diff/140001/src/parsing/parser-bas...
src/parsing/parser-base.h:108: stack_limit_(stack_limit),
On 2016/04/22 23:11:00, adamk wrote:
> Please initialize parsing_module_ to false here.

Acknowledged.

[adamk, 2016-04-22 23:32:25]

lgtm

[mike3, 2016-04-22 23:33:04]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2016-04-22 23:33:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723313002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723313002/160001

[commit-bot: I haz the power, 2016-04-22 23:50:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 23:50:19]

Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/6199)
  v8_win_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[adamk, 2016-04-23 00:05:50]

https://codereview.chromium.org/1723313002/diff/160001/src/parsing/preparser.h
File src/parsing/preparser.h (right):

https://codereview.chromium.org/1723313002/diff/160001/src/parsing/preparser....
src/parsing/preparser.h:983: Scope* scope = NewScope(scope_, is_module ?
MODULE_SCOPE : SCRIPT_SCOPE);
This is where the trouble lies. MODULE_SCOPE can't be at the top level, so
passing MODULE_SCOPE here is busted.

I think you can just remove this ternary operator and everything will continue
to work (although I'll admit it's pretty ugly).

[mike3, 2016-04-25 15:26:13]

On 2016/04/23 00:05:50, adamk wrote:

> I think you can just remove this ternary operator and everything will
> continue to work (although I'll admit it's pretty ugly).

That doesn't quite work, either, as the top-level scope must still be a module
scope if the PreParser is to behave consistently with the Parser there. It's a
bit annoying because (as we've discussed), pre-parsing top-level code is not a
real-world use case (inside a module or no), but getting consistency here is
easy, and actually closer to spec (so maybe not so ugly, after all). Uploading
a new patch in just a moment...

[adamk, 2016-04-26 18:29:11]

lgtm

[mike3, 2016-04-26 19:52:48]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2016-04-26 19:52:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723313002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723313002/180001

[commit-bot: I haz the power, 2016-04-26 19:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 19:54:10]

Try jobs failed on following builders:
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/759)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/4903)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[mike3, 2016-04-26 20:01:36]

Looks like there are new conflicts with this patch in master. I'll see what I
can do about resolving them.

[mike3, 2016-04-26 21:31:42]

The conflicts are resolved in the latest patch

[adamk, 2016-04-26 22:06:57]

still lgtm, thanks

[mike3, 2016-04-26 22:30:58]

The CQ bit was checked by mike@mikepennisi.com

[commit-bot: I haz the power, 2016-04-26 22:31:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723313002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723313002/200001

[commit-bot: I haz the power, 2016-04-26 22:48:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 22:48:50]

Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/6327)
  v8_win_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[mike3, 2016-04-29 17:38:21]

The DCHECK failure here (from a statement that I added myself) is due to a test
that includes a global "use strict" directive and that is is written to run as
a module. While redundant, this is completely valid.

I briefly considered updating call sites to only set strict mode when
necessary, but this would make SetLanguageMode harder to use without any real
benefit. Instead, I've decided to relax the DCHECK condition in order to allow
for the case of redundantly enabling strict mode for a module scope. This will
still catch truly invalid invocations--those that attempt to disable strict
mode of a module scope.

I've just uploaded a patch that includes this change.

In hopes of catching any other inadvertent failures locally, I am now testing
using the command `extrachecks=on make x64.release.check`. I'm experiencing
intermittent failures in what seem to be stress tests (example output included
below). I don't understand how this is related; is it something I should
investigate for this patch?

    $ extrachecks=on make x64.release.check

    >>> Running tests for x64.release
    No connection to distribution server; running tests locally.
    === mjsunit/verify-assert-false ===                                         
  
    #
    # Fatal error in ../src/extensions/trigger-failure-extension.cc, line 48
    # Check failed: false.
    #

    ==== C stack trace ===============================
    
     1: 0xb655f9
     2: 0x62e99c
     3: 0x94eea8
     4: 0x474e07
     5: 0x4a7457
     6: 0x480f1b
     7: 0x2c98a39089c7
    Command: /home/mike/projects/bocoup/google-es6/v8/out/x64.release/d8 --test
--random-seed=-1908414204 --nohard-abort --nodead-code-elimination
--nofold-constants --expose-trigger-failure
/home/mike/projects/bocoup/google-es6/v8/test/mjsunit/mjsunit.js
/home/mike/projects/bocoup/google-es6/v8/test/mjsunit/verify-assert-false.js
    [10:11|% 100|+ 22608|-   1]: Done                                           
  
    make: *** [x64.release.check] Error 1

[adamk, 2016-04-29 17:48:55]

Relaxed DCHECK looks fine. lgtm (the trigger failure stuff looks unrelated)

[adamk, 2016-04-29 17:49:01]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-04-29 17:49:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723313002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723313002/220001

[commit-bot: I haz the power, 2016-04-29 18:13:06]

Committed patchset #12 (id:220001)

[mike3, 2016-04-29 18:32:48]

On 2016/04/29 18:13:06, commit-bot: I haz the power wrote:
> Committed patchset #12 (id:220001)

This was a long one. Thanks for your patience and your help, Adam!

[commit-bot: I haz the power, 2016-05-01 22:04:05]

Description was changed from

==========
[parser] Enforce module-specific identifier restriction

Restrict the use of the `await` token as an identifier when parsing
source text as module code.

From
http://www.ecma-international.org/ecma-262/6.0/#sec-future-reserved-words:

> 11.6.2.2 Future Reserved Words
>
> The following tokens are reserved for used as keywords in future
> language extensions.
>
> Syntax
>
>     FutureReservedWord ::
>         enum
>         await
>
> await is only treated as a FutureReservedWord when Module is the goal
> symbol of the syntactic grammar.

BUG=v8:4767
LOG=N
R=adamk@chromium.org
==========

to

==========
[parser] Enforce module-specific identifier restriction

Restrict the use of the `await` token as an identifier when parsing
source text as module code.

From
http://www.ecma-international.org/ecma-262/6.0/#sec-future-reserved-words:

> 11.6.2.2 Future Reserved Words
>
> The following tokens are reserved for used as keywords in future
> language extensions.
>
> Syntax
>
>     FutureReservedWord ::
>         enum
>         await
>
> await is only treated as a FutureReservedWord when Module is the goal
> symbol of the syntactic grammar.

BUG=v8:4767
LOG=N
R=adamk@chromium.org

Committed: https://crrev.com/efe5b72d021f4b099e445bf1118b71df065d6e8c
Cr-Commit-Position: refs/heads/master@{#35914}
==========

[commit-bot: I haz the power, 2016-05-01 22:04:06]

Patchset 12 (id:??) landed as
https://crrev.com/efe5b72d021f4b099e445bf1118b71df065d6e8c
Cr-Commit-Position: refs/heads/master@{#35914}

[nickie, 2016-05-03 17:07:51]

nikolaos@chromium.org changed reviewers:
+ nikolaos@chromium.org

[nickie, 2016-05-03 17:07:52]

https://codereview.chromium.org/1723313002/diff/220001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (left):

https://codereview.chromium.org/1723313002/diff/220001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:1549: if (test_preparser) {
If I understand it right, this change requires the parser and the preparser to
provide the same error message in the case of modules.  This does not seem to be
very probable, as the preparser currently does not parse module-related
declarations, e.g. export declarations.  It seems that there are no tests that
break in the master branch.  But there are many in the experimental branch for
typed mode, as I found out today. :-)

Is there a plan that dictated this change?  Should I simply disable my tests?

[mike3, 2016-05-03 17:25:50]

On 2016/05/03 17:07:52, nickie wrote:
>
https://codereview.chromium.org/1723313002/diff/220001/test/cctest/test-parsi...
> File test/cctest/test-parsing.cc (left):
> 
>
https://codereview.chromium.org/1723313002/diff/220001/test/cctest/test-parsi...
> test/cctest/test-parsing.cc:1549: if (test_preparser) {
> If I understand it right, this change requires the parser and the preparser to
> provide the same error message in the case of modules.  This does not seem to
be
> very probable, as the preparser currently does not parse module-related
> declarations, e.g. export declarations.  It seems that there are no tests that
> break in the master branch.  But there are many in the experimental branch for
> typed mode, as I found out today. :-)
> 
> Is there a plan that dictated this change?  Should I simply disable my tests?

This patch concerns the use of the `await` identifier within module code, which
is independent of module declarations. You are correct that the PreParser does
not visit the module scope in production, so programs like this do not benefit
from the consistent error detection:

    var await;

...however, the PreParser does visit functions declared within module scope, so
it is important for it to report errors that match the Parser for input like
this:

    function f() {
      var await;
    }

[nickie, 2016-05-03 18:32:36]

What I'm saying is that this change practically forbids the use of
RunModuleParserSyncTest for any tests related to module syntax.  For example,
unless it is reverted or unless the preparser is modified to handle
import/export declarations, tests like
cctest/test-parsing/BasicImportExportParsing will have to be written very
verbosely (as it is now the case for this specific test), by directly calling
low-level parser functions and duplicating a lot of code.

So, I can see two plans: either the preparser is synchronized and understands
module declarations, or we add again some way (a flag, maybe?) to circumvent
this in RunModuleParserSyncTest.

[mike3, 2016-05-03 23:09:51]

Understood. I think the decision there has to come from someone who understands
the future of the PreParser (I do not).

If there is a potential use case for preparsing top-level code, then it may
make sense to update the preparser here and now.

Otherwise, a flag for RunModuleParserSyncTest is probably the best option. I
think we can agree that most tests which use that function will include module
declarations, so (if this is the preferred solution) it seems logical to skip
the PreParser by default.

Though in that case, there is a third option. We could keep
RunModuleParserSyncText simple and never run the PreParser. The tests added in
this patch could be re-written to be more verbose, explicitly invoking the
PreParser as needed. I can't think of any other aspects of "module code"
semantics that would concern the PreParser (as it operates today), so this may
be the best way forward.

nickie/adamk: What do you think?

[caitp (gmail), 2016-05-03 23:22:39]

On 2016/05/03 23:09:51, mike3 wrote:
> Understood. I think the decision there has to come from someone who
understands
> the future of the PreParser (I do not).
> 
> If there is a potential use case for preparsing top-level code, then it may
> make sense to update the preparser here and now.
> 
> Otherwise, a flag for RunModuleParserSyncTest is probably the best option. I
> think we can agree that most tests which use that function will include module
> declarations, so (if this is the preferred solution) it seems logical to skip
> the PreParser by default.
> 
> Though in that case, there is a third option. We could keep
> RunModuleParserSyncText simple and never run the PreParser. The tests added in
> this patch could be re-written to be more verbose, explicitly invoking the
> PreParser as needed. I can't think of any other aspects of "module code"
> semantics that would concern the PreParser (as it operates today), so this may
> be the best way forward.
> 
> nickie/adamk: What do you think?

For now, lets switch RunModuleParserSyncTest back to not running the preparser.
My original reading of this is that that didn't change, though, did I miss
something?

[mike3, 2016-05-03 23:25:38]

On 2016/05/03 23:22:39, caitp wrote:
> For now, lets switch RunModuleParserSyncTest back to not running the
preparser.
> My original reading of this is that that didn't change, though, did I miss
> something?

I think so. This patch removes the line

    bool test_preparser = !is_module;

and all further references to `test_preparser`

[nickie, 2016-05-04 15:06:47]

Here's what I suggest: https://codereview.chromium.org/1952473003/