From 1c354babebd7eb851323669e0a0d7247a5083d6d Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Wed, 21 Dec 2016 17:04:41 -0500 Subject: [PATCH 1/4] deshadow destructured parameters with assignments (#1078) --- package.json | 1 + src/ast/nodes/Identifier.js | 19 +++++++++++-- src/ast/nodes/MemberExpression.js | 2 +- src/ast/nodes/shared/callHasEffects.js | 2 +- src/ast/utils/isReference.js | 28 ------------------- .../_config.js | 4 +++ .../deshadowed-destructured-parameter/main.js | 10 +++++++ .../deshadowed-destructured-parameter/x.js | 1 + 8 files changed, 35 insertions(+), 32 deletions(-) delete mode 100644 src/ast/utils/isReference.js create mode 100644 test/function/deshadowed-destructured-parameter/_config.js create mode 100644 test/function/deshadowed-destructured-parameter/main.js create mode 100644 test/function/deshadowed-destructured-parameter/x.js diff --git a/package.json b/package.json index 1247a19..8e85a0b 100644 --- a/package.json +++ b/package.json @@ -55,6 +55,7 @@ "eslint": "^2.13.0", "eslint-plugin-import": "^1.14.0", "estree-walker": "^0.2.1", + "is-reference": "^1.0.0", "istanbul": "^0.4.3", "magic-string": "^0.15.2", "minimist": "^1.2.0", diff --git a/src/ast/nodes/Identifier.js b/src/ast/nodes/Identifier.js index 836462b..3444402 100644 --- a/src/ast/nodes/Identifier.js +++ b/src/ast/nodes/Identifier.js @@ -1,9 +1,24 @@ import Node from '../Node.js'; -import isReference from '../utils/isReference.js'; +import isReference from 'is-reference'; + +function isAssignmentPatternLhs ( node, parent ) { + // special case: `({ foo = 42 }) => {...}` + // `foo` actually has two different parents, the Property of the + // ObjectPattern, and the AssignmentPattern. In one case it's a + // reference, in one case it's not, because it's shorthand for + // `({ foo: foo = 42 }) => {...}`. But unlike a regular shorthand + // property, the `foo` node appears at different levels of the tree + return ( + parent.type === 'Property' && + parent.shorthand && + parent.value.type === 'AssignmentPattern' && + parent.value.left === node + ); +} export default class Identifier extends Node { bind ( scope ) { - if ( isReference( this, this.parent ) ) { + if ( isReference( this, this.parent ) || isAssignmentPatternLhs( this, this.parent ) ) { this.declaration = scope.findDeclaration( this.name ); this.declaration.addReference( this ); // TODO necessary? } diff --git a/src/ast/nodes/MemberExpression.js b/src/ast/nodes/MemberExpression.js index eca57a2..bcdcf26 100644 --- a/src/ast/nodes/MemberExpression.js +++ b/src/ast/nodes/MemberExpression.js @@ -1,6 +1,6 @@ +import isReference from 'is-reference'; import getLocation from '../../utils/getLocation.js'; import relativeId from '../../utils/relativeId.js'; -import isReference from '../utils/isReference.js'; import Node from '../Node.js'; import { UNKNOWN } from '../values.js'; diff --git a/src/ast/nodes/shared/callHasEffects.js b/src/ast/nodes/shared/callHasEffects.js index 9660b8a..ce48fc0 100644 --- a/src/ast/nodes/shared/callHasEffects.js +++ b/src/ast/nodes/shared/callHasEffects.js @@ -1,5 +1,5 @@ +import isReference from 'is-reference'; import flatten from '../../utils/flatten.js'; -import isReference from '../../utils/isReference.js'; import pureFunctions from './pureFunctions.js'; import { UNKNOWN } from '../../values.js'; diff --git a/src/ast/utils/isReference.js b/src/ast/utils/isReference.js deleted file mode 100644 index c2e6c89..0000000 --- a/src/ast/utils/isReference.js +++ /dev/null @@ -1,28 +0,0 @@ -export default function isReference ( node, parent ) { - if ( node.type === 'MemberExpression' ) { - return !node.computed && isReference( node.object, node ); - } - - if ( node.type === 'Identifier' ) { - // the only time we could have an identifier node without a parent is - // if it's the entire body of a function without a block statement – - // i.e. an arrow function expression like `a => a` - if ( !parent ) return true; - - // TODO is this right? - if ( parent.type === 'MemberExpression' || parent.type === 'MethodDefinition' ) { - return parent.computed || node === parent.object; - } - - // disregard the `bar` in `{ bar: foo }`, but keep it in `{ [bar]: foo }` - if ( parent.type === 'Property' ) return parent.computed || node === parent.value; - - // disregard the `bar` in `class Foo { bar () {...} }` - if ( parent.type === 'MethodDefinition' ) return false; - - // disregard the `bar` in `export { foo as bar }` - if ( parent.type === 'ExportSpecifier' && node !== parent.local ) return; - - return true; - } -} diff --git a/test/function/deshadowed-destructured-parameter/_config.js b/test/function/deshadowed-destructured-parameter/_config.js new file mode 100644 index 0000000..886dd2f --- /dev/null +++ b/test/function/deshadowed-destructured-parameter/_config.js @@ -0,0 +1,4 @@ +module.exports = { + description: 'correctly deshadows destructured function parameters (#1008)', + buble: true +}; diff --git a/test/function/deshadowed-destructured-parameter/main.js b/test/function/deshadowed-destructured-parameter/main.js new file mode 100644 index 0000000..70dcf61 --- /dev/null +++ b/test/function/deshadowed-destructured-parameter/main.js @@ -0,0 +1,10 @@ +import { x } from './x.js'; + +let y; + +function foo ({ x = 42 }) { + y = x; +} + +foo({}); +assert.equal( y, 42 ); diff --git a/test/function/deshadowed-destructured-parameter/x.js b/test/function/deshadowed-destructured-parameter/x.js new file mode 100644 index 0000000..415fe38 --- /dev/null +++ b/test/function/deshadowed-destructured-parameter/x.js @@ -0,0 +1 @@ +export var x = 'whatever'; From fe16d37f8af7774854ca24f6beeba351a6eef968 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Wed, 21 Dec 2016 17:33:03 -0500 Subject: [PATCH 2/4] update some deps to get tests to pass --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 8e85a0b..75b5a21 100644 --- a/package.json +++ b/package.json @@ -48,12 +48,12 @@ "homepage": "https://github.com/rollup/rollup", "devDependencies": { "acorn": "^4.0.1", - "buble": "^0.12.5", + "buble": "^0.15.1", "chalk": "^1.1.3", "codecov.io": "^0.1.6", "console-group": "^0.3.1", - "eslint": "^2.13.0", - "eslint-plugin-import": "^1.14.0", + "eslint": "^3.12.2", + "eslint-plugin-import": "^2.2.0", "estree-walker": "^0.2.1", "is-reference": "^1.0.0", "istanbul": "^0.4.3", From 5430096c2643b0c8b8f73049d93d85c0fccdff3b Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Wed, 21 Dec 2016 17:39:46 -0500 Subject: [PATCH 3/4] downgrade eslint to a version that supports node 0.12 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 75b5a21..bac8334 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "chalk": "^1.1.3", "codecov.io": "^0.1.6", "console-group": "^0.3.1", - "eslint": "^3.12.2", + "eslint": "^2.13.1", "eslint-plugin-import": "^2.2.0", "estree-walker": "^0.2.1", "is-reference": "^1.0.0", From 21a5d0d0622ca9a1ea722479162939beff8c9a3f Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Wed, 21 Dec 2016 17:42:26 -0500 Subject: [PATCH 4/4] alright i give up. bye node 0.12 --- .travis.yml | 1 - appveyor.yml | 1 - 2 files changed, 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 83d1bca..956a81b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,6 @@ sudo: false language: node_js node_js: - - "0.12" - "4" - "6" env: diff --git a/appveyor.yml b/appveyor.yml index a4389cb..8ab6c7d 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -10,7 +10,6 @@ init: environment: matrix: # node.js - - nodejs_version: 0.12 - nodejs_version: 4 - nodejs_version: 6