Closed Bug 1095439 Opened 10 years ago Closed 10 years ago

Assigning to a const variable is a syntax error

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(1 file, 1 obsolete file)

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."
See Also: → 1094995
Assignee: nobody → evilpies
Attached patch v1 - Implement new behavior (obsolete) — Splinter Review
Currently pushing this to try to see what needs to be fixed in the browser.
Attachment #8521757 - Flags: review?(efaustbmo)
Attachment #8521757 - Flags: review?(efaustbmo)
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.
Attachment #8521757 - Attachment is obsolete: true
Attachment #8522583 - Flags: review?(efaustbmo)
Attachment #8522583 - Attachment is patch: true
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?
Attachment #8522583 - Flags: review?(efaustbmo) → review+
(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.
Blocks: es6:let
No longer blocks: es6
https://hg.mozilla.org/mozilla-central/rev/ef2967c20784
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.