Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

Fix arrow function scope strictness #50

Merged
merged 1 commit into from
Feb 22, 2019
Merged

Fix arrow function scope strictness #50

merged 1 commit into from
Feb 22, 2019

Conversation

futpib
Copy link
Contributor

@futpib futpib commented Feb 19, 2019

Fixes #49

@jsf-clabot
Copy link

jsf-clabot commented Feb 19, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

platinumazure
platinumazure approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contributing!

mysticatea
mysticatea approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@not-an-aardvark
Copy link
Member

Will this have an effect on arrow functions with duplicate arguments?

(duplicateName, duplicateName) => {}

Duplicate argument names are disallowed in strict mode functions, and also in arrow functions. (Aside from duplicate arguments, I think there might be other features like this, but I'm not sure.) If the previous code was relying on arrow functions always being considered strict mode in order to implement those checks, then it might be necessary to do the checks explicitly now in order to preserve correct behavior.

(I'm not very familiar with the eslint-scope codebase, so I'm not sure whether this is actually the case. I'm just bringing it up since it seems like a plausible thing that someone could have missed.)

@mysticatea
Copy link
Member

mysticatea commented Feb 19, 2019 •
edited
Loading

I don't think eslint-scope checks duplication. If a variable was redeclared, the Variable object has two Definitions in defs property. We can check redeclaration with that.

@not-an-aardvark
Copy link
Member

Ok, sounds good.

@mysticatea mysticatea merged commit 2533966 into eslint:master Feb 22, 2019
@futpib futpib deleted the arrow-strictness branch February 22, 2019 08:10
not-an-aardvark added a commit that referenced this pull request Mar 2, 2019
@not-an-aardvark
Revert "Fix: Arrow function scope strictness (fixes #49) (#50)"
b9ddacc 
This reverts commit 2533966.

The change is causing ESLint to crash with a parsing error on the following code:

```js
console.log(this); z(x => console.log(x, this))
```

```
TypeError: Cannot read property 'length' of undefined
    at isStrictScope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope.js:93:40)
    at new Scope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope.js:254:25)
    at new FunctionScope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope.js:641:9)
    at ScopeManager.__nestFunctionScope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope-manager.js:209:33)
    at Referencer.visitFunction (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/referencer.js:200:27)
    at Referencer.ArrowFunctionExpression (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/referencer.js:567:14)
    at Referencer.Visitor.visit (/Users/tkatz/code/eslint/node_modules/esrecurse/esrecurse.js:104:34)
    at Referencer.Visitor.visitChildren (/Users/tkatz/code/eslint/node_modules/esrecurse/esrecurse.js:83:38)
    at Referencer.CallExpression (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/referencer.js:494:14)
    at Referencer.Visitor.visit (/Users/tkatz/code/eslint/node_modules/esrecurse/esrecurse.js:104:34)
```

Since this is currently affecting ESLint users, we're reverting the change for now until we figure out the root cause.
not-an-aardvark added a commit that referenced this pull request Mar 2, 2019
@not-an-aardvark
Revert "Fix: Arrow function scope strictness (fixes #49) (#50)" (#51)
c925600 
This reverts commit 2533966.

The change is causing ESLint to crash with a parsing error on the following code:

```js
console.log(this); z(x => console.log(x, this))
```

```
TypeError: Cannot read property 'length' of undefined
    at isStrictScope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope.js:93:40)
    at new Scope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope.js:254:25)
    at new FunctionScope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope.js:641:9)
    at ScopeManager.__nestFunctionScope (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/scope-manager.js:209:33)
    at Referencer.visitFunction (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/referencer.js:200:27)
    at Referencer.ArrowFunctionExpression (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/referencer.js:567:14)
    at Referencer.Visitor.visit (/Users/tkatz/code/eslint/node_modules/esrecurse/esrecurse.js:104:34)
    at Referencer.Visitor.visitChildren (/Users/tkatz/code/eslint/node_modules/esrecurse/esrecurse.js:83:38)
    at Referencer.CallExpression (/Users/tkatz/code/eslint/node_modules/eslint-scope/lib/referencer.js:494:14)
    at Referencer.Visitor.visit (/Users/tkatz/code/eslint/node_modules/esrecurse/esrecurse.js:104:34)
```

Since this is currently affecting ESLint users, we're reverting the change for now until we figure out the root cause.
@not-an-aardvark
Copy link
Member

FYI, this change has been temporarily reverted because it was causing some crashes in ESLint. See #51 for more information.

futpib added a commit to futpib/eslint-scope that referenced this pull request Mar 2, 2019
@futpib
Fix: Arrow function scope strictness (fixes eslint#49) (eslint#50)
82085ed
@futpib futpib mentioned this pull request Mar 2, 2019
Merged
not-an-aardvark pushed a commit that referenced this pull request Mar 15, 2019
@futpib @mysticatea @not-an-aardvark
Fix: arrow function scope strictness (take 2) (#52)
299df64 
* Fix: Arrow function scope strictness (fixes #49) (#50)

* Fix: Error on functions with non-block body

* Apply suggestion from review

Co-authored-by: Toru Nagashima <star.ctor@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mysticatea mysticatea mysticatea approved these changes

@platinumazure platinumazure platinumazure approved these changes

Assignees
No one assigned
Labels
triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@futpib @jsf-clabot @not-an-aardvark @mysticatea @platinumazure