1095439 - Assigning to a const variable is a syntax error

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S]

We already produce a syntax error when assigning to a const in strict mode, but from understanding of 12.14.1 this should throw in normal code as well.

"It is a Syntax Error if LeftHandSideExpression is an IdentifierReference that can be statically determined to always resolve to a declarative environment record binding and the resolved binding is an immutable binding."

Comment 1

[Tom S]

Attachment: v1 - Implement new behavior

Currently pushing this to try to see what needs to be fixed in the browser.

Comment 2

[Tom S]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29afe5026081

Comment 3

[Tom S]

Attachment: v2 - Assining to const is a syntax error

We might actually get away with this. :) I think we can remove these and probably even more ->isConst checks, now that this disallowed so early already. But I am not totally sure, e.g. what about JSOP_NAME?

Also included is a change from TypeError to SyntaxError, which should match what the spec wants.

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 8522583 [details] [diff] [review]
v2 - Assining to const is a syntax error

Review of attachment 8522583 [details] [diff] [review]:
-----------------------------------------------------------------

While I feel a little queasy about deleting these tests, I feel like they are all fundamentally illegal, now.

r=me with error message changed, and comment about asm.js test addressed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1688,5 @@
>          if (pn->isConst()) {
> +            JSAutoByteString name;
> +            if (!AtomToPrintableString(cx, pn->pn_atom, &name))
> +                return false;
> +            bce->reportError(pn, JSMSG_REDECLARED_CONST, name.ptr());

As discussed on IRC, this is not the right error message for this action.

::: js/src/jit-test/tests/asm.js/testGlobals.js
@@ +60,5 @@
>  assertAsmTypeFail('global', USE_ASM + "var i=global.i|0; function f() { return i|0 } return f");
>  assertAsmTypeFail('global', USE_ASM + "const i=global.i|0; function f() { return i|0 } return f");
>  assertAsmTypeFail('global', USE_ASM + "var i=global.i|0; function f() { return i|0 } return f");
>  assertAsmTypeFail('global', USE_ASM + 'var i=global.Infinity; function f() { i = 0.0 } return f');
> +// assertAsmTypeFail('global', USE_ASM + 'const i=global.Infinity; function f() { i = 0.0 } return f');

What's the deal with this line? Do we have something to replace it with, or should we delete it?

::: js/src/jit-test/tests/saved-stacks/gc-frame-cache.js
@@ +8,5 @@
>      throw new Error("Assertion failed: expected about " + expected + ", got " + actual +
>                      ". FUZZ_FACTOR = " + FUZZ_FACTOR);
>  }
>  
> +var stacks = [];

why is this necessary?

::: js/src/tests/js1_5/Regress/regress-383674.js
@@ +43,5 @@
>  
>    actual = 'toString not called';
>    try
>    {
> +    (function() { var a = 

nit: preexisting: whitespace after =

::: js/src/tests/js1_6/extensions/regress-465443.js
@@ +18,5 @@
>  {
>    enterFunc ('test');
>    printBugNumber(BUGNUMBER);
>    printStatus (summary);
>   

nit: preexisting: whitespace on blank line

::: js/src/tests/js1_8_1/extensions/regress-437288-01.js
@@ +19,5 @@
>    enterFunc ('test');
>    printBugNumber(BUGNUMBER);
>    printStatus (summary);
>  
> +  expect = 'SyntaxError: redeclaration of const x';

This is a good example of why the error message is incorrect. Really the error here is tha the loop cannot assign to a const variable, so it is indeed an incorrect for loop LHS. I would prefer if we could leave this error message intact?

Comment 5

[Tom S]

(In reply to Eric Faust [:efaust] from comment #4)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1688,5 @@
> >          if (pn->isConst()) {
> > +            JSAutoByteString name;
> > +            if (!AtomToPrintableString(cx, pn->pn_atom, &name))
> > +                return false;
> > +            bce->reportError(pn, JSMSG_REDECLARED_CONST, name.ptr());
> 
> As discussed on IRC, this is not the right error message for this action.
>
Yes. const x = 1; const x = 2; will still show the right message.
> ::: js/src/jit-test/tests/asm.js/testGlobals.js
> @@ +60,5 @@
> >  assertAsmTypeFail('global', USE_ASM + "var i=global.i|0; function f() { return i|0 } return f");
> >  assertAsmTypeFail('global', USE_ASM + "const i=global.i|0; function f() { return i|0 } return f");
> >  assertAsmTypeFail('global', USE_ASM + "var i=global.i|0; function f() { return i|0 } return f");
> >  assertAsmTypeFail('global', USE_ASM + 'var i=global.Infinity; function f() { i = 0.0 } return f');
> > +// assertAsmTypeFail('global', USE_ASM + 'const i=global.Infinity; function f() { i = 0.0 } return f');
> 
> What's the deal with this line? Do we have something to replace it with, or
> should we delete it?
> 
I just deleted them now. As far as I can tell we don't handle SyntaxErrors that would be thrown in normal JS as well.
> ::: js/src/jit-test/tests/saved-stacks/gc-frame-cache.js
> @@ +8,5 @@
> >      throw new Error("Assertion failed: expected about " + expected + ", got " + actual +
> >                      ". FUZZ_FACTOR = " + FUZZ_FACTOR);
> >  }
> >  
> > +var stacks = [];
> 
> why is this necessary?
stacks = null is done later.

Comment 6

[Tom S]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=633052c58bac
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2967c20784

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef2967c20784

Comment 9

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const
https://developer.mozilla.org/en-US/Firefox/Releases/36#JavaScript

A review would be really great.