1198833 - Redeclaration of variables should be a SyntaxError

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

Attachment: make-redeclaration-a-syntax-error

For scripts, modules and function bodies the spec says it is a syntax error if:
 - LexicallyDeclaredNames contains duplicate entries
 - any element of the LexicallyDeclaredNames also occurs in the VarDeclaredNames 

Currently we report a TypeError for this.

The comment in tests/ecma_6/LexicalEnvironment/for-loop.js makes me slightly leery of just changing this though:

// This is wrong!  Per 13.2.1.1, "It is a Syntax Error if the BoundNames of
// BindingList contains any duplicate entries."  But we don't implement this
// yet, so it becomes a TypeError at runtime.

I don't understand what the last bit is driving at.

Comment 1

[Shu-yu Guo [:shu]]

Let's wait on this until I finish global lexicals, which will turn up more redecl errors.

We can plug the holes after that. Mixing the two efforts is going to be confusing.

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 8652939 [details] [diff] [review]
make-redeclaration-a-syntax-error

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

Cancelling feedback based on Comment 1. Feel free to rerequest if this is in error. This patch at least looks very strange, because it makes no changes to the existing system, though I admit it looks like that error is already only thrown in the parser.

Comment 3

[Jon Coppeard (:jonco) (PTO until 27th Jan)]

Attachment: make-redeclaration-a-syntax-error v2

Posting an updated patch now global lexical support has landed.

The patch just changes the exception type of JSMSG_REDECLARED_VAR to be JSEXN_SYNTAXERR rather than JSEXN_TYPEERR and updates the tests.

Is there anything else I need to do to make this work?

Looking at js.msg again it seems there a couple of other errors (JSMSG_REDECLARED_PARAM and JSMSG_REDECLARED_CATCH_IDENTIFIER) that should also be syntax errors according to the spec.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8692613 [details] [diff] [review]
make-redeclaration-a-syntax-error v2

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

We should land this now.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d447860f4550

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 7

[Kohei Yoshino]

Updated the site compatibility docs:
https://www.fxsitecompat.com/en-CA/docs/2014/es6-compatible-let-disallows-variable-redeclaration/
https://www.fxsitecompat.com/en-CA/docs/2015/variables-defined-with-const-and-let-are-no-longer-properties-on-window-redeclaration-with-let-will-throw/

Comment 8

[Florian Scholz (Open Web Docs)]

Added to
https://developer.mozilla.org/en-US/Firefox/Releases/46#JavaScript

Updated 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const