Fix things that walk tokens to properly handle templates, etc.

Fixes palantir#357, palantir#349, palantir#332, and possibly a couple others I missed
mitchellwills · Apr 13, 2015 · 85b1b02 · 85b1b02
1 parent 55391e8
commit 85b1b02
Show file tree

Hide file tree

Showing 14 changed files with 141 additions and 73 deletions.
diff --git a/lib/tslint.d.ts b/lib/tslint.d.ts
@@ -164,7 +164,19 @@ declare module Lint {
     function findFormatter(name: string, formattersDirectory?: string): any;
 }
 declare module Lint {
-    class EnableDisableRulesWalker extends Lint.RuleWalker {
+    class SkipableTokenAwareRuleWalker extends RuleWalker {
+        protected tokensToSkipStartEndMap: {
+            [start: number]: number;
+        };
+        constructor(sourceFile: ts.SourceFile, options: Lint.IOptions);
+        protected visitRegularExpressionLiteral(node: ts.Node): void;
+        protected visitIdentifier(node: ts.Identifier): void;
+        protected visitTemplateExpression(node: ts.TemplateExpression): void;
+        private addTokenToSkipFromNode(node);
+    }
+}
+declare module Lint {
+    class EnableDisableRulesWalker extends Lint.SkipableTokenAwareRuleWalker {
         enableDisableRuleMap: {
             [rulename: string]: Lint.IEnableDisablePosition[];
         };

diff --git a/src/enableDisableRules.ts b/src/enableDisableRules.ts
@@ -14,21 +14,29 @@
  * limitations under the License.
 */
 
-/// <reference path='language/walker/ruleWalker.ts'/>
+/// <reference path='language/walker/skipableTokenAwareRuleWalker.ts'/>
 
 module Lint {
-    export class EnableDisableRulesWalker extends Lint.RuleWalker {
+    export class EnableDisableRulesWalker extends Lint.SkipableTokenAwareRuleWalker {
         public enableDisableRuleMap: {[rulename: string]: Lint.IEnableDisablePosition[]} = {};
 
         public visitSourceFile(node: ts.SourceFile): void {
+            super.visitSourceFile(node);
             Lint.scanAllTokens(ts.createScanner(ts.ScriptTarget.ES5, false, node.text), (scanner: ts.Scanner) => {
+                var startPos = scanner.getStartPos();
+                if (this.tokensToSkipStartEndMap[startPos] != null) {
+                    // tokens to skip are places where the scanner gets confused about what the token is, without the proper context
+                    // (specifically, regex, identifiers, and templates). So skip those tokens.
+                    scanner.setTextPos(this.tokensToSkipStartEndMap[startPos]);
+                    return;
+                }
+
                 if (scanner.getToken() === ts.SyntaxKind.MultiLineCommentTrivia) {
                     var commentText = scanner.getTokenText();
                     var startPosition = scanner.getTokenPos();
                     this.handlePossibleTslintSwitch(commentText, startPosition);
                 }
             });
-            // no need to call super to visit the rest of the nodes, so don't call super here
         }
 
         private handlePossibleTslintSwitch(commentText: string, startingPosition: number) {

diff --git a/src/language/walker/skipableTokenAwareRuleWalker.ts b/src/language/walker/skipableTokenAwareRuleWalker.ts
@@ -0,0 +1,50 @@
+/*
+ * Copyright 2015 Palantir Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/// <reference path='ruleWalker.ts'/>
+
+module Lint {
+    export class SkipableTokenAwareRuleWalker extends RuleWalker {
+        protected tokensToSkipStartEndMap: {[start: number]: number};
+
+        constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) {
+            super(sourceFile, options);
+            this.tokensToSkipStartEndMap = {};
+        }
+
+        protected visitRegularExpressionLiteral(node: ts.Node) {
+            this.addTokenToSkipFromNode(node);
+            super.visitRegularExpressionLiteral(node);
+        }
+
+        protected visitIdentifier(node: ts.Identifier) {
+            this.addTokenToSkipFromNode(node);
+            super.visitIdentifier(node);
+        }
+
+        protected visitTemplateExpression(node: ts.TemplateExpression) {
+            this.addTokenToSkipFromNode(node);
+            super.visitTemplateExpression(node);
+        }
+
+        private addTokenToSkipFromNode(node: ts.Node) {
+            if (node.getStart() < node.getEnd()) {
+                // only add to the map nodes whose end comes after their start, to prevent infinite loops
+                this.tokensToSkipStartEndMap[node.getStart()] = node.getEnd();
+            }
+        }
+    }
+}
diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts
@@ -12,7 +12,7 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
-*/
+ */
 
 var OPTION_SPACE = "check-space";
 var OPTION_LOWERCASE = "check-lowercase";
@@ -28,14 +28,7 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 }
 
-class CommentWalker extends Lint.RuleWalker {
-    private tokensToSkipStartEndMap: {[start: number]: number};
-
-    constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) {
-        super(sourceFile, options);
-        this.tokensToSkipStartEndMap = {};
-    }
-
+class CommentWalker extends Lint.SkipableTokenAwareRuleWalker {
     public visitSourceFile(node: ts.SourceFile): void {
         super.visitSourceFile(node);
         Lint.scanAllTokens(ts.createScanner(ts.ScriptTarget.ES5, false, node.text), (scanner: ts.Scanner) => {
@@ -73,28 +66,6 @@ class CommentWalker extends Lint.RuleWalker {
         });
     }
 
-    public visitRegularExpressionLiteral(node: ts.Node) {
-        this.addTokenToSkipFromNode(node);
-        super.visitRegularExpressionLiteral(node);
-    }
-
-    public visitIdentifier(node: ts.Identifier) {
-        this.addTokenToSkipFromNode(node);
-        super.visitIdentifier(node);
-    }
-
-    public visitTemplateExpression(node: ts.TemplateExpression) {
-        this.addTokenToSkipFromNode(node);
-        super.visitTemplateExpression(node);
-    }
-
-    public addTokenToSkipFromNode(node: ts.Node) {
-        if (node.getStart() < node.getEnd()) {
-            // only add to the map nodes whose end comes after their start, to prevent infinite loops
-            this.tokensToSkipStartEndMap[node.getStart()] = node.getEnd();
-        }
-    }
-
     private startsWithSpace(commentText: string): boolean {
         if (commentText.length <= 2) {
             return true; // comment is "//"? Technically not a violation.

diff --git a/src/rules/jsdocFormatRule.ts b/src/rules/jsdocFormatRule.ts
@@ -23,16 +23,24 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 }
 
-class JsdocWalker extends Lint.RuleWalker {
+class JsdocWalker extends Lint.SkipableTokenAwareRuleWalker {
     public visitSourceFile(node: ts.SourceFile): void {
+        super.visitSourceFile(node);
         Lint.scanAllTokens(ts.createScanner(ts.ScriptTarget.ES5, false, node.text), (scanner: ts.Scanner) => {
+            var startPos = scanner.getStartPos();
+            if (this.tokensToSkipStartEndMap[startPos] != null) {
+                // tokens to skip are places where the scanner gets confused about what the token is, without the proper context
+                // (specifically, regex, identifiers, and templates). So skip those tokens.
+                scanner.setTextPos(this.tokensToSkipStartEndMap[startPos]);
+                return;
+            }
+
             if (scanner.getToken() === ts.SyntaxKind.MultiLineCommentTrivia) {
                 var commentText = scanner.getTokenText();
                 var startPosition = scanner.getTokenPos();
                 this.findFailuresForJsdocComment(commentText, startPosition, node);
             }
         });
-        // no need to call super to visit the rest of the nodes, so don't call super here
     }
 
     private findFailuresForJsdocComment(commentText: string, startingPosition: number, sourceFile: ts.SourceFile) {

diff --git a/src/rules/noConsecutiveBlankLinesRule.ts b/src/rules/noConsecutiveBlankLinesRule.ts
@@ -22,11 +22,21 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 }
 
-class BlankLinesWalker extends Lint.RuleWalker {
+class BlankLinesWalker extends Lint.SkipableTokenAwareRuleWalker {
     public visitSourceFile(node: ts.SourceFile): void {
+        super.visitSourceFile(node);
         // starting with 1 to cover the case where the file starts with two blank lines
         var newLinesInARowSeenSoFar = 1;
         Lint.scanAllTokens(ts.createScanner(ts.ScriptTarget.ES5, false, node.text), (scanner: ts.Scanner) => {
+            var startPos = scanner.getStartPos();
+            if (this.tokensToSkipStartEndMap[startPos] != null) {
+                // tokens to skip are places where the scanner gets confused about what the token is, without the proper context
+                // (specifically, regex, identifiers, and templates). So skip those tokens.
+                scanner.setTextPos(this.tokensToSkipStartEndMap[startPos]);
+                newLinesInARowSeenSoFar = 0;
+                return;
+            }
+
             if (scanner.getToken() === ts.SyntaxKind.NewLineTrivia) {
                 newLinesInARowSeenSoFar += 1;
                 if (newLinesInARowSeenSoFar >= 3) {
@@ -37,6 +47,5 @@ class BlankLinesWalker extends Lint.RuleWalker {
                 newLinesInARowSeenSoFar = 0;
             }
         });
-        // no need to call super to visit the rest of the nodes, so don't call super here
     }
 }
diff --git a/src/rules/noTrailingWhitespaceRule.ts b/src/rules/noTrailingWhitespaceRule.ts
@@ -22,11 +22,21 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 }
 
-class NoTrailingWhitespaceWalker extends Lint.RuleWalker {
+class NoTrailingWhitespaceWalker extends Lint.SkipableTokenAwareRuleWalker {
     public visitSourceFile(node: ts.SourceFile): void {
+        super.visitSourceFile(node);
         var lastSeenWasWhitespace = false;
         var lastSeenWhitespacePosition = 0;
         Lint.scanAllTokens(ts.createScanner(ts.ScriptTarget.ES5, false, node.text), (scanner: ts.Scanner) => {
+            var startPos = scanner.getStartPos();
+            if (this.tokensToSkipStartEndMap[startPos] != null) {
+                // tokens to skip are places where the scanner gets confused about what the token is, without the proper context
+                // (specifically, regex, identifiers, and templates). So skip those tokens.
+                scanner.setTextPos(this.tokensToSkipStartEndMap[startPos]);
+                lastSeenWasWhitespace = false;
+                return;
+            }
+
             if (scanner.getToken() === ts.SyntaxKind.NewLineTrivia) {
                 if (lastSeenWasWhitespace) {
                     var width = scanner.getStartPos() - lastSeenWhitespacePosition;
@@ -41,6 +51,5 @@ class NoTrailingWhitespaceWalker extends Lint.RuleWalker {
                 lastSeenWasWhitespace = false;
             }
         });
-        // no need to call super to visit the rest of the nodes, so don't call super here
     }
 }
diff --git a/src/rules/whitespaceRule.ts b/src/rules/whitespaceRule.ts
@@ -29,14 +29,12 @@ export class Rule extends Lint.Rules.AbstractRule {
     }
 }
 
-class WhitespaceWalker extends Lint.RuleWalker {
+class WhitespaceWalker extends Lint.SkipableTokenAwareRuleWalker {
     private scanner: ts.Scanner;
-    private tokensToSkipStartEndMap: {[start: number]: number};
 
     constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) {
         super(sourceFile, options);
         this.scanner = ts.createScanner(ts.ScriptTarget.ES5, false, sourceFile.text);
-        this.tokensToSkipStartEndMap = {};
     }
 
     public visitSourceFile(node: ts.SourceFile): void {
@@ -95,28 +93,6 @@ class WhitespaceWalker extends Lint.RuleWalker {
         });
     }
 
-    public visitRegularExpressionLiteral(node: ts.Node) {
-        this.addTokenToSkipFromNode(node);
-        super.visitRegularExpressionLiteral(node);
-    }
-
-    public visitIdentifier(node: ts.Identifier) {
-        this.addTokenToSkipFromNode(node);
-        super.visitIdentifier(node);
-    }
-
-    public visitTemplateExpression(node: ts.TemplateExpression) {
-        this.addTokenToSkipFromNode(node);
-        super.visitTemplateExpression(node);
-    }
-
-    public addTokenToSkipFromNode(node: ts.Node) {
-        if (node.getStart() < node.getEnd()) {
-            // only add to the map nodes whose end comes after their start, to prevent infinite loops
-            this.tokensToSkipStartEndMap[node.getStart()] = node.getEnd();
-        }
-    }
-
     // check for spaces between the operator symbol (except in the case of comma statements)
     public visitBinaryExpression(node: ts.BinaryExpression): void {
         var operatorKind = node.operatorToken.kind;

diff --git a/src/tslint.ts b/src/tslint.ts
@@ -28,6 +28,7 @@
 /// <reference path='language/rule/abstractRule.ts'/>
 /// <reference path='language/walker/ruleWalker.ts'/>
 /// <reference path='language/walker/scopeAwareRuleWalker.ts'/>
+/// <reference path='language/walker/skipableTokenAwareRuleWalker.ts'/>
 
 module Lint {
     var path = require("path");

diff --git a/test/files/rules/enabledisable.test.ts b/test/files/rules/enabledisable.test.ts
@@ -18,3 +18,10 @@ var AAAaA = 'test'
 var AAAaA = 'test'
 /* tslint:enable:zasdadsa */
 var AAAaA = 'test'
+
+/* tslint:enable */
+var  re;
+re = /`/;
+
+/* tslint:disable:quotemark */
+var s = 'xxx';
diff --git a/test/files/rules/jsdoc.test.ts b/test/files/rules/jsdoc.test.ts
@@ -1,3 +1,11 @@
+function makeHeader(pkg: Package) {
+    return [
+        `\t/**`,
+        `\t * ${pkg.name} v${pkg.version}`,
+        `\t */`
+    ].join("\r\n");
+}
+
 class Clazz { //this is not a block comment
     /* block comment
      *Not a jsdoc and not subject to the rules lalala
@@ -50,3 +58,5 @@ one  *
     /** a good one */
 
 }
+
+
diff --git a/test/files/rules/whitespace.test.ts b/test/files/rules/whitespace.test.ts
@@ -46,3 +46,9 @@ a.then(() => {
 }).if(() => {
     return 1;
 });
+
+var name = "something";
+var test = `
+<sl-property-group label=${name} Axes">
+  <div class="repeat"
+`;
diff --git a/test/rules/jsdocFormatRuleTests.ts b/test/rules/jsdocFormatRuleTests.ts
@@ -23,13 +23,13 @@ describe("<jsdoc-format>", () => {
         var fileName = "rules/jsdoc.test.ts";
         var createFormatFailure = Lint.Test.createFailuresOnFile(fileName, JsdocFormatRule.FORMAT_FAILURE_STRING);
         var createAlignmentFailure = Lint.Test.createFailuresOnFile(fileName, JsdocFormatRule.ALIGNMENT_FAILURE_STRING);
-        var expectedFailure1 = createFormatFailure([20, 1], [20, 40]);
-        var expectedFailure2 = createAlignmentFailure([26, 1], [26, 6]);
-        var expectedFailure3 = createFormatFailure([30, 1], [30, 8]);
-        var expectedFailure4 = createFormatFailure([34, 1], [34, 7]);
-        var expectedFailure5 = createAlignmentFailure([39, 1], [39, 19]);
-        var expectedFailure6 = createFormatFailure([42, 5], [42, 26]);
-        var expectedFailure7 = createFormatFailure([44, 5], [44, 32]);
+        var expectedFailure1 = createFormatFailure([28, 1], [28, 40]);
+        var expectedFailure2 = createAlignmentFailure([34, 1], [34, 6]);
+        var expectedFailure3 = createFormatFailure([38, 1], [38, 8]);
+        var expectedFailure4 = createFormatFailure([42, 1], [42, 7]);
+        var expectedFailure5 = createAlignmentFailure([47, 1], [47, 19]);
+        var expectedFailure6 = createFormatFailure([50, 5], [50, 26]);
+        var expectedFailure7 = createFormatFailure([52, 5], [52, 32]);
 
         var actualFailures = Lint.Test.applyRuleOnFile(fileName, JsdocFormatRule);
 

diff --git a/test/rules/noTrailingWhitespaceRuleTests.ts b/test/rules/noTrailingWhitespaceRuleTests.ts
@@ -35,5 +35,6 @@ describe("<no-trailing-whitespace>", () => {
         Lint.Test.assertContainsFailure(actualFailures, expectedFailure3);
         Lint.Test.assertContainsFailure(actualFailures, expectedFailure4);
         Lint.Test.assertContainsFailure(actualFailures, expectedFailure5);
+        assert.lengthOf(actualFailures, 5);
     });
 });