[strong] Implement static restrictions on binding/assignment to 'undefined' identifier.

[strong] Implement static restrictions on binding/assignment to 'undefined'
identifier. Delete unused (and now incorrect) function IsValidStrictVariable.

Implements the strong mode proposal's static restrictions on the use of the
identifier 'undefined'. Assumes these restrictions are intended to be identical
to the restrictions on the use of 'eval' and 'arguments' in strict mode. The
AllowEvalOrArgumentsAsIdentifier enum has been renamed to
AllowRestrictedIdentifiers as logic involving it is now also used for this case.

BUG=v8:3956
LOG=N

Committed: https://crrev.com/8ef7159582dde933c71a7c6d73b6e936cf42a5cb
Cr-Commit-Position: refs/heads/master@{#27744}

[conradw, 2015-04-08 09:23:48]

Patchset #1 (id:1) has been deleted

[conradw, 2015-04-08 09:35:20]

conradw@chromium.org changed reviewers:
+ rossberg@chromium.org

[conradw, 2015-04-08 09:35:49]

On 2015/04/08 09:35:20, conradw wrote:
> mailto:conradw@chromium.org changed reviewers:
> + mailto:rossberg@chromium.org

PTAL

[rossberg, 2015-04-08 11:30:11]

Mostly looking good, just needs more test coverage.

https://codereview.chromium.org/1070633002/diff/20001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1070633002/diff/20001/src/messages.js#newcode169
src/messages.js:169: strong_undefined:              ["In strong mode, binding or
assigning to 'undefined' is not allowed"],
"is not allowed" -> "is deprecated"

https://codereview.chromium.org/1070633002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1070633002/diff/20001/src/preparser.h#newcode149
src/preparser.h:149: enum AllowRestrictedIdentifiers {
That's nicer indeed.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
File test/mjsunit/strong/undefined.js (right):

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:7: //repurposing the strict mode 'eval' and
'arguments' tests to test for correct
Style nit: Spaces after // (here and below). Also, upper case.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:8: //behaviour of 'undefined' as an identifier
in strong mode
Please add tests for:

- bindings to undefined (var, let, const)
- bindings in for-loops (for, for-in, for-of)
- function expressions
- arrow function parameters
- classes named undefined (declarations, expressions)

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:28: }
We also should have test contexts that are (strict or strong) classes.

More importantly, test contexts where the 'use strong' is _inside_ the function
(or class) you are checking. For example, to cover

  function undefined() { 'use strong'; }
  function(undefined) { 'use strong'; }
  class undefined { 'use strong'; }

etc. Though I'm not sure what the best generalisation of the CheckStrongMode
helper is to support that.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:32: for (var i = 0; i < arguments.length; i ++)
{
With adding --harmony-rest-parameters to Flags you should now be able to say

function CheckFunctionConstructorStrongMode(...args)

and drop the loop.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:64: CheckStrongMode("function strong() {
print(undefined = undefined); }", SyntaxError);
Style nit: line length (here and below)

[conradw, 2015-04-08 17:19:45]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-04-08 17:19:45]

The CQ bit was unchecked by conradw@chromium.org

[conradw, 2015-04-08 17:19:52]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-04-08 17:19:52]

The CQ bit was unchecked by conradw@chromium.org

[conradw, 2015-04-09 12:22:22]

https://codereview.chromium.org/1070633002/diff/20001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1070633002/diff/20001/src/messages.js#newcode169
src/messages.js:169: strong_undefined:              ["In strong mode, binding or
assigning to 'undefined' is not allowed"],
On 2015/04/08 11:30:11, rossberg wrote:
> "is not allowed" -> "is deprecated"

Done.

[conradw, 2015-04-09 12:24:17]

https://codereview.chromium.org/1070633002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1070633002/diff/20001/src/preparser.h#newcode149
src/preparser.h:149: enum AllowRestrictedIdentifiers {
On 2015/04/08 11:30:11, rossberg wrote:
> That's nicer indeed.

Acknowledged.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
File test/mjsunit/strong/undefined.js (right):

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:7: //repurposing the strict mode 'eval' and
'arguments' tests to test for correct
On 2015/04/08 11:30:11, rossberg wrote:
> Style nit: Spaces after // (here and below). Also, upper case.

Done.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:8: //behaviour of 'undefined' as an identifier
in strong mode
On 2015/04/08 11:30:11, rossberg wrote:
> Please add tests for:
> 
> - bindings to undefined (var, let, const)
> - bindings in for-loops (for, for-in, for-of)
> - function expressions
> - arrow function parameters
> - classes named undefined (declarations, expressions)

Done.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:28: }
On 2015/04/08 11:30:11, rossberg wrote:
> We also should have test contexts that are (strict or strong) classes.
> 
> More importantly, test contexts where the 'use strong' is _inside_ the
function
> (or class) you are checking. For example, to cover
> 
>   function undefined() { 'use strong'; }
>   function(undefined) { 'use strong'; }
>   class undefined { 'use strong'; }
> 
> etc. Though I'm not sure what the best generalisation of the CheckStrongMode
> helper is to support that.

Done.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:32: for (var i = 0; i < arguments.length; i ++)
{
On 2015/04/08 11:30:11, rossberg wrote:
> With adding --harmony-rest-parameters to Flags you should now be able to say
> 
> function CheckFunctionConstructorStrongMode(...args)
> 
> and drop the loop.

Acknowledged.

https://codereview.chromium.org/1070633002/diff/20001/test/mjsunit/strong/und...
test/mjsunit/strong/undefined.js:64: CheckStrongMode("function strong() {
print(undefined = undefined); }", SyntaxError);
On 2015/04/08 11:30:11, rossberg wrote:
> Style nit: line length (here and below)

Done.

[rossberg, 2015-04-09 13:40:18]

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
File test/mjsunit/strong/undefined.js (right):

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:5: // Flags: --strong-mode --harmony-classes
--harmony-sloppy --harmony-arrow-functions
You shouldn't need --harmony-classes anymore, it's on by default.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:11: if(strictThrowsDyn) {
Instead of having this tedious flag, I'd perhaps do

assertThrows("'use strict';\n" + code + "; throw TypeError;", TypeError);

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:19: var funcs = ["sloppy() { " + code + " }",
Hm, I'm not sure what this tests. The outer/inner below already seemed to cover
these cases.

But I sympathise with considering more contexts using loops. How about changing
this whole function into something like

  let sloppyContexts = [
    ["", ""],
    ["function outer() {", "}"],
  ]
  let strictContexts = [
    ["'use strict';", ""],
    ["function outer() { 'use strict';", "}"],
    ["function outer() { 'use strict'; function inner() {", "}}"],
    ["class C { m() {", "} }"],
  ]
  let strongContexts = [
    ["'use strong';", ""],
    ["function outer() { 'use strong';", "}"],
    ["function outer() { 'use strong'; function inner() {", "}}"],
    ["class C { m() { 'use strong';", "} }"],
  ]

  for (let context of sloppyContexts) {
    assertDoesNotThrow(context[0] + code + context[1])
  }
  for (let context of strictContexts) {
    assertThrows(context[0] + code + context[1] + "; throw TypeError",
TypeError)
  }
  for (let context of strongContexts) {
    assertThrows(context[0] + code + context[1], SyntaxError)
  }

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:54: assertThrows(strongClass, SyntaxError);
I don't think this should throw. Method names are not variable names but
property names, which can be anything, including keywords. In particular, the
following is legal, too:

  class C { eval() {} }
  class C { class() {} }

So I think CheckClassEnv isn't needed. What I meant earlier with testing class
contexts was merely extending CheckStrongMode with contexts introducing
strict/strong mode with a class (see suggestion above).

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:60: CheckStrongMode("const undefined = 0;",
false)
Perhaps also add

  let undefined;
  let x, y = 0, undefined;

etc.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:73: // FunctionExpression named 'undefined';
Some more cases: generators and methods with undefined parameters:

  function* g(undefined) {}
  (function*(undefined) {})
  ({ m(undefined) {} })
  ({ *m(undefined) {} })
  (class { m(undefined) {} })
  (class { *m(undefined) {} })

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:113:
CheckStrongMode("try{}catch(undefined){};", false);
Nit: no reason to be cheap with spaces. :)

[conradw, 2015-04-09 16:53:09]

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
File test/mjsunit/strong/undefined.js (right):

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:5: // Flags: --strong-mode --harmony-classes
--harmony-sloppy --harmony-arrow-functions
On 2015/04/09 13:40:17, rossberg wrote:
> You shouldn't need --harmony-classes anymore, it's on by default.

Done.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:11: if(strictThrowsDyn) {
On 2015/04/09 13:40:17, rossberg wrote:
> Instead of having this tedious flag, I'd perhaps do
> 
> assertThrows("'use strict';\n" + code + "; throw TypeError;", TypeError);

Done.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:19: var funcs = ["sloppy() { " + code + " }",
On 2015/04/09 13:40:17, rossberg wrote:
> Hm, I'm not sure what this tests. The outer/inner below already seemed to
cover
> these cases.
> 
> But I sympathise with considering more contexts using loops. How about
changing
> this whole function into something like
> 
>   let sloppyContexts = [
>     ["", ""],
>     ["function outer() {", "}"],
>   ]
>   let strictContexts = [
>     ["'use strict';", ""],
>     ["function outer() { 'use strict';", "}"],
>     ["function outer() { 'use strict'; function inner() {", "}}"],
>     ["class C { m() {", "} }"],
>   ]
>   let strongContexts = [
>     ["'use strong';", ""],
>     ["function outer() { 'use strong';", "}"],
>     ["function outer() { 'use strong'; function inner() {", "}}"],
>     ["class C { m() { 'use strong';", "} }"],
>   ]
> 
>   for (let context of sloppyContexts) {
>     assertDoesNotThrow(context[0] + code + context[1])
>   }
>   for (let context of strictContexts) {
>     assertThrows(context[0] + code + context[1] + "; throw TypeError",
> TypeError)
>   }
>   for (let context of strongContexts) {
>     assertThrows(context[0] + code + context[1], SyntaxError)
>   }

Done, although without the sloppy case, since most tests make use of let/const
etc.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:54: assertThrows(strongClass, SyntaxError);
On 2015/04/09 13:40:17, rossberg wrote:
> I don't think this should throw. Method names are not variable names but
> property names, which can be anything, including keywords. In particular, the
> following is legal, too:
> 
>   class C { eval() {} }
>   class C { class() {} }
> 
> So I think CheckClassEnv isn't needed. What I meant earlier with testing class
> contexts was merely extending CheckStrongMode with contexts introducing
> strict/strong mode with a class (see suggestion above).

You're right, and I'm ashamed to admit that the test appeared to pass due to
"SyntaxError: unexpected ;"

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:60: CheckStrongMode("const undefined = 0;",
false)
On 2015/04/09 13:40:17, rossberg wrote:
> Perhaps also add
> 
>   let undefined;
>   let x, y = 0, undefined;
> 
> etc.

Done.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:73: // FunctionExpression named 'undefined';
On 2015/04/09 13:40:17, rossberg wrote:
> Some more cases: generators and methods with undefined parameters:
> 
>   function* g(undefined) {}
>   (function*(undefined) {})
>   ({ m(undefined) {} })
>   ({ *m(undefined) {} })
>   (class { m(undefined) {} })
>   (class { *m(undefined) {} })

Done.

https://codereview.chromium.org/1070633002/diff/100001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:113:
CheckStrongMode("try{}catch(undefined){};", false);
On 2015/04/09 13:40:17, rossberg wrote:
> Nit: no reason to be cheap with spaces. :)

Done.

[rossberg, 2015-04-10 07:31:29]

LGTM, modulo minor tweaks

https://codereview.chromium.org/1070633002/diff/120001/test/mjsunit/strong/un...
File test/mjsunit/strong/undefined.js (right):

https://codereview.chromium.org/1070633002/diff/120001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:12: assertThrows("'use strict'; " + code + ";
throw new TypeError();", TypeError);
I think you can drop these 3 lines now, they are already induced by the loops
below (or can be, for the last one).

https://codereview.chromium.org/1070633002/diff/120001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:69: assertThrows("class C { foo(a, b,
undefined, c, d) {'use strong';} }",
Also add a case for methods in object literals:

  ({ foo(...) {...} })

Similarly for generator methods.

[conradw, 2015-04-10 11:02:06]

Also added parser sync tests - PTAL

https://codereview.chromium.org/1070633002/diff/120001/test/mjsunit/strong/un...
File test/mjsunit/strong/undefined.js (right):

https://codereview.chromium.org/1070633002/diff/120001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:12: assertThrows("'use strict'; " + code + ";
throw new TypeError();", TypeError);
On 2015/04/10 07:31:28, rossberg wrote:
> I think you can drop these 3 lines now, they are already induced by the loops
> below (or can be, for the last one).

Done.

https://codereview.chromium.org/1070633002/diff/120001/test/mjsunit/strong/un...
test/mjsunit/strong/undefined.js:69: assertThrows("class C { foo(a, b,
undefined, c, d) {'use strong';} }",
On 2015/04/10 07:31:28, rossberg wrote:
> Also add a case for methods in object literals:
> 
>   ({ foo(...) {...} })
> 
> Similarly for generator methods.

Done.

[rossberg, 2015-04-10 11:33:57]

lgtm

[conradw, 2015-04-10 11:42:10]

The CQ bit was checked by conradw@chromium.org

[commit-bot: I haz the power, 2015-04-10 11:42:20]

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

[commit-bot: I haz the power, 2015-04-10 12:04:57]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-04-10 12:05:12]

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