no-shadowed-variable: fix bugs and add ignore options (palantir#3030)

[new-rule-option] `no-shadowed-variable` let's you optionally ignore certain kinds of declarations
[bugfix] `no-shadowed-variable` fixed false positive with parameter inside function decorator
Fixes: palantir#3000
[bugfix] `no-shadowed-variable` don't warn for shadowed type parameter on static class members
Fixes: palantir#3019

src/rules/noShadowedVariableRule.ts

@@ -16,7 +16,8 @@
  */
 
 import {
-    isBlockScopedVariableDeclarationList, isFunctionExpression, isFunctionWithBody, isScopeBoundary, isThisParameter, ScopeBoundary,
+    hasModifier, isBlockScopedVariableDeclarationList, isClassExpression, isFunctionExpression, isFunctionWithBody, isScopeBoundary,
+    isThisParameter, ScopeBoundary,
 } from "tsutils";
 import * as ts from "typescript";
 
@@ -28,9 +29,30 @@ export class Rule extends Lint.Rules.AbstractRule {
         ruleName: "no-shadowed-variable",
         description: "Disallows shadowing variable declarations.",
         rationale: "Shadowing a variable masks access to it and obscures to what value an identifier actually refers.",
-        optionsDescription: "Not configurable.",
-        options: null,
-        optionExamples: [true],
+        optionsDescription: Lint.Utils.dedent`
+            You can optionally pass an object to disable checking for certain kinds of declarations.
+            Possible keys are \`"class"\`, \`"enum"\`, \`"function"\`, \`"import"\`, \`"interface"\`, \`"namespace"\`, \`"typeAlias"\`
+            and \`"typeParameter"\`. Just set the value to \`false\` for the check you want to disable.
+            All checks default to \`true\`, i.e. are enabled by default.
+            Not that you cannot disable variables and parameters.
+        `,
+        options: {
+            type: "object",
+            properties: {
+                class: {type: "boolean"},
+                enum: {type: "boolean"},
+                function: {type: "boolean"},
+                import: {type: "boolean"},
+                interface: {type: "boolean"},
+                namespace: {type: "boolean"},
+                typeAlias: {type: "boolean"},
+                typeParameter: {type: "boolean"},
+            },
+        },
+        optionExamples: [
+            true,
+            [true, {class: true, enum: true, function: true, interface: false, namespace: true, typeAlias: false, typeParameter: false}],
+        ],
         type: "functionality",
         typescriptOnly: false,
     };
@@ -41,10 +63,29 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 
     public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
-        return this.applyWithWalker(new NoShadowedVariableWalker(sourceFile, this.ruleName, undefined));
+        return this.applyWithWalker(
+            new NoShadowedVariableWalker(sourceFile, this.ruleName, parseOptions(this.ruleArguments[0] as Partial<Options> | undefined)),
+        );
     }
 }
 
+type Kind = "class" | "import" | "interface" | "function" | "enum" | "namespace" | "typeParameter" | "typeAlias";
+type Options = Record<Kind, boolean>;
+
+function parseOptions(option: Partial<Options> | undefined): Options {
+    return {
+        class: true,
+        enum: true,
+        function: true,
+        import: true,
+        interface: true,
+        namespace: true,
+        typeAlias: true,
+        typeParameter: true,
+        ...option,
+    };
+}
+
 class Scope {
     public functionScope: Scope;
     public variables = new Map<string, ts.Identifier[]>();
@@ -67,78 +108,117 @@ class Scope {
     }
 }
 
-class NoShadowedVariableWalker extends Lint.AbstractWalker<void> {
+class NoShadowedVariableWalker extends Lint.AbstractWalker<Options> {
     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:
+            if ((this.options.function && isFunctionExpression(node) || this.options.class && isClassExpression(node)) &&
+                node.name !== undefined) {
+                /* special handling for named function and class 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);
+                if (isClassExpression(node)) {
+                    this.visitClassLikeDeclaration(node, functionScope, cb);
+                } else {
+                    ts.forEachChild(node, cb);
+                }
                 this.onScopeEnd(functionScope);
                 this.scope = functionScope;
                 this.onScopeEnd(parentScope);
                 this.scope = parentScope;
                 return;
             }
+
+            /* Visit decorators before entering a function scope.
+               In the AST decorators are children of the declaration they decorate, but we don't want to warn for the following code:
+               @decorator((param) => param)
+               function foo(param) {}
+            */
+            if (node.decorators !== undefined) {
+                for (const decorator of node.decorators) {
+                    ts.forEachChild(decorator, cb);
+                }
+            }
+
             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.Decorator:
+                    return; // handled above
                 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);
+                    if (this.options.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);
+                    if (this.options.function && (node as ts.FunctionDeclaration).name !== undefined) {
+                        parentScope.addVariable((node as ts.FunctionDeclaration).name!, false);
                     }
                     break;
+                case ts.SyntaxKind.ClassDeclaration:
+                    if (this.options.class && (node as ts.ClassDeclaration).name !== undefined) {
+                        parentScope.addVariable((node as ts.ClassDeclaration).name!);
+                    }
+                    // falls through
+                case ts.SyntaxKind.ClassExpression:
+                    this.visitClassLikeDeclaration(node as ts.ClassLikeDeclaration, parentScope, cb);
+                    this.onScopeEnd(parentScope);
+                    this.scope = parentScope;
+                    return;
                 case ts.SyntaxKind.TypeAliasDeclaration:
+                    if (this.options.typeAlias) {
+                        parentScope.addVariable((node as ts.TypeAliasDeclaration).name);
+                    }
+                    break;
                 case ts.SyntaxKind.EnumDeclaration:
+                    if (this.options.enum) {
+                        parentScope.addVariable((node as ts.EnumDeclaration).name);
+                    }
+                    break;
                 case ts.SyntaxKind.InterfaceDeclaration:
-                    parentScope.addVariable((node as ts.TypeAliasDeclaration | ts.EnumDeclaration | ts.InterfaceDeclaration).name);
+                    if (this.options.interface) {
+                        parentScope.addVariable((node as ts.InterfaceDeclaration).name);
+                    }
                     break;
                 case ts.SyntaxKind.Parameter:
-                    if (!isThisParameter(node as ts.ParameterDeclaration) && isFunctionWithBody(node.parent!)) {
+                    if (node.parent!.kind !== ts.SyntaxKind.IndexSignature &&
+                        !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 &&
+                    if (this.options.namespace &&
+                        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) {
+                    if (this.options.import && (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 (this.options.import) {
+                        this.scope.addVariable((node as ts.NamespaceImport | ts.ImportSpecifier | ts.ImportEqualsDeclaration).name, false);
+                    }
             }
             if (boundary !== ScopeBoundary.None) {
                 ts.forEachChild(node, cb);
@@ -153,6 +233,23 @@ class NoShadowedVariableWalker extends Lint.AbstractWalker<void> {
         this.onScopeEnd();
     }
 
+    private visitClassLikeDeclaration(declaration: ts.ClassLikeDeclaration, parentScope: Scope, cb: (node: ts.Node) => void) {
+        const currentScope = this.scope;
+        ts.forEachChild(declaration, (node) => {
+            if (!hasModifier(node.modifiers, ts.SyntaxKind.StaticKeyword)) {
+                return cb(node);
+            }
+            /* Don't treat static members as children of the class' scope. That avoid shadowed type parameter warnings on static members.
+               class C<T> {
+                   static method<T>() {}
+               }
+            */
+            this.scope = parentScope;
+            cb(node);
+            this.scope = currentScope;
+        });
+    }
+
     private handleVariableDeclarationList(node: ts.VariableDeclarationList) {
         const blockScoped = isBlockScopedVariableDeclarationList(node);
         for (const variable of node.declarations) {

test/rules/no-shadowed-variable/test.ts.lint → test/rules/no-shadowed-variable/default/test.ts.lint

@@ -331,4 +331,28 @@ function f(this): void {
     function inner(this): void {}
 }
 
-[err]: Shadowed name: '%s'
+@decorator((param) => param)
+function funxion(param) {}
+
+class Clazz<T> {
+    static method<T>() {}
+    static prop = <T>() => {}
+    method<T>() {}
+           ~ [err % ('T')]
+    prop = <T>() => {}
+            ~ [err % ('T')]
+    constructor() {
+        class Inner<T> {
+                    ~ [err % ('T')]
+            static method<T>() {}
+                          ~ [err % ('T')]
+            static prop = <T>() => {}
+                           ~ [err % ('T')]
+        }
+    }
+}
+
+// allow IndexSignature names
+function baz(param: {[baz: string]: string}) {}
+
+[err]: Shadowed name: '%s'

test/rules/no-shadowed-variable/ignore-class/test.ts.lint

@@ -0,0 +1,16 @@
+class C {
+    constructor() {
+        var C;
+        var D;
+            ~ [Shadowed name: 'D']
+        {
+            class C {
+                constructor() {
+                    var C;
+                        ~ [Shadowed name: 'C']
+                }
+            }
+        }
+    }
+}
+interface D {}

test/rules/no-shadowed-variable/ignore-class/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-shadowed-variable": [true, {"class": false}]
+  }
+}

test/rules/no-shadowed-variable/ignore-import/test.ts.lint

@@ -0,0 +1,13 @@
+import {diff} from "diff";
+import clear from "storage-helper";
+
+diff(original, current).forEach((diff) => {
+    console.log(diff);
+});
+
+{
+    (function clear() {
+        let clear = true;
+            ~~~~~ [Shadowed name: 'clear']
+    })();
+}

test/rules/no-shadowed-variable/ignore-import/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-shadowed-variable": [true, {"import": false}]
+  }
+}