From 1c354babebd7eb851323669e0a0d7247a5083d6d Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Wed, 21 Dec 2016 17:04:41 -0500 Subject: [PATCH] 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';