Redeclaration of variables should be a SyntaxError

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla46
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8652939 [details] [diff] [review]
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.
Attachment #8652939 - Flags: feedback?(efaustbmo)
(Assignee)

Updated

3 years ago
Assignee: nobody → jcoppeard

Comment 1

3 years ago
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 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.
Attachment #8652939 - Flags: feedback?(efaustbmo)
(Assignee)

Comment 3

3 years ago
Created attachment 8692613 [details] [diff] [review]
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.
Attachment #8652939 - Attachment is obsolete: true
Attachment #8692613 - Flags: feedback?(efaustbmo)

Comment 4

3 years ago
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.
Attachment #8692613 - Flags: feedback?(efaustbmo) → review+

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d447860f4550
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.