Rewrite and fix EnableDisableRulesWalker

docs/usage/rule-flags/index.md

@@ -15,7 +15,7 @@ You can enable/disable TSLint or a subset of rules within a file with the follow
 * `// tslint:disable-next-line:rule1 rule2 rule3...` - Disables the listed rules for the next line
 * etc.
 
-Rules flags enable or disable rules as they are parsed. Disabling an already disabled rule or enabling an already enabled rule has no effect.
+Rules flags enable or disable rules as they are parsed. Disabling an already disabled rule or enabling an already enabled rule has no effect. Enabling a rule that is not present or disabled in `tslint.json` has also no effect.
 
 For example, imagine the directive `/* tslint:disable */` on the first line of a file, `/* tslint:enable:ban class-name */` on the 10th line and `/* tslint:enable */` on the 20th. No rules will be checked between the 1st and 10th lines, only the `ban` and `class-name` rules will be checked between the 10th and 20th, and all rules will be checked for the remainder of the file.
 

src/enableDisableRules.ts

@@ -18,20 +18,20 @@
 import * as ts from "typescript";
 
 import {AbstractRule} from "./language/rule/abstractRule";
-import {IOptions} from "./language/rule/rule";
 import {forEachComment, TokenPosition} from "./language/utils";
-import {RuleWalker} from "./language/walker/ruleWalker";
 import {IEnableDisablePosition} from "./ruleLoader";
 
-export class EnableDisableRulesWalker extends RuleWalker {
-    public enableDisableRuleMap: {[rulename: string]: IEnableDisablePosition[]} = {};
-
-    constructor(sourceFile: ts.SourceFile, options: IOptions, rules: {[name: string]: any}) {
-        super(sourceFile, options);
+export class EnableDisableRulesWalker {
+    private enableDisableRuleMap: {[rulename: string]: IEnableDisablePosition[]};
+    private enabledRules: string[];
 
+    constructor(private sourceFile: ts.SourceFile, rules: {[name: string]: any}) {
+        this.enableDisableRuleMap = {};
+        this.enabledRules = [];
         if (rules) {
-            for (const rule in rules) {
-                if (rules.hasOwnProperty(rule) && AbstractRule.isRuleEnabled(rules[rule])) {
+            for (const rule of Object.keys(rules)) {
+                if (AbstractRule.isRuleEnabled(rules[rule])) {
+                    this.enabledRules.push(rule);
                     this.enableDisableRuleMap[rule] = [{
                         isEnabled: true,
                         position: 0,
@@ -41,116 +41,106 @@ export class EnableDisableRulesWalker extends RuleWalker {
         }
     }
 
-    public visitSourceFile(node: ts.SourceFile) {
-        forEachComment(node, (fullText, _kind, pos) => {
-            return this.handlePossibleTslintSwitch(fullText.substring(pos.tokenStart, pos.end), node, pos);
+    public getEnableDisableRuleMap() {
+        forEachComment(this.sourceFile, (fullText, kind, pos) => {
+            const commentText = kind === ts.SyntaxKind.SingleLineCommentTrivia
+                ? fullText.substring(pos.tokenStart + 2, pos.end)
+                : fullText.substring(pos.tokenStart + 2, pos.end - 2);
+            return this.handleComment(commentText, pos);
         });
+
+        return this.enableDisableRuleMap;
     }
 
-    private getStartOfLinePosition(node: ts.SourceFile, position: number, lineOffset = 0) {
-        const line = ts.getLineAndCharacterOfPosition(node, position).line + lineOffset;
-        const lineStarts = node.getLineStarts();
+    private getStartOfLinePosition(position: number, lineOffset = 0) {
+        const line = ts.getLineAndCharacterOfPosition(this.sourceFile, position).line + lineOffset;
+        const lineStarts = this.sourceFile.getLineStarts();
         if (line >= lineStarts.length) {
             // next line ends with eof or there is no next line
-            return node.getFullWidth();
+            // undefined switches the rule until the end and avoids an extra array entry
+            return undefined;
         }
         return lineStarts[line];
     }
 
     private switchRuleState(ruleName: string, isEnabled: boolean, start: number, end?: number): void {
         const ruleStateMap = this.enableDisableRuleMap[ruleName];
+        if (ruleStateMap === undefined || // skip switches for unknown or disabled rules
+            isEnabled === ruleStateMap[ruleStateMap.length - 1].isEnabled // no need to add switch points if there is no change
+        ) {
+            return;
+        }
 
         ruleStateMap.push({
             isEnabled,
             position: start,
         });
 
         if (end) {
-            // switchRuleState method is only called when rule state changes therefore we can safely use opposite state
+            // we only get here when rule state changes therefore we can safely use opposite state
             ruleStateMap.push({
                 isEnabled: !isEnabled,
                 position: end,
             });
         }
     }
 
-    private getLatestRuleState(ruleName: string): boolean {
-        const ruleStateMap = this.enableDisableRuleMap[ruleName];
+    private handleComment(commentText: string, pos: TokenPosition) {
+        // regex is: start of string followed by any amount of whitespace
+        // followed by tslint and colon
+        // followed by either "enable" or "disable"
+        // followed optionally by -line or -next-line
+        // followed by either colon, whitespace or end of string
+        const match = /^\s*tslint:(enable|disable)(?:-(line|next-line))?(:|\s|$)/.exec(commentText);
+        if (match !== null) {
+            // remove everything matched by the previous regex to get only the specified rules
+            // split at whitespaces
+            // filter empty items coming from whitespaces at start, at end or empty list
+            let rulesList = commentText.substr(match[0].length)
+                                       .split(/\s+/)
+                                       .filter((rule) => !!rule);
+            if (rulesList.length === 0 && match[3] === ":") {
+                // nothing to do here: an explicit separator was specified but no rules to switch
+                return;
+            }
+            if (rulesList.length === 0 ||
+                rulesList.indexOf("all") !== -1) {
+                // if list is empty we default to all enabled rules
+                // if `all` is specified we ignore the other rules and take all enabled rules
+                rulesList = this.enabledRules;
+            }
 
-        return ruleStateMap[ruleStateMap.length - 1].isEnabled;
+            this.handleTslintLineSwitch(rulesList, match[1] === "enable", match[2], pos);
+        }
     }
 
-    private handlePossibleTslintSwitch(commentText: string, node: ts.SourceFile, pos: TokenPosition) {
-        // regex is: start of string followed by "/*" or "//" followed by any amount of whitespace followed by "tslint:"
-        if (commentText.match(/^(\/\*|\/\/)\s*tslint:/)) {
-            const commentTextParts = commentText.split(":");
-            // regex is: start of string followed by either "enable" or "disable"
-            // followed optionally by -line or -next-line
-            // followed by either whitespace or end of string
-            const enableOrDisableMatch = commentTextParts[1].match(/^(enable|disable)(-(line|next-line))?(\s|$)/);
-
-            if (enableOrDisableMatch != null) {
-                const isEnabled = enableOrDisableMatch[1] === "enable";
-                const isCurrentLine = enableOrDisableMatch[3] === "line";
-                const isNextLine = enableOrDisableMatch[3] === "next-line";
-
-                let rulesList = ["all"];
-
-                if (commentTextParts.length === 2) {
-                    // an implicit whitespace separator is used for the rules list.
-                    rulesList = commentTextParts[1].split(/\s+/).slice(1);
-
-                    // remove empty items and potential comment end.
-                    rulesList = rulesList.filter((item) => !!item && !item.includes("*/"));
-
-                    // potentially there were no items, so default to `all`.
-                    rulesList = rulesList.length > 0 ? rulesList : ["all"];
-                } else if (commentTextParts.length > 2) {
-                    // an explicit separator was specified for the rules list.
-                    rulesList = commentTextParts[2].split(/\s+/);
-                }
-
-                if (rulesList.indexOf("all") !== -1) {
-                    // iterate over all enabled rules
-                    rulesList = Object.keys(this.enableDisableRuleMap);
-                }
-
-                for (const ruleToSwitch of rulesList) {
-                    if (!(ruleToSwitch in this.enableDisableRuleMap)) {
-                        // all rules enabled in configuration are already in map - skip switches for disabled rules
-                        continue;
-                    }
-
-                    const previousState = this.getLatestRuleState(ruleToSwitch);
-
-                    if (previousState === isEnabled) {
-                        // no need to add switch points if there is no change in rule state
-                        continue;
-                    }
-
-                    let start: number;
-                    let end: number | undefined;
-
-                    if (isCurrentLine) {
-                        // start at the beginning of the current line
-                        start = this.getStartOfLinePosition(node, pos.tokenStart);
-                        // end at the beginning of the next line
-                        end = pos.end + 1;
-                    } else if (isNextLine) {
-                        // start at the current position
-                        start = pos.tokenStart;
-                        // end at the beginning of the line following the next line
-                        end = this.getStartOfLinePosition(node, pos.tokenStart, 2);
-                    } else {
-                        // disable rule for the rest of the file
-                        // start at the current position, but skip end position
-                        start = pos.tokenStart;
-                        end = undefined;
-                    }
-
-                    this.switchRuleState(ruleToSwitch, isEnabled, start, end);
-                }
+    private handleTslintLineSwitch(rules: string[], isEnabled: boolean, modifier: string, pos: TokenPosition) {
+        let start: number | undefined;
+        let end: number | undefined;
+
+        if (modifier === "line") {
+            // start at the beginning of the line where comment starts
+            start = this.getStartOfLinePosition(pos.tokenStart)!;
+            // end at the beginning of the line following the comment
+            end = this.getStartOfLinePosition(pos.end, 1);
+        } else if (modifier === "next-line") {
+            // start at the beginning of the line following the comment
+            start = this.getStartOfLinePosition(pos.end, 1);
+            if (start === undefined) {
+                // no need to switch anything, there is no next line
+                return;
             }
+            // end at the beginning of the line following the next line
+            end = this.getStartOfLinePosition(pos.end, 2);
+        } else {
+            // switch rule for the rest of the file
+            // start at the current position, but skip end position
+            start = pos.tokenStart;
+            end = undefined;
+        }
+
+        for (const ruleToSwitch of rules) {
+            this.switchRuleState(ruleToSwitch, isEnabled, start, end);
         }
     }
 }

src/linter.ts

@@ -180,13 +180,7 @@ class Linter {
         const configurationRules = isJs ? configuration.jsRules : configuration.rules;
 
         // walk the code first to find all the intervals where rules are disabled
-        const rulesWalker = new EnableDisableRulesWalker(sourceFile, {
-            disabledIntervals: [],
-            ruleArguments: [],
-            ruleName: "",
-        }, configurationRules);
-        rulesWalker.walk(sourceFile);
-        const enableDisableRuleMap = rulesWalker.enableDisableRuleMap;
+        const enableDisableRuleMap = new EnableDisableRulesWalker(sourceFile, configurationRules).getEnableDisableRuleMap();
 
         const rulesDirectories = arrayify(this.options.rulesDirectory)
             .concat(arrayify(configuration.rulesDirectory));

test/rules/_integration/enable-disable/test.ts.lint

@@ -65,11 +65,11 @@ var AAAaA = 'test'
 /* tslint:disable:quotemark */
 var s = 'xxx';
 
-//Test case for issue #1624
+// Test case for issue #1624
 // tslint:disable:quotemark variable-name
 var AAAaA = 'test' // tslint:disable-line:quotemark
 // tslint:disable-next-line:variable-name
-var AAAaA = 'test' //previously `quotemark` rule was enabled after previous line
+var AAAaA = 'test' // previously `quotemark` rule was enabled after previous line
 
 var AAAaA = 'test' // previously both `quotemark` and `variable-name` rules were enabled after previous lines
 
@@ -80,3 +80,49 @@ var AAAaA = 'test' // ensure that disable-line rule correctly handles previous `
 
 // tslint:enable:no-var-keyword
 var AAAaA = 'test' // ensure that disabled in config rule isn't enabled
+
+/* tslint:enable-next-line: */
+var AAAaA = 'test' // ensure that nothing is enabled with explicit separator and empty list of rules
+
+/* tslint:enable-next-line quotemark*/
+var AAAaA = 'test' // ensure that comment end is handled correctly with implicit separator
+            ~~~~~~                                 [' should be "]
+
+/* tslint:enable-next-line:quotemark*/
+var AAAaA = 'test' // ensure that comment end is handled correctly with explicit separator
+            ~~~~~~                                 [' should be "]
+
+// ensure that line switches switch the whole line
+var AAAaA = 'test'; /* tslint:enable-line quotemark */ var AAAaA = 'test'
+            ~~~~~~                                 [' should be "]
+                                                                   ~~~~~~ [' should be "]
+var AAAaA = 'test' // line should not be affected
+
+// ensure that next-line switch only switches next line
+var AAAaA = 'test'; /*tslint:enable-next-line*/ var AAAaA = 'test'
+var AAAaA = 'test'
+    ~~~~~          [variable name must be in camelcase or uppercase]
+            ~~~~~~ [' should be "]
+var AAAaA = 'test'
+var AAAaA = 'test'; //tslint:enable-next-line
+
+
+// ensure that enable/disable switch switches at start of comment
+var AAAaA = 'test'; //tslint:enable
+                      ~~~~~~~~~~~~~ [comment must start with a space]
+var AAAaA = 'test'; //tslint:disable
+    ~~~~~          [variable name must be in camelcase or uppercase]
+            ~~~~~~ [' should be "]
+var AAAaA = 'test'
+
+// ensure that "all" switches all rules
+var AAAaA = 'test'; //tslint:enable-line all
+    ~~~~~          [variable name must be in camelcase or uppercase]
+            ~~~~~~ [' should be "]
+                      ~~~~~~~~~~~~~~~~~~~~~~ [comment must start with a space]
+
+// ensure that "all" switches all rules even if others are in the list
+var AAAaA = 'test'; //tslint:enable-line quotemark all
+    ~~~~~          [variable name must be in camelcase or uppercase]
+            ~~~~~~ [' should be "]
+                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [comment must start with a space]

test/rules/_integration/enable-disable/tslint.json

@@ -2,6 +2,10 @@
   "rules": {
     "quotemark": [true, "double"],
     "variable-name": true,
-    "no-var-keyword": false
+    "no-var-keyword": false,
+    "comment-format": [
+      true,
+      "check-space"
+    ]
   }
 }