Fix and refactor no-shadowed-variable (#1816)

palantir · Dec 17, 2016 · 87d2d0c · 87d2d0c
1 parent 2a9f3d7
commit 87d2d0c
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 33 deletions.
diff --git a/src/language/walker/blockScopeAwareRuleWalker.ts b/src/language/walker/blockScopeAwareRuleWalker.ts
@@ -76,20 +76,19 @@ export abstract class BlockScopeAwareRuleWalker<T, U> extends ScopeAwareRuleWalk
 
     private isBlockScopeBoundary(node: ts.Node): boolean {
         return super.isScopeBoundary(node)
+            || node.kind === ts.SyntaxKind.Block
             || node.kind === ts.SyntaxKind.DoStatement
             || node.kind === ts.SyntaxKind.WhileStatement
             || node.kind === ts.SyntaxKind.ForStatement
             || node.kind === ts.SyntaxKind.ForInStatement
             || node.kind === ts.SyntaxKind.ForOfStatement
             || node.kind === ts.SyntaxKind.WithStatement
             || node.kind === ts.SyntaxKind.SwitchStatement
-            || (node.parent != null
-                && (node.parent.kind === ts.SyntaxKind.TryStatement
-                    || node.parent.kind === ts.SyntaxKind.IfStatement)
-                )
-            || (node.kind === ts.SyntaxKind.Block && node.parent != null
-                && (node.parent.kind === ts.SyntaxKind.Block
-                    || node.parent.kind === ts.SyntaxKind.SourceFile)
-                );
+            || isParentKind(node, ts.SyntaxKind.TryStatement)
+            || isParentKind(node, ts.SyntaxKind.IfStatement);
     }
 }
+
+function isParentKind(node: ts.Node, kind: ts.SyntaxKind) {
+    return node.parent != null && node.parent.kind === kind;
+}
diff --git a/src/rules/noShadowedVariableRule.ts b/src/rules/noShadowedVariableRule.ts
@@ -61,7 +61,7 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
                 const isBlockScopedVariable = Lint.isBlockScopedVariable(variableDeclaration);
                 this.handleSingleVariableIdentifier(name, isBlockScopedVariable);
             } else {
-                this.handleSingleParameterIdentifier(name);
+                this.handleSingleVariableIdentifier(name, false);
             }
         }
 
@@ -98,7 +98,7 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
         const isSingleParameter = node.name.kind === ts.SyntaxKind.Identifier;
 
         if (isSingleParameter) {
-            this.handleSingleParameterIdentifier(<ts.Identifier> node.name);
+            this.handleSingleVariableIdentifier(<ts.Identifier> node.name, false);
         }
 
         super.visitParameterDeclaration(node);
@@ -120,38 +120,36 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
 
     private handleSingleVariableIdentifier(variableIdentifier: ts.Identifier, isBlockScoped: boolean) {
         const variableName = variableIdentifier.text;
-        const currentScope = this.getCurrentScope();
-        const currentBlockScope = this.getCurrentBlockScope();
 
-        // this var is shadowing if there's already a var of the same name in any available scope AND
-        // it is not in the current block (those are handled by the 'no-duplicate-variable' rule)
-        if (this.isVarInAnyScope(variableName) && currentBlockScope.varNames.indexOf(variableName) < 0) {
+        if (this.isVarInCurrentScope(variableName) && !this.inCurrentBlockScope(variableName)) {
+            // shadowing if there's already a `var` of the same name in the scope AND
+            // it's not in the current block (handled by the 'no-duplicate-variable' rule)
+            this.addFailureOnIdentifier(variableIdentifier);
+        } else if (this.inPreviousBlockScope(variableName)) {
+            // shadowing if there is a `var`, `let`, 'const`, or parameter in a previous block scope
             this.addFailureOnIdentifier(variableIdentifier);
         }
 
-        // regular vars should always be added to the scope; block-scoped vars should be added iff
-        // the current scope is same as current block scope
-        if (!isBlockScoped
-                || this.getCurrentBlockDepth() === 1
-                || this.getCurrentBlockDepth() === this.getCurrentDepth()) {
-            currentScope.varNames.push(variableName);
+        if (!isBlockScoped) {
+            // `var` variables go on the scope
+            this.getCurrentScope().variableNames.push(variableName);
         }
-        currentBlockScope.varNames.push(variableName);
+        // all variables go on block scope, including `var`
+        this.getCurrentBlockScope().variableNames.push(variableName);
     }
 
-    private handleSingleParameterIdentifier(variableIdentifier: ts.Identifier) {
-        // treat parameters as block-scoped variables
-        const variableName = variableIdentifier.text;
-        const currentScope = this.getCurrentScope();
+    private isVarInCurrentScope(varName: string) {
+        return this.getCurrentScope().variableNames.indexOf(varName) >= 0;
+    }
 
-        if (this.isVarInAnyScope(variableName)) {
-            this.addFailureOnIdentifier(variableIdentifier);
-        }
-        currentScope.varNames.push(variableName);
+    private inCurrentBlockScope(varName: string) {
+        return this.getCurrentBlockScope().variableNames.indexOf(varName) >= 0;
     }
 
-    private isVarInAnyScope(varName: string) {
-        return this.getAllScopes().some((scopeInfo) => scopeInfo.varNames.indexOf(varName) >= 0);
+    private inPreviousBlockScope(varName: string) {
+        return this.getAllBlockScopes().some((scopeInfo) => {
+            return scopeInfo !== this.getCurrentBlockScope() && scopeInfo.variableNames.indexOf(varName) >= 0 ;
+        });
     }
 
     private addFailureOnIdentifier(ident: ts.Identifier) {
@@ -161,5 +159,5 @@ class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<ScopeInfo,
 }
 
 class ScopeInfo {
-    public varNames: string[] = [];
+    public variableNames: string[] = [];
 }
diff --git a/test/rules/no-shadowed-variable/test.ts.lint b/test/rules/no-shadowed-variable/test.ts.lint
@@ -212,3 +212,8 @@ function creatorFunction(constructor: MyConstructor, filter: MyFunction): void {
     const myParameter = 1;
     console.log(myVariable);
 }
+
+for (let i = 0; i < 3; i++) {
+    let i = 34;
+        ~       [Shadowed variable: 'i']
+}