Merge pull request palantir#440 from adidahiya/bugfix-var-keyword-style

Style improvements to no-var-keyword rule
moveleft · Jun 23, 2015 · 3df72a8 · 3df72a8
2 parents 3b3c56c + 785614a
commit 3df72a8
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 24 deletions.
diff --git a/lib/tslint.d.ts b/lib/tslint.d.ts
@@ -180,7 +180,8 @@ declare module Lint {
     function abstract(): string;
     function scanAllTokens(scanner: ts.Scanner, callback: (scanner: ts.Scanner) => void): void;
     function hasModifier(modifiers: ts.ModifiersArray, ...modifierKinds: ts.SyntaxKind[]): boolean;
-    function isBlockScopedVariable(node: ts.VariableDeclaration): boolean;
+    function isBlockScopedVariable(node: ts.VariableDeclaration | ts.VariableStatement): boolean;
+    function isBlockScopedBindingElement(node: ts.BindingElement): boolean;
 }
 declare module Lint {
     class BlockScopeAwareRuleWalker<T, U> extends ScopeAwareRuleWalker<T> {

diff --git a/src/language/utils.ts b/src/language/utils.ts
@@ -21,7 +21,7 @@ module Lint {
         const normalizedName = path.normalize(fileName).replace(/\\/g, "/");
         const compilerOptions = createCompilerOptions();
 
-        const compilerHost = {
+        const compilerHost: ts.CompilerHost = {
             getCanonicalFileName: (filename: string) => filename,
             getCurrentDirectory: () => "",
             getDefaultLibFileName: () => "lib.d.ts",
@@ -84,10 +84,38 @@ module Lint {
         });
     }
 
-    export function isBlockScopedVariable(node: ts.VariableDeclaration): boolean {
-        // determine if the appropriate bit in the parent (VariableDeclarationList) is set,
-        // which indicates this is a "let" or "const"
-        return (Math.floor(node.parent.flags / ts.NodeFlags.Let) % 2) === 1
-            || (Math.floor(node.parent.flags / ts.NodeFlags.Const) % 2) === 1;
+    /**
+     * Determines if the appropriate bit in the parent (VariableDeclarationList) is set,
+     * which indicates this is a "let" or "const".
+     */
+    export function isBlockScopedVariable(node: ts.VariableDeclaration | ts.VariableStatement): boolean {
+        const parentNode = (node.kind === ts.SyntaxKind.VariableDeclaration)
+            ? (<ts.VariableDeclaration> node).parent
+            : (<ts.VariableStatement> node).declarationList;
+
+        return isNodeFlagSet(parentNode, ts.NodeFlags.Let)
+            || isNodeFlagSet(parentNode, ts.NodeFlags.Const);
+    }
+
+    export function isBlockScopedBindingElement(node: ts.BindingElement): boolean {
+        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;
+            } else {
+                currentParent = currentParent.parent;
+            }
+        }
+        return isBlockScopedVariable(<ts.VariableDeclaration> currentParent);
+    }
+
+    /**
+     * Bitwise check for node flags.
+     */
+    function isNodeFlagSet(node: ts.Node, flagToCheck: ts.NodeFlags): boolean {
+        /* tslint:disable:no-bitwise */
+        return (node.flags & flagToCheck) !== 0;
+        /* tslint:enable:no-bitwise */
     }
 }
diff --git a/src/rules/noDuplicateVariableRule.ts b/src/rules/noDuplicateVariableRule.ts
@@ -34,7 +34,7 @@ class NoDuplicateVariableWalker extends Lint.BlockScopeAwareRuleWalker<{}, Scope
     public visitBindingElement(node: ts.BindingElement) {
         // TODO: handle node.dotdotToken?
         const isSingleVariable = node.name.kind === ts.SyntaxKind.Identifier;
-        const isBlockScoped = Lint.isBlockScopedVariable(<ts.VariableDeclaration> node.parent.parent);
+        const isBlockScoped = Lint.isBlockScopedBindingElement(node);
 
         if (isSingleVariable && !isBlockScoped) {
             this.handleSingleVariableIdentifier(<ts.Identifier> node.name);
@@ -83,7 +83,6 @@ class NoDuplicateVariableWalker extends Lint.BlockScopeAwareRuleWalker<{}, Scope
         const failureString = `${Rule.FAILURE_STRING}${ident.text}'`;
         this.addFailure(this.createFailure(ident.getStart(), ident.getWidth(), failureString));
     }
-
 }
 
 class ScopeInfo {

diff --git a/src/rules/noVarKeywordRule.ts b/src/rules/noVarKeywordRule.ts
@@ -14,8 +14,6 @@
  * limitations under the License.
  */
 
-///<reference path='../../lib/tslint.d.ts' />
-
 const OPTION_LEADING_UNDERSCORE = "no-var-keyword";
 
 export class Rule extends Lint.Rules.AbstractRule {
@@ -29,14 +27,9 @@ export class Rule extends Lint.Rules.AbstractRule {
 
 class NoVarKeywordWalker extends Lint.RuleWalker {
     public visitVariableStatement(node: ts.VariableStatement) {
-        if (!Lint.hasModifier(node.modifiers, ts.SyntaxKind.ExportKeyword)
-            && !Lint.hasModifier(node.modifiers, ts.SyntaxKind.DeclareKeyword)) {
-            const flags = node.declarationList.flags;
-            const declarationIsLet = (Math.floor(flags / ts.NodeFlags.Let) % 2) === 1;
-            const declarationIsConst = (Math.floor(flags / ts.NodeFlags.Const) % 2) === 1;
-            if (!declarationIsConst && !declarationIsLet) {
-                this.addFailure(this.createFailure(node.getStart(), "var".length, Rule.FAILURE_STRING));
-            }
+        if (!Lint.hasModifier(node.modifiers, ts.SyntaxKind.ExportKeyword, ts.SyntaxKind.DeclareKeyword)
+                && !Lint.isBlockScopedVariable(node)) {
+            this.addFailure(this.createFailure(node.getStart(), "var".length, Rule.FAILURE_STRING));
         }
 
         super.visitVariableStatement(node);

diff --git a/test/files/rules/no-duplicate-variable.test.ts b/test/files/rules/no-duplicate-variable.test.ts
@@ -151,4 +151,9 @@ function testDestructuring() {
 
     var [a2, [b2, c2]] = [1, [2, 3]];
     var [{a2, d2}] = [{a2: 1, d2: 4}]; // failure
+
+    function myFunc2([a, b]) {
+        var a; // not a failure; caught by no-shadowed-variable
+        return b;
+    }
 }
diff --git a/test/rules/noVarKeywordRuleTests.ts b/test/rules/noVarKeywordRuleTests.ts
@@ -19,12 +19,12 @@ describe("<no-var-keyword>", () => {
     const fileName = "rules/novarkeyword.test.ts";
 
     it("disallows use of creating variables with 'var'", () => {
-        const failure = Lint.Test.createFailuresOnFile(fileName, NoVarKeywordRule.FAILURE_STRING);
+        const createFailure = Lint.Test.createFailuresOnFile(fileName, NoVarKeywordRule.FAILURE_STRING);
         const expectedFailures = [
-            failure([1, 1], [1, 4]),
-            failure([4, 5], [4, 8]),
-            failure([7, 1], [7, 4]),
-            failure([10, 1], [10, 4]),
+            createFailure([1, 1], [1, 4]),
+            createFailure([4, 5], [4, 8]),
+            createFailure([7, 1], [7, 4]),
+            createFailure([10, 1], [10, 4]),
         ];
         const actualFailures = Lint.Test.applyRuleOnFile(fileName, NoVarKeywordRule);