no-shadowed-variable: Rewrite and lint for other shadowed names

src/rules/arrayTypeRule.ts

@@ -97,8 +97,7 @@ function walk(ctx: Lint.WalkContext<Option>): void {
         const failureString = option === "array" ? Rule.FAILURE_STRING_ARRAY : Rule.FAILURE_STRING_ARRAY_SIMPLE;
         if (typeArguments === undefined || typeArguments.length === 0) {
             // Create an 'any' array
-            const fix = Lint.Replacement.replaceFromTo(node.getStart(), node.getEnd(), "any[]");
-            ctx.addFailureAtNode(node, failureString, fix);
+            ctx.addFailureAtNode(node, failureString, Lint.Replacement.replaceFromTo(node.getStart(), node.getEnd(), "any[]"));
             return;
         }
 
@@ -108,13 +107,12 @@ function walk(ctx: Lint.WalkContext<Option>): void {
 
         const type = typeArguments[0];
         const parens = typeNeedsParentheses(type);
-        const fix = [
+        ctx.addFailureAtNode(node, failureString, [
             // Delete 'Array<'
             Lint.Replacement.replaceFromTo(node.getStart(), type.getStart(), parens ? "(" : ""),
             // Delete '>' and replace with '[]
             Lint.Replacement.replaceFromTo(type.getEnd(), node.getEnd(), parens ? ")[]" : "[]"),
-        ];
-        ctx.addFailureAtNode(node, failureString, fix);
+        ]);
     }
 }
 

src/rules/memberAccessRule.ts

@@ -129,12 +129,12 @@ function walk(ctx: Lint.WalkContext<Options>) {
                 ? getChildOfKind(node, ts.SyntaxKind.ConstructorKeyword, ctx.sourceFile)!
                 : node.name !== undefined ? node.name : node;
             const memberName = node.name !== undefined && node.name.kind === ts.SyntaxKind.Identifier ? node.name.text : undefined;
-            ctx.addFailureAtNode(nameNode, Rule.FAILURE_STRING_FACTORY(memberType(node), memberName));
+            ctx.addFailureAtNode(nameNode, Rule.FAILURE_STRING_FACTORY(typeToString(node), memberName));
         }
     }
 }
 
-function memberType(node: ts.ClassElement): string {
+function typeToString(node: ts.ClassElement): string {
     switch (node.kind) {
         case ts.SyntaxKind.MethodDeclaration:
             return "class method";

src/rules/noShadowedVariableRule.ts

@@ -15,9 +15,9 @@
  * limitations under the License.
  */
 
-// tslint:disable deprecation
-// (https://github.com/palantir/tslint/pull/2598)
-
+import {
+    isBlockScopedVariableDeclarationList, isFunctionExpression, isFunctionWithBody, isScopeBoundary, isThisParameter, ScopeBoundary,
+} from "tsutils";
 import * as ts from "typescript";
 
 import * as Lint from "../index";
@@ -37,121 +37,165 @@ export class Rule extends Lint.Rules.AbstractRule {
     /* tslint:enable:object-literal-sort-keys */
 
     public static FAILURE_STRING_FACTORY(name: string) {
-        return `Shadowed variable: '${name}'`;
+        return `Shadowed name: '${name}'`;
     }
 
     public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
-        return this.applyWithWalker(new NoShadowedVariableWalker(sourceFile, this.getOptions()));
+        return this.applyWithWalker(new NoShadowedVariableWalker(sourceFile, this.ruleName, undefined));
     }
 }
 
-class NoShadowedVariableWalker extends Lint.BlockScopeAwareRuleWalker<Set<string>, Set<string>> {
-    public createScope() {
-        return new Set();
-    }
-
-    public createBlockScope() {
-        return new Set();
-    }
-
-    public visitBindingElement(node: ts.BindingElement) {
-        const isSingleVariable = node.name.kind === ts.SyntaxKind.Identifier;
-        if (isSingleVariable) {
-            const name = node.name as ts.Identifier;
-            const variableDeclaration = Lint.getBindingElementVariableDeclaration(node);
-            const isBlockScopedVariable = variableDeclaration !== null && Lint.isBlockScopedVariable(variableDeclaration);
-            this.handleSingleVariableIdentifier(name, isBlockScopedVariable);
+class Scope {
+    public functionScope: Scope;
+    public variables = new Map<string, ts.Identifier[]>();
+    public variablesSeen = new Map<string, ts.Identifier[]>();
+    public reassigned = new Set<string>();
+    constructor(functionScope?: Scope) {
+        // if no functionScope is provided we are in the process of creating a new function scope, which for consistency links to itself
+        this.functionScope = functionScope !== undefined ? functionScope : this;
+    }
+
+    public addVariable(identifier: ts.Identifier, blockScoped = true) {
+        // block scoped variables go to the block scope, function scoped variables to the containing function scope
+        const scope = blockScoped ? this : this.functionScope;
+        const list = scope.variables.get(identifier.text);
+        if (list === undefined) {
+            scope.variables.set(identifier.text, [identifier]);
+        } else {
+            list.push(identifier);
         }
-
-        super.visitBindingElement(node);
-    }
-
-    public visitCatchClause(node: ts.CatchClause) {
-        // don't visit the catch clause variable declaration, just visit the block
-        // the catch clause variable declaration has its own special scoping rules
-        this.visitBlock(node.block);
-    }
-
-    public visitCallSignature(_node: ts.SignatureDeclaration) {
-        // don't call super, we don't need to check parameter names in call signatures
     }
+}
 
-    public visitFunctionType(_node: ts.FunctionOrConstructorTypeNode) {
-        // don't call super, we don't need to check names in function types
-    }
-
-    public visitConstructorType(_node: ts.FunctionOrConstructorTypeNode) {
-        // don't call super, we don't need to check names in constructor types
-    }
-
-    public visitIndexSignatureDeclaration(_node: ts.SignatureDeclaration) {
-        // don't call super, we don't want to walk index signatures
-    }
-
-    public visitMethodSignature(_node: ts.SignatureDeclaration) {
-        // don't call super, we don't want to walk method signatures either
-    }
-
-    public visitParameterDeclaration(node: ts.ParameterDeclaration) {
-        const isSingleParameter = node.name.kind === ts.SyntaxKind.Identifier;
-
-        if (isSingleParameter) {
-            this.handleSingleVariableIdentifier(node.name as ts.Identifier, false);
-        }
-
-        super.visitParameterDeclaration(node);
-    }
-
-    public visitTypeLiteral(_node: ts.TypeLiteralNode) {
-        // don't call super, we don't want to walk the inside of type nodes
-    }
-
-    public visitVariableDeclaration(node: ts.VariableDeclaration) {
-        const isSingleVariable = node.name.kind === ts.SyntaxKind.Identifier;
-
-        if (isSingleVariable) {
-            this.handleSingleVariableIdentifier(node.name as ts.Identifier, Lint.isBlockScopedVariable(node));
+class NoShadowedVariableWalker extends Lint.AbstractWalker<void> {
+    private scope: Scope;
+    public walk(sourceFile: ts.SourceFile) {
+        this.scope = new Scope();
+
+        const cb = (node: ts.Node): void => {
+            const parentScope = this.scope;
+            if (isFunctionExpression(node) && node.name !== undefined) {
+                /* special handling for named function expressions:
+                   technically the name of the function is only visible inside of it,
+                   but variables with the same name declared inside don't cause compiler errors.
+                   Therefore we add an additional function scope only for the function name to avoid merging with other declarations */
+                const functionScope = new Scope();
+                functionScope.addVariable(node.name, false);
+                this.scope = new Scope();
+                ts.forEachChild(node, cb);
+                this.onScopeEnd(functionScope);
+                this.scope = functionScope;
+                this.onScopeEnd(parentScope);
+                this.scope = parentScope;
+                return;
+            }
+            const boundary = isScopeBoundary(node);
+            if (boundary === ScopeBoundary.Block) {
+                this.scope = new Scope(parentScope.functionScope);
+            } else if (boundary === ScopeBoundary.Function) {
+                this.scope = new Scope();
+            }
+            switch (node.kind) {
+                case ts.SyntaxKind.VariableDeclarationList:
+                    this.handleVariableDeclarationList(node as ts.VariableDeclarationList);
+                    break;
+                case ts.SyntaxKind.ClassExpression:
+                    if ((node as ts.ClassExpression).name !== undefined) {
+                        this.scope.addVariable((node as ts.ClassExpression).name!);
+                    }
+                    break;
+                case ts.SyntaxKind.TypeParameter:
+                    this.scope.addVariable((node as ts.TypeParameterDeclaration).name);
+                    break;
+                case ts.SyntaxKind.FunctionDeclaration:
+                case ts.SyntaxKind.ClassDeclaration:
+                    if ((node as ts.FunctionDeclaration | ts.ClassDeclaration).name !== undefined) {
+                        parentScope.addVariable((node as ts.FunctionDeclaration | ts.ClassDeclaration).name!,
+                                                node.kind !== ts.SyntaxKind.FunctionDeclaration);
+                    }
+                    break;
+                case ts.SyntaxKind.TypeAliasDeclaration:
+                case ts.SyntaxKind.EnumDeclaration:
+                case ts.SyntaxKind.InterfaceDeclaration:
+                    parentScope.addVariable((node as ts.TypeAliasDeclaration | ts.EnumDeclaration | ts.InterfaceDeclaration).name);
+                    break;
+                case ts.SyntaxKind.Parameter:
+                    if (!isThisParameter(node as ts.ParameterDeclaration) && isFunctionWithBody(node.parent!)) {
+                        this.handleBindingName((node as ts.ParameterDeclaration).name, false);
+                    }
+                    break;
+                case ts.SyntaxKind.ModuleDeclaration:
+                    if (node.parent!.kind !== ts.SyntaxKind.ModuleDeclaration &&
+                        (node as ts.ModuleDeclaration).name.kind === ts.SyntaxKind.Identifier) {
+                        parentScope.addVariable((node as ts.NamespaceDeclaration).name, false);
+                    }
+                    break;
+                case ts.SyntaxKind.ImportClause:
+                    if ((node as ts.ImportClause).name !== undefined) {
+                        this.scope.addVariable((node as ts.ImportClause).name!, false);
+                    }
+                    break;
+                case ts.SyntaxKind.NamespaceImport:
+                case ts.SyntaxKind.ImportSpecifier:
+                case ts.SyntaxKind.ImportEqualsDeclaration:
+                    this.scope.addVariable((node as ts.NamespaceImport | ts.ImportSpecifier | ts.ImportEqualsDeclaration).name, false);
+            }
+            if (boundary !== ScopeBoundary.None) {
+                ts.forEachChild(node, cb);
+                this.onScopeEnd(parentScope);
+                this.scope = parentScope;
+            } else {
+                return ts.forEachChild(node, cb);
+            }
+        };
+
+        ts.forEachChild(sourceFile, cb);
+        this.onScopeEnd();
+    }
+
+    private handleVariableDeclarationList(node: ts.VariableDeclarationList) {
+        const blockScoped = isBlockScopedVariableDeclarationList(node);
+        for (const variable of node.declarations) {
+            this.handleBindingName(variable.name, blockScoped);
         }
-
-        super.visitVariableDeclaration(node);
     }
 
-    private handleSingleVariableIdentifier(variableIdentifier: ts.Identifier, isBlockScoped: boolean) {
-        const variableName = variableIdentifier.text;
-
-        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);
-        }
-
-        if (!isBlockScoped) {
-            // `var` variables go on the scope
-            this.getCurrentScope().add(variableName);
+    private handleBindingName(node: ts.BindingName, blockScoped: boolean) {
+        if (node.kind === ts.SyntaxKind.Identifier) {
+            this.scope.addVariable(node, blockScoped);
+        } else {
+            for (const element of node.elements) {
+                if (element.kind !== ts.SyntaxKind.OmittedExpression) {
+                    this.handleBindingName(element.name, blockScoped);
+                }
+            }
         }
-        // all variables go on block scope, including `var`
-        this.getCurrentBlockScope().add(variableName);
-    }
-
-    private isVarInCurrentScope(varName: string) {
-        return this.getCurrentScope().has(varName);
     }
 
-    private inCurrentBlockScope(varName: string) {
-        return this.getCurrentBlockScope().has(varName);
-    }
-
-    private inPreviousBlockScope(varName: string) {
-        return this.getAllBlockScopes().some((scopeInfo) => {
-            return scopeInfo !== this.getCurrentBlockScope() && scopeInfo.has(varName);
+    private onScopeEnd(parent?: Scope) {
+        const {variables, variablesSeen} = this.scope;
+        variablesSeen.forEach((identifiers, name) => {
+            if (variables.has(name)) {
+                for (const identifier of identifiers) {
+                    this.addFailureAtNode(identifier, Rule.FAILURE_STRING_FACTORY(name));
+                }
+            } else if (parent !== undefined) {
+                addToList(parent.variablesSeen, name, identifiers);
+            }
         });
+        if (parent !== undefined) {
+            variables.forEach((identifiers, name) => {
+                addToList(parent.variablesSeen, name, identifiers);
+            });
+        }
     }
+}
 
-    private addFailureOnIdentifier(ident: ts.Identifier) {
-        const failureString = Rule.FAILURE_STRING_FACTORY(ident.text);
-        this.addFailureAtNode(ident, failureString);
+function addToList(map: Map<string, ts.Identifier[]>, name: string, identifiers: ts.Identifier[]) {
+    const list = map.get(name);
+    if (list === undefined) {
+        map.set(name, identifiers);
+    } else {
+        list.push(...identifiers);
     }
 }

src/rules/strictBooleanExpressionsRule.ts

@@ -422,12 +422,12 @@ function showLocation(n: Location): string {
     }
 }
 
-function showFailure(location: Location, ty: TypeFailure, isUnionType: boolean, options: Options): string {
+function showFailure(location: Location, ty: TypeFailure, unionType: boolean, options: Options): string {
     const expectedTypes = showExpectedTypes(options);
     const expected = expectedTypes.length === 1
         ? `Only ${expectedTypes[0]}s are allowed`
         : `Allowed types are ${stringOr(expectedTypes)}`;
-    const tyFail = showTypeFailure(ty, isUnionType, options.strictNullChecks);
+    const tyFail = showTypeFailure(ty, unionType, options.strictNullChecks);
     return `This type is not allowed in the ${showLocation(location)} because it ${tyFail}. ${expected}.`;
 }
 
@@ -441,8 +441,8 @@ function showExpectedTypes(options: Options): string[] {
     return parts;
 }
 
-function showTypeFailure(ty: TypeFailure, isUnionType: boolean, strictNullChecks: boolean) {
-    const is = isUnionType ? "could be" : "is";
+function showTypeFailure(ty: TypeFailure, unionType: boolean, strictNullChecks: boolean) {
+    const is = unionType ? "could be" : "is";
     switch (ty) {
         case TypeFailure.AlwaysTruthy:
             return strictNullChecks

src/rules/whitespaceRule.ts

@@ -144,7 +144,7 @@ function walk(ctx: Lint.WalkContext<Options>) {
                     // an import clause can have _both_ named bindings and a name (the latter for the default import)
                     // but the named bindings always come last, so we only need to check that for whitespace
                     let position: number | undefined;
-                    const { name, namedBindings } = importClause;
+                    const { namedBindings } = importClause;
                     if (namedBindings !== undefined) {
                         if (namedBindings.kind !== ts.SyntaxKind.NamespaceImport) {
                             namedBindings.elements.forEach((element, idx, arr) => {
@@ -162,8 +162,8 @@ function walk(ctx: Lint.WalkContext<Options>) {
                             });
                         }
                         position = namedBindings.getEnd();
-                    } else if (name !== undefined) {
-                        position = name.getEnd();
+                    } else if (importClause.name !== undefined) {
+                        position = importClause.name.getEnd();
                     }
 
                     if (position !== undefined) {

src/test.ts

@@ -117,7 +117,7 @@ export function runTest(testDirectory: string, rulesDirectory?: string | string[
                 getCanonicalFileName: (filename) => filename,
                 getCurrentDirectory: () => process.cwd(),
                 getDefaultLibFileName: () => ts.getDefaultLibFileName(compilerOptions),
-                getDirectories: (path) => fs.readdirSync(path),
+                getDirectories: (dir) => fs.readdirSync(dir),
                 getNewLine: () => "\n",
                 getSourceFile(filenameToGet) {
                     const target = compilerOptions.target === undefined ? ts.ScriptTarget.ES5 : compilerOptions.target;
@@ -223,8 +223,8 @@ export function consoleTestResultHandler(testResult: TestResult): boolean {
         } else {
             const markupDiffResults = diff.diffLines(results.markupFromMarkup, results.markupFromLinter);
             const fixesDiffResults = diff.diffLines(results.fixesFromLinter, results.fixesFromMarkup);
-            const didMarkupTestPass = !markupDiffResults.some((diff) => diff.added === true || diff.removed === true);
-            const didFixesTestPass = !fixesDiffResults.some((diff) => diff.added === true || diff.removed === true);
+            const didMarkupTestPass = !markupDiffResults.some((hunk) => hunk.added === true || hunk.removed === true);
+            const didFixesTestPass = !fixesDiffResults.some((hunk) => hunk.added === true || hunk.removed === true);
 
             if (didMarkupTestPass && didFixesTestPass) {
                 console.log(colors.green(" Passed"));