Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing early error for duplicate lexically-bound variable declarations #487

Closed
not-an-aardvark opened this issue Dec 7, 2016 · 6 comments

Comments

@not-an-aardvark
Copy link
Contributor

Using Acorn 4.0.3:

let foo = 1;
let foo = 2;

I expected a syntax error to get thrown, because this code is invalid syntax according to the spec. However, acorn parsed it without any errors.

Relevant portion of the spec:

It is a Syntax Error if the LexicallyDeclaredNames of StatementList contains any duplicate entries.

not-an-aardvark added a commit to eslint/eslint that referenced this issue Dec 7, 2016
Due to acornjs/acorn#487, espree does not throw a syntax error for duplicate `let` or `const` declarations. Previously, the `prefer-const` rule assumed that a `let` variable would always have exactly one declaration. This adds a sanity check to avoid reporting (and possibly creating incorrect autofixes) a `let` variable if it has more than one declaration.
not-an-aardvark added a commit to eslint/eslint that referenced this issue Dec 8, 2016
Due to acornjs/acorn#487, espree does not throw a syntax error for duplicate `let` or `const` declarations. Previously, the `prefer-const` rule assumed that a `let` variable would always have exactly one declaration. This adds a sanity check to avoid reporting (and possibly creating incorrect autofixes) a `let` variable if it has more than one declaration.
@oliverwoodings
Copy link

Bump

@nzakas
Copy link
Contributor

nzakas commented Jan 11, 2017

This needs to be fixed in Acorn for it to flow through here, although, I'm not sure this check is feasible implementation-wise.

@oliverwoodings please don't do that. We are all very busy and sometimes it takes longer for us to get around to less critical issues.

@not-an-aardvark
Copy link
Contributor Author

This needs to be fixed in Acorn for it to flow through here, although, I'm not sure this check is feasible implementation-wise.

I'm confused -- this is Acorn.

@oliverwoodings
Copy link

@nzakas apologies, I wouldn't normally, but given this was posted a month ago and has had zero responses, what else should I do? Whilst it may not seem critical, it is causing myself and others issues downstream.

@nzakas
Copy link
Contributor

nzakas commented Jan 12, 2017

@not-an-aardvark sorry, some reason I thought this was in Espree.

@oliverwoodings something like, "I'm affected by this, is there anything I can do to help address it?" is much nicer. "Bump" is a bit rude as it triggers notifications without providing any additional information.

@oliverwoodings
Copy link

Will do next time 👍

not-an-aardvark added a commit to not-an-aardvark/acorn that referenced this issue Feb 14, 2017
This implements the static errors specified [here](https://tc39.github.io/ecma262/#sec-block-static-semantics-early-errors):

> It is a Syntax Error if the LexicallyDeclaredNames of StatementList contains any duplicate entries.

> It is a Syntax Error if any element of the LexicallyDeclaredNames of StatementList also occurs in the VarDeclaredNames of StatementList.

In other words, if a `let`/`const` variable is declared in the same scope as another variable with the same name, a parsing error should be thrown. This is implemented with the following steps:

* When entering a function scope, create an empty set of var-declared names
* When entering a block scope, create a new empty set of lexically-declared names. Additionally, push the current set of var-declared names, and work with a new empty set.
* When exiting a block scope, merge the set of var-declared names from the block with the set of var-declared names from the parent scope.
* When parsing a binding pattern (e.g. a variable declaration or function parameters), check if the bound identifier clashes with any lexically-declared names (if the bound identifier is non-lexical), or any variable names at all (if the bound identifier is lexical). If so, raise a parsing error. Otherwise, add it to the current lexically-declared name set or var-declared name set as appropriate.

The merging behavior for var-declared names in the third bullet-point is necessary due to how `var` scoping works. For example, this should be a syntax error:

```js
{
  var foo = 1;
}

let foo = 1;
```

But this should not be a syntax error:

```js
{
  var foo = 1;
  {
    let foo = 1;
  }
}
```
not-an-aardvark added a commit to not-an-aardvark/acorn that referenced this issue Feb 17, 2017
This implements the static errors specified [here](https://tc39.github.io/ecma262/#sec-block-static-semantics-early-errors):

> It is a Syntax Error if the LexicallyDeclaredNames of StatementList contains any duplicate entries.

> It is a Syntax Error if any element of the LexicallyDeclaredNames of StatementList also occurs in the VarDeclaredNames of StatementList.

In other words, if a `let`/`const` variable is declared in the same scope as another variable with the same name, a parsing error should be thrown. This is implemented with the following steps:

* When entering a function scope, create an empty set of var-declared names
* When entering a block scope, create a new empty set of lexically-declared names. Additionally, push the current set of var-declared names, and work with a new empty set.
* When exiting a block scope, merge the set of var-declared names from the block with the set of var-declared names from the parent scope.
* When parsing a binding pattern (e.g. a variable declaration or function parameters), check if the bound identifier clashes with any lexically-declared names (if the bound identifier is non-lexical), or any variable names at all (if the bound identifier is lexical). If so, raise a parsing error. Otherwise, add it to the current lexically-declared name set or var-declared name set as appropriate.

The merging behavior for var-declared names in the third bullet-point is necessary due to how `var` scoping works. For example, this should be a syntax error:

```js
{
  var foo = 1;
}

let foo = 1;
```

But this should not be a syntax error:

```js
{
  var foo = 1;
  {
    let foo = 1;
  }
}
```
not-an-aardvark added a commit to eslint/eslint that referenced this issue Jun 3, 2017
This code was added to work around a bug in acorn (acornjs/acorn#487), which has since been fixed.
not-an-aardvark added a commit to eslint/eslint that referenced this issue Jun 3, 2017
This code was added to work around a bug in acorn (acornjs/acorn#487), which has since been fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants