[parser] Relex restriction on reserved words

[parser] Relex restriction on reserved words

Some IdentifierNames are only included in the set of FutureReservedWords
for strict mode code. Outside of strict mode, these IdentifierNames may
be used as Identifiers. Notably, this includes their use as
BindingIdentifiers in LexicalBindings.

From ES2015 12.1.1 Static Semantics: Early Errors (Identifiers):

> It is a Syntax Error if this phrase is contained in strict mode code
> and the StringValue of IdentifierName is: "implements", "interface",
> "let", "package", "private", "protected", "public", "static", or
> "yield".

http://www.ecma-international.org/ecma-262/6.0/#sec-identifiers-static-semantics-early-errors

Due to a error in its heuristic for disambiguating the `let` token, V8
does not currently allow any of the strict-mode-only FutureReservedWords
to be used as a BindingIdentifier outside of strict mode.

Update V8's heuristic for disambiguating the `let` keyword to account
for strict mode, enabling these IdentifierNames to be used

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

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

[adamk, 2016-04-20 20:42:32]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org

[adamk, 2016-04-20 20:42:32]

lgtm, but +littledan who came up with IsNextLetKeyword() in the first place to
confirm this isn't breaking something else...

[Dan Ehrenberg, 2016-04-20 20:59:56]

lgtm

lgtm % testing suggestions

This isn't new from your patch, so it doesn't have to be fixed here, but I
noticed that the errors generated here seem off by one. Although, in the
parser's logic, the 'let' is being used in a weird way, the user might expect
the strict mode reserved word to be the one that is underlined in the error.

d8> 'use strict'; let implements = 1
(d8):1: SyntaxError: Unexpected strict mode reserved word
'use strict'; let implements = 1
              ^^^
SyntaxError: Unexpected strict mode reserved word

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

https://codereview.chromium.org/1891453005/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2774: case Token::LET:  // Yes, you can do let let =
... in sloppy mode
Actually, this comment reflects a previous misunderstanding that I had. let let
= ... is disallowed. This is enforced in ParserBase::ParseAndClassifyIdentifier
(search for LetInLexicalBinding). It probably wouldn't change any behavior
(beyond how pretty the error messages are) to remove this line. Sorry for
leaving this out of date.

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:2020: V(yield)
I believe all of these extra tests would be valid with 'let' as well, due to the
static semantics early errors for use of let in a lexical binding.

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:2054: "for (let " #NAME " of []) {}",
For completeness, you can use const for most of these as well, since that has
the same lexical binding rules, right?

[Dan Ehrenberg, 2016-04-20 21:02:32]

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:2020: V(yield)
On 2016/04/20 at 20:59:56, Dan Ehrenberg wrote:
> I believe all of these extra tests would be valid with 'let' as well, due to
the static semantics early errors for use of let in a lexical binding.

Actually, I see you are using 'no let' for your sloppy mode kSuccess tests. In
this case, have you verified that there is a kError test for 'let' for all of
these cases? For the strict mode error tests, it should not be necessary to use
_NO_LET, however

[mike3, 2016-04-21 19:51:20]

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

https://codereview.chromium.org/1891453005/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2774: case Token::LET:  // Yes, you can do let let =
... in sloppy mode
On 2016/04/20 20:59:56, Dan Ehrenberg wrote:
> Actually, this comment reflects a previous misunderstanding that I had. let
let
> = ... is disallowed. This is enforced in
ParserBase::ParseAndClassifyIdentifier
> (search for LetInLexicalBinding). It probably wouldn't change any behavior
> (beyond how pretty the error messages are) to remove this line. Sorry for
> leaving this out of date.

No worries; I'll remove this line while I'm at it.

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:2020: V(yield)
On 2016/04/20 21:02:31, Dan Ehrenberg wrote:
> On 2016/04/20 at 20:59:56, Dan Ehrenberg wrote:
> > I believe all of these extra tests would be valid with 'let' as well, due to
> the static semantics early errors for use of let in a lexical binding.
> 
> Actually, I see you are using 'no let' for your sloppy mode kSuccess tests. In
> this case, have you verified that there is a kError test for 'let' for all of
> these cases? For the strict mode error tests, it should not be necessary to
use
> _NO_LET, however

Sure thing, I'll add tests for both cases.

https://codereview.chromium.org/1891453005/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:2054: "for (let " #NAME " of []) {}",
On 2016/04/20 20:59:56, Dan Ehrenberg wrote:
> For completeness, you can use const for most of these as well, since that has
> the same lexical binding rules, right?

Acknowledged.

[mike3, 2016-04-22 20:42:53]

Hello Adam and Dan. I've uploaded a patch that incorporates Dan's feedback.

[Dan Ehrenberg, 2016-04-25 21:16:17]

lgtm

[mike3, 2016-04-26 16:45:36]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2016-04-26 16:45:36]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1891453005/#ps20001
(title: "incorporate review feedback")

[commit-bot: I haz the power, 2016-04-26 16:45:39]

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

[commit-bot: I haz the power, 2016-04-26 17:03:19]

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

[commit-bot: I haz the power, 2016-04-26 17:03:19]

Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Dan Ehrenberg, 2016-05-17 20:37:35]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 20:37:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891453005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891453005/40001

[commit-bot: I haz the power, 2016-05-17 21:09:38]

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

[commit-bot: I haz the power, 2016-05-17 21:09:39]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-05-17 21:10:34]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-05-17 21:10:34]

lgtm

[Dan Ehrenberg, 2016-05-17 21:10:34]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1891453005/#ps40001
(title: "Restoring 'let let' case (with explanation)")

[commit-bot: I haz the power, 2016-05-17 21:10:38]

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

[commit-bot: I haz the power, 2016-05-17 21:12:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-17 21:14:08]

Description was changed from

==========
[parser] Relex restriction on reserved words

Some IdentifierNames are only included in the set of FutureReservedWords
for strict mode code. Outside of strict mode, these IdentifierNames may
be used as Identifiers. Notably, this includes their use as
BindingIdentifiers in LexicalBindings.

From ES2015 12.1.1 Static Semantics: Early Errors (Identifiers):

> It is a Syntax Error if this phrase is contained in strict mode code
> and the StringValue of IdentifierName is: "implements", "interface",
> "let", "package", "private", "protected", "public", "static", or
> "yield".

http://www.ecma-international.org/ecma-262/6.0/#sec-identifiers-static-semant...

Due to a error in its heuristic for disambiguating the `let` token, V8
does not currently allow any of the strict-mode-only FutureReservedWords
to be used as a BindingIdentifier outside of strict mode.

Update V8's heuristic for disambiguating the `let` keyword to account
for strict mode, enabling these IdentifierNames to be used

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

to

==========
[parser] Relex restriction on reserved words

Some IdentifierNames are only included in the set of FutureReservedWords
for strict mode code. Outside of strict mode, these IdentifierNames may
be used as Identifiers. Notably, this includes their use as
BindingIdentifiers in LexicalBindings.

From ES2015 12.1.1 Static Semantics: Early Errors (Identifiers):

> It is a Syntax Error if this phrase is contained in strict mode code
> and the StringValue of IdentifierName is: "implements", "interface",
> "let", "package", "private", "protected", "public", "static", or
> "yield".

http://www.ecma-international.org/ecma-262/6.0/#sec-identifiers-static-semant...

Due to a error in its heuristic for disambiguating the `let` token, V8
does not currently allow any of the strict-mode-only FutureReservedWords
to be used as a BindingIdentifier outside of strict mode.

Update V8's heuristic for disambiguating the `let` keyword to account
for strict mode, enabling these IdentifierNames to be used

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

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

[commit-bot: I haz the power, 2016-05-17 21:14:09]

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

[mike3, 2016-05-17 21:30:43]

On 2016/05/17 21:14:09, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/d0c65f93bf950987e6cdefc1a07b7121edb76583
> Cr-Commit-Position: refs/heads/master@{#36296}

Great! Thanks Dan and Adam!