Merge pull request #734 from lbesson/master

Fixes #727
palantir · Nov 24, 2015 · 73e8873 · 73e8873
2 parents 9be7b27 + 026634f
commit 73e8873
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 13 deletions.
diff --git a/src/language/utils.ts b/src/language/utils.ts
@@ -97,16 +97,21 @@ export function isBlockScopedVariable(node: ts.VariableDeclaration | ts.Variable
 }
 
 export function isBlockScopedBindingElement(node: ts.BindingElement): boolean {
+    const variableDeclaration = getBindingElementVariableDeclaration(node);
+    // if no variable declaration, it must be a function param, which is block scoped
+    return (variableDeclaration == null) || isBlockScopedVariable(variableDeclaration);
+}
+
+export function getBindingElementVariableDeclaration(node: ts.BindingElement): ts.VariableDeclaration {
     let currentParent = node.parent;
     while (currentParent.kind !== ts.SyntaxKind.VariableDeclaration) {
         if (currentParent.parent == null) {
-            // if we didn't encounter a VariableDeclaration, this must be a function parameter, which is block scoped
-            return true;
+            return null; // function parameter, no variable declaration
         } else {
             currentParent = currentParent.parent;
         }
     }
-    return isBlockScopedVariable(<ts.VariableDeclaration> currentParent);
+    return <ts.VariableDeclaration> currentParent;
 }
 
 /**

diff --git a/src/rules/noShadowedVariableRule.ts b/src/rules/noShadowedVariableRule.ts
@@ -36,10 +36,15 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
 
     public visitBindingElement(node: ts.BindingElement) {
         const isSingleVariable = node.name.kind === ts.SyntaxKind.Identifier;
-        const isBlockScoped = Lint.isBlockScopedBindingElement(node);
+        const variableDeclaration = Lint.getBindingElementVariableDeclaration(node);
 
         if (isSingleVariable) {
-            this.handleSingleVariableIdentifier(<ts.Identifier> node.name, isBlockScoped);
+            if (variableDeclaration) {
+                const isBlockScopedVariable = Lint.isBlockScopedVariable(variableDeclaration);
+                this.handleSingleVariableIdentifier(<ts.Identifier> node.name, isBlockScopedVariable);
+            } else {
+                this.handleSingleParameterIdentifier(<ts.Identifier> node.name);
+            }
         }
 
         super.visitBindingElement(node);
@@ -64,15 +69,11 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
     }
 
     public visitParameterDeclaration(node: ts.ParameterDeclaration) {
-        // treat parameters as block-scoped variables
-        const variableIdentifier = <ts.Identifier> node.name;
-        const variableName = variableIdentifier.text;
-        const currentScope = this.getCurrentScope();
+        const isSingleParameter = node.name.kind === ts.SyntaxKind.Identifier;
 
-        if (this.isVarInAnyScope(variableName)) {
-            this.addFailureOnIdentifier(variableIdentifier);
+        if (isSingleParameter) {
+            this.handleSingleParameterIdentifier(<ts.Identifier> node.name);
         }
-        currentScope.varNames.push(variableName);
 
         super.visitParameterDeclaration(node);
     }
@@ -112,6 +113,17 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
         currentBlockScope.varNames.push(variableName);
     }
 
+    private handleSingleParameterIdentifier(variableIdentifier: ts.Identifier) {
+        // treat parameters as block-scoped variables
+        const variableName = variableIdentifier.text;
+        const currentScope = this.getCurrentScope();
+
+        if (this.isVarInAnyScope(variableName)) {
+            this.addFailureOnIdentifier(variableIdentifier);
+        }
+        currentScope.varNames.push(variableName);
+    }
+
     private isVarInAnyScope(varName: string) {
         return this.getAllScopes().some((scopeInfo) => scopeInfo.varNames.indexOf(varName) >= 0);
     }

diff --git a/test/files/rules/no-shadowed-variable.test.ts b/test/files/rules/no-shadowed-variable.test.ts
@@ -114,3 +114,17 @@ interface FalsePositive4<T, TResult> {
     (parameters: T, runSynchonous: boolean): TResult;
     (parameters: T, callback: (error: Error, result: TResult) => void): void;
 }
+
+let p = 1;
+function testParameterDestructuring(
+    { pos: p }: FalsePositive3, // failure
+    { pos: q, specular: r }: FalsePositive3,
+    { pos }: FalsePositive3
+) {
+    p = 2;
+    var q = 1; // failure
+
+    function someInnerFunc() {
+        let pos = 3; // failure
+    }
+}
diff --git a/test/rules/noShadowedVariableRuleTests.ts b/test/rules/noShadowedVariableRuleTests.ts
@@ -41,7 +41,10 @@ describe("<no-shadowed-variable>", () => {
             Lint.Test.createFailure(fileName, [90, 16], [90, 17], NoShadowedVariableRule.FAILURE_STRING + "z'"),
             Lint.Test.createFailure(fileName, [94, 15], [94, 16], NoShadowedVariableRule.FAILURE_STRING + "x'"),
             Lint.Test.createFailure(fileName, [95, 15], [95, 16], NoShadowedVariableRule.FAILURE_STRING + "y'"),
-            Lint.Test.createFailure(fileName, [97, 17], [97, 18], NoShadowedVariableRule.FAILURE_STRING + "z'")
+            Lint.Test.createFailure(fileName, [97, 17], [97, 18], NoShadowedVariableRule.FAILURE_STRING + "z'"),
+            Lint.Test.createFailure(fileName, [120, 12], [120, 13], NoShadowedVariableRule.FAILURE_STRING + "p'"),
+            Lint.Test.createFailure(fileName, [125, 9], [125, 10], NoShadowedVariableRule.FAILURE_STRING + "q'"),
+            Lint.Test.createFailure(fileName, [128, 13], [128, 16], NoShadowedVariableRule.FAILURE_STRING + "pos'")
         ];
         const actualFailures = Lint.Test.applyRuleOnFile(fileName, NoShadowedVariableRule);