Track and restore initial or previous rule state for one line lint switches (fixes palantir#1624) (palantir#1634)

Author: IllusionMH

src/enableDisableRules.ts

@@ -17,13 +17,30 @@
 
 import * as ts from "typescript";
 
+import {AbstractRule} from "./language/rule/abstractRule";
+import {IOptions} from "./language/rule/rule";
 import {scanAllTokens} from "./language/utils";
 import {SkippableTokenAwareRuleWalker} from "./language/walker/skippableTokenAwareRuleWalker";
 import {IEnableDisablePosition} from "./ruleLoader";
 
 export class EnableDisableRulesWalker extends SkippableTokenAwareRuleWalker {
     public enableDisableRuleMap: {[rulename: string]: IEnableDisablePosition[]} = {};
 
+    constructor(sourceFile: ts.SourceFile, options: IOptions, rules: {[name: string]: any}) {
+        super(sourceFile, options);
+
+        if (rules) {
+            for (const rule in rules) {
+                if (rules.hasOwnProperty(rule) && AbstractRule.isRuleEnabled(rules[rule])) {
+                    this.enableDisableRuleMap[rule] = [{
+                        isEnabled: true,
+                        position: 0,
+                    }];
+                }
+            }
+        }
+    }
+
     public visitSourceFile(node: ts.SourceFile) {
         super.visitSourceFile(node);
         const scan = ts.createScanner(ts.ScriptTarget.ES5, false, ts.LanguageVariant.Standard, node.text);
@@ -52,6 +69,29 @@ export class EnableDisableRulesWalker extends SkippableTokenAwareRuleWalker {
         );
     }
 
+    private switchRuleState(ruleName: string, isEnabled: boolean, start: number, end?: number): void {
+        const ruleStateMap = this.enableDisableRuleMap[ruleName];
+
+        ruleStateMap.push({
+            isEnabled,
+            position: start,
+        });
+
+        if (end) {
+            // switchRuleState method is only called 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];
+
+        return ruleStateMap[ruleStateMap.length - 1].isEnabled;
+    }
+
     private handlePossibleTslintSwitch(commentText: string, startingPosition: number, node: ts.SourceFile, scanner: ts.Scanner) {
         // regex is: start of string followed by "/*" or "//" followed by any amount of whitespace followed by "tslint:"
         if (commentText.match(/^(\/\*|\/\/)\s*tslint:/)) {
@@ -82,35 +122,44 @@ export class EnableDisableRulesWalker extends SkippableTokenAwareRuleWalker {
                     rulesList = commentTextParts[2].split(/\s+/);
                 }
 
-                for (const ruleToAdd of rulesList) {
-                    if (!(ruleToAdd in this.enableDisableRuleMap)) {
-                        this.enableDisableRuleMap[ruleToAdd] = [];
+                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;
+
                     if (isCurrentLine) {
                         // start at the beginning of the current line
-                        this.enableDisableRuleMap[ruleToAdd].push({
-                            isEnabled,
-                            position: this.getStartOfLinePosition(node, startingPosition),
-                        });
+                        start = this.getStartOfLinePosition(node, startingPosition);
                         // end at the beginning of the next line
-                        this.enableDisableRuleMap[ruleToAdd].push({
-                            isEnabled: !isEnabled,
-                            position: scanner.getTextPos() + 1,
-                        });
-                    } else {
+                        end = scanner.getTextPos() + 1;
+                    } else if (isNextLine) {
                         // start at the current position
-                        this.enableDisableRuleMap[ruleToAdd].push({
-                            isEnabled,
-                            position: startingPosition,
-                        });
+                        start = startingPosition;
                         // end at the beginning of the line following the next line
-                        if (isNextLine) {
-                            this.enableDisableRuleMap[ruleToAdd].push({
-                                isEnabled: !isEnabled,
-                                position: this.getStartOfLinePosition(node, startingPosition, 2),
-                            });
-                        }
+                        end = this.getStartOfLinePosition(node, startingPosition, 2);
+                    } else {
+                        // disable rule for the rest of the file
+                        // start at the current position, but skip end position
+                        start = startingPosition;
                     }
+
+                    this.switchRuleState(ruleToSwitch, isEnabled, start, end);
                 }
             }
         }

src/language/rule/abstractRule.ts

@@ -24,6 +24,18 @@ export abstract class AbstractRule implements IRule {
     public static metadata: IRuleMetadata;
     private options: IOptions;
 
+    public static isRuleEnabled(ruleConfigValue: any): boolean {
+        if (typeof ruleConfigValue === "boolean") {
+            return ruleConfigValue;
+        }
+
+        if (Array.isArray(ruleConfigValue) && ruleConfigValue.length > 0) {
+            return ruleConfigValue[0];
+        }
+
+        return false;
+    }
+
     constructor(ruleName: string, private value: any, disabledIntervals: IDisabledInterval[]) {
         let ruleArguments: any[] = [];
 
@@ -50,16 +62,6 @@ export abstract class AbstractRule implements IRule {
     }
 
     public isEnabled(): boolean {
-        const value = this.value;
-
-        if (typeof value === "boolean") {
-            return value;
-        }
-
-        if (Array.isArray(value) && value.length > 0) {
-            return value[0];
-        }
-
-        return false;
+        return AbstractRule.isRuleEnabled(this.value);
     }
 }

src/linter.ts

@@ -179,19 +179,19 @@ class Linter {
 
     private getEnabledRules(fileName: string, source?: string, configuration: IConfigurationFile = DEFAULT_CONFIG): IRule[] {
         const sourceFile = this.getSourceFile(fileName, source);
+        const isJs = /\.jsx?$/i.test(fileName);
+        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: [],
             ruleName: "",
-        });
+        }, configurationRules);
         rulesWalker.walk(sourceFile);
         const enableDisableRuleMap = rulesWalker.enableDisableRuleMap;
 
         const rulesDirectories = arrayify(this.options.rulesDirectory)
             .concat(arrayify(configuration.rulesDirectory));
-        const isJs = /\.jsx?$/i.test(fileName);
-        const configurationRules = isJs ? configuration.jsRules : configuration.rules;
         let configuredRules = loadRules(configurationRules, enableDisableRuleMap, rulesDirectories, isJs);
 
         return configuredRules.filter((r) => r.isEnabled());

src/ruleLoader.ts

@@ -50,10 +50,8 @@ export function loadRules(ruleConfiguration: {[name: string]: any},
                 if (isJs && Rule.metadata && Rule.metadata.typescriptOnly != null && Rule.metadata.typescriptOnly) {
                     notAllowedInJsRules.push(ruleName);
                 } else {
-                    const all = "all"; // make the linter happy until we can turn it on and off
-                    const allList = (all in enableDisableRuleMap ? enableDisableRuleMap[all] : []);
                     const ruleSpecificList = (ruleName in enableDisableRuleMap ? enableDisableRuleMap[ruleName] : []);
-                    const disabledIntervals = buildDisabledIntervalsFromSwitches(ruleSpecificList, allList);
+                    const disabledIntervals = buildDisabledIntervalsFromSwitches(ruleSpecificList);
                     rules.push(new Rule(ruleName, ruleValue, disabledIntervals));
 
                     if (Rule.metadata && Rule.metadata.deprecationMessage && shownDeprecations.indexOf(Rule.metadata.ruleName) === -1) {
@@ -141,52 +139,28 @@ function loadRule(directory: string, ruleName: string) {
     return undefined;
 }
 
-/*
- * We're assuming both lists are already sorted top-down so compare the tops, use the smallest of the two,
- * and build the intervals that way.
+/**
+ * creates disabled intervals for rule based on list of switchers for it
+ * @param ruleSpecificList - contains all switchers for rule states sorted top-down and strictly alternating between enabled and disabled
  */
-function buildDisabledIntervalsFromSwitches(ruleSpecificList: IEnableDisablePosition[], allList: IEnableDisablePosition[]) {
-    let isCurrentlyDisabled = false;
-    let disabledStartPosition: number;
+function buildDisabledIntervalsFromSwitches(ruleSpecificList: IEnableDisablePosition[]) {
     const disabledIntervalList: IDisabledInterval[] = [];
-    let i = 0;
-    let j = 0;
-
-    while (i < ruleSpecificList.length || j < allList.length) {
-        const ruleSpecificTopPositon = (i < ruleSpecificList.length ? ruleSpecificList[i].position : Infinity);
-        const allTopPositon = (j < allList.length ? allList[j].position : Infinity);
-        let newPositionToCheck: IEnableDisablePosition;
-        if (ruleSpecificTopPositon < allTopPositon) {
-            newPositionToCheck = ruleSpecificList[i];
-            i++;
-        } else {
-            newPositionToCheck = allList[j];
-            j++;
-        }
+    // starting from second element in the list since first is always enabled in position 0;
+    let i = 1;
 
-        // we're currently disabled and enabling, or currently enabled and disabling -- a switch
-        if (newPositionToCheck.isEnabled === isCurrentlyDisabled) {
-            if (!isCurrentlyDisabled) {
-                // start a new interval
-                disabledStartPosition = newPositionToCheck.position;
-                isCurrentlyDisabled = true;
-            } else {
-                // we're currently disabled and about to enable -- end the interval
-                disabledIntervalList.push({
-                    endPosition: newPositionToCheck.position,
-                    startPosition: disabledStartPosition,
-                });
-                isCurrentlyDisabled = false;
-            }
-        }
-    }
+    while (i < ruleSpecificList.length) {
+        const startPosition = ruleSpecificList[i].position;
+
+        // rule enabled state is always alternating therefore we can use position of next switch as end of disabled interval 
+        // set endPosition as Infinity in case when last switch for rule in a file is disabled
+        const endPosition = ruleSpecificList[i + 1] ? ruleSpecificList[i + 1].position : Infinity;
 
-    if (isCurrentlyDisabled) {
-        // we started an interval but didn't finish one -- so finish it with an Infinity
         disabledIntervalList.push({
-            endPosition: Infinity,
-            startPosition: disabledStartPosition,
+            endPosition,
+            startPosition,
         });
+
+        i += 2;
     }
 
     return disabledIntervalList;

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

@@ -64,3 +64,19 @@ var AAAaA = 'test'
 
 /* tslint:disable:quotemark */
 var s = 'xxx';
+
+//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 both `quotemark` and `variable-name` rules were enabled after previous lines
+
+// tslint:enable:quotemark
+// tslint:disable
+var AAAaA = 'test' // tslint:disable-line:quotemark
+var AAAaA = 'test' // ensure that disable-line rule correctly handles previous `enable rule - disable all` switches
+
+// tslint:enable:no-var-keyword
+var AAAaA = 'test' // ensure that disabled in config rule isn't enabled

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

@@ -1,6 +1,7 @@
 {
   "rules": {
     "quotemark": [true, "double"],
-    "variable-name": true
+    "variable-name": true,
+    "no-var-keyword": false
   }
 }