Rewrite enable/disable logic

src/configs/all.ts

@@ -240,7 +240,7 @@ for (const key in rules) {
     }
 
     const Rule = findRule(key, joinPaths(__dirname, "..", "rules"));
-    if (Rule === "not-found") {
+    if (Rule === undefined) {
         throw new Error(`Couldn't find rule '${key}'.`);
     }
     if (!Rule.metadata.typescriptOnly) {

src/configuration.ts

@@ -452,7 +452,6 @@ export function convertRuleOptions(ruleConfiguration: Map<string, Partial<IOptio
     const output: IOptions[] = [];
     ruleConfiguration.forEach((partialOptions, ruleName) => {
         const options: IOptions = {
-            disabledIntervals: [],
             ruleArguments: partialOptions.ruleArguments || [],
             ruleName,
             ruleSeverity: partialOptions.ruleSeverity || "error",

src/enableDisableRules.ts

@@ -15,133 +15,142 @@
  * limitations under the License.
  */
 
-// tslint:disable prefer-switch
-// (waiting on https://github.com/palantir/tslint/pull/2369)
+// tslint:disable object-literal-sort-keys
 
 import * as utils from "tsutils";
 import * as ts from "typescript";
-import { IOptions } from "./language/rule/rule";
+import { RuleFailure } from "./language/rule/rule";
 
-import { IEnableDisablePosition } from "./ruleLoader";
-
-export class EnableDisableRulesWalker {
-    private enableDisableRuleMap: Map<string, IEnableDisablePosition[]>;
-    private enabledRules: string[];
-
-    constructor(private sourceFile: ts.SourceFile, ruleOptionsList: IOptions[]) {
-        this.enableDisableRuleMap = new Map<string, IEnableDisablePosition[]>();
-        this.enabledRules = [];
-        for (const ruleOptions of ruleOptionsList) {
-            if (ruleOptions.ruleSeverity !== "off") {
-                this.enabledRules.push(ruleOptions.ruleName);
-                this.enableDisableRuleMap.set(ruleOptions.ruleName, [{
-                    isEnabled: true,
-                    position: 0,
-                }]);
-            }
-        }
-    }
-
-    public getEnableDisableRuleMap() {
-        utils.forEachComment(this.sourceFile, (fullText, comment) => {
-            const commentText = comment.kind === ts.SyntaxKind.SingleLineCommentTrivia
-                ? fullText.substring(comment.pos + 2, comment.end)
-                : fullText.substring(comment.pos + 2, comment.end - 2);
-            return this.handleComment(commentText, comment);
-        });
-
-        return this.enableDisableRuleMap;
+export function removeDisabledFailures(sourceFile: ts.SourceFile, failures: RuleFailure[]): RuleFailure[] {
+    if (!failures.length) {
+        // Usually there won't be failures anyway, so no need to look for "tslint:disable".
+        return failures;
     }
 
-    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
-            // 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.get(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,
+    const failingRules = new Set(failures.map((f) => f.getRuleName()));
+    const map = getDisableMap(sourceFile, failingRules);
+    return failures.filter((failure) => {
+        const disabledIntervals = map.get(failure.getRuleName());
+        return !disabledIntervals || !disabledIntervals.some(({ pos, end }) => {
+            const failPos = failure.getStartPosition().getPosition();
+            const failEnd = failure.getEndPosition().getPosition();
+            return failEnd >= pos && (end === -1 || failPos <= end);
         });
+    });
+}
 
-        if (end) {
-            // we only get here when rule state changes therefore we can safely use opposite state
-            ruleStateMap.push({
-                isEnabled: !isEnabled,
-                position: end,
-            });
+/**
+ * The map will have an array of TextRange for each disable of a rule in a file.
+ * (It will have no entry if the rule is never disabled, meaning all arrays are non-empty.)
+ */
+function getDisableMap(sourceFile: ts.SourceFile, failingRules: Set<string>): ReadonlyMap<string, ts.TextRange[]> {
+    const map = new Map<string, ts.TextRange[]>();
+
+    utils.forEachComment(sourceFile, (fullText, comment) => {
+        const commentText = comment.kind === ts.SyntaxKind.SingleLineCommentTrivia
+            ? fullText.substring(comment.pos + 2, comment.end)
+            : fullText.substring(comment.pos + 2, comment.end - 2);
+        const parsed = parseComment(commentText);
+        if (parsed) {
+            const { rulesList, isEnabled, modifier } = parsed;
+            const switchRange = getSwitchRange(modifier, comment, sourceFile);
+            if (switchRange) {
+                const rulesToSwitch = rulesList === "all" ? Array.from(failingRules) : rulesList.filter((r) => failingRules.has(r));
+                for (const ruleToSwitch of rulesToSwitch) {
+                    switchRuleState(ruleToSwitch, isEnabled, switchRange.pos, switchRange.end);
+                }
+            }
         }
-    }
-
-    private handleComment(commentText: string, range: ts.TextRange) {
-        // 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;
+    });
+
+    return map;
+
+    function switchRuleState(ruleName: string, isEnable: boolean, start: number, end: number): void {
+        const disableRanges = map.get(ruleName);
+
+        if (isEnable) {
+            if (disableRanges) {
+                const lastDisable = disableRanges[disableRanges.length - 1];
+                if (lastDisable.end === -1) {
+                    lastDisable.end = start;
+                    if (end !== -1) {
+                        // Disable it again after the enable range is over.
+                        disableRanges.push({ pos: end, end: -1 });
+                    }
+                }
             }
-            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;
+        } else { // disable
+            if (!disableRanges) {
+                map.set(ruleName, [{ pos: start, end }]);
+            } else if (disableRanges[disableRanges.length - 1].end !== -1) {
+                disableRanges.push({ pos: start, end });
             }
-
-            this.handleTslintLineSwitch(rulesList, match[1] === "enable", match[2], range);
         }
     }
+}
 
-    private handleTslintLineSwitch(rules: string[], isEnabled: boolean, modifier: string, range: ts.TextRange) {
-        let start: number | undefined;
-        let end: number | undefined;
-
-        if (modifier === "line") {
-            // start at the beginning of the line where comment starts
-            start = this.getStartOfLinePosition(range.pos)!;
-            // end at the beginning of the line following the comment
-            end = this.getStartOfLinePosition(range.end, 1);
-        } else if (modifier === "next-line") {
+/** End will be -1 to indicate no end. */
+function getSwitchRange(modifier: Modifier, range: ts.TextRange, sourceFile: ts.SourceFile): ts.TextRange | undefined {
+    const lineStarts = sourceFile.getLineStarts();
+
+    switch (modifier) {
+        case "line":
+            return {
+                // start at the beginning of the line where comment starts
+                pos: getStartOfLinePosition(range.pos),
+                // end at the beginning of the line following the comment
+                end: getStartOfLinePosition(range.end, 1),
+            };
+        case "next-line":
             // start at the beginning of the line following the comment
-            start = this.getStartOfLinePosition(range.end, 1);
-            if (start === undefined) {
+            const pos = getStartOfLinePosition(range.end, 1);
+            if (pos === -1) {
                 // no need to switch anything, there is no next line
-                return;
+                return undefined;
             }
             // end at the beginning of the line following the next line
-            end = this.getStartOfLinePosition(range.end, 2);
-        } else {
+            return { pos, end: getStartOfLinePosition(range.end, 2) };
+        default:
             // switch rule for the rest of the file
             // start at the current position, but skip end position
-            start = range.pos;
-            end = undefined;
-        }
+            return { pos: range.pos, end: -1 };
+    }
 
-        for (const ruleToSwitch of rules) {
-            this.switchRuleState(ruleToSwitch, isEnabled, start, end);
-        }
+    /** Returns -1 for last line. */
+    function getStartOfLinePosition(position: number, lineOffset = 0): number {
+        const line = ts.getLineAndCharacterOfPosition(sourceFile, position).line + lineOffset;
+        return line >= lineStarts.length ? -1 : lineStarts[line];
+    }
+}
+
+type Modifier = "line" | "next-line" | undefined;
+function parseComment(commentText: string): { rulesList: string[] | "all", isEnabled: boolean, modifier: Modifier } | undefined {
+    // 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) {
+        return undefined;
     }
+
+    // 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: string[] | "all" = 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 undefined;
+    }
+    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 = "all";
+    }
+
+    return { rulesList, isEnabled: match[1] === "enable", modifier: match[2] as Modifier };
 }

src/language/rule/abstractRule.ts

@@ -17,7 +17,6 @@
 
 import * as ts from "typescript";
 
-import {doesIntersect} from "../utils";
 import {IWalker, WalkContext} from "../walker";
 import { IOptions, IRule, IRuleMetadata, RuleFailure, RuleSeverity } from "./rule";
 
@@ -41,7 +40,7 @@ export abstract class AbstractRule implements IRule {
 
     public applyWithWalker(walker: IWalker): RuleFailure[] {
         walker.walk(walker.getSourceFile());
-        return this.filterFailures(walker.getFailures());
+        return walker.getFailures();
     }
 
     public isEnabled(): boolean {
@@ -61,17 +60,6 @@ export abstract class AbstractRule implements IRule {
     ): RuleFailure[] {
         const ctx = new WalkContext(sourceFile, this.ruleName, options);
         walkFn(ctx);
-        return this.filterFailures(ctx.failures);
-    }
-
-    protected filterFailures(failures: RuleFailure[]): RuleFailure[] {
-        const result: RuleFailure[] = [];
-        for (const failure of failures) {
-            // don't add failures for a rule if the failure intersects an interval where that rule is disabled
-            if (!doesIntersect(failure, this.options.disabledIntervals) && !result.some((f) => f.equals(failure))) {
-                result.push(failure);
-            }
-        }
-        return result;
+        return ctx.failures;
     }
 }

src/language/rule/rule.ts

@@ -18,7 +18,11 @@
 import * as ts from "typescript";
 
 import {arrayify, flatMap} from "../../utils";
-import {IWalker} from "../walker";
+
+export interface RuleStatic {
+    metadata: IRuleMetadata;
+    new(options: IOptions): IRule;
+}
 
 export interface RuleConstructor {
     metadata: IRuleMetadata;
@@ -99,19 +103,12 @@ export interface IOptions {
     ruleArguments: any[];
     ruleSeverity: RuleSeverity;
     ruleName: string;
-    disabledIntervals: IDisabledInterval[];
-}
-
-export interface IDisabledInterval {
-    startPosition: number;
-    endPosition: number;
 }
 
 export interface IRule {
     getOptions(): IOptions;
     isEnabled(): boolean;
     apply(sourceFile: ts.SourceFile): RuleFailure[];
-    applyWithWalker(walker: IWalker): RuleFailure[];
 }
 
 export interface ITypedRule extends IRule {

src/language/utils.ts

@@ -19,21 +19,11 @@ import * as path from "path";
 import { isBlockScopedVariableDeclarationList } from "tsutils";
 import * as ts from "typescript";
 
-import {IDisabledInterval, RuleFailure} from "./rule/rule";
-
 export function getSourceFile(fileName: string, source: string): ts.SourceFile {
     const normalizedName = path.normalize(fileName).replace(/\\/g, "/");
     return ts.createSourceFile(normalizedName, source, ts.ScriptTarget.ES5, /*setParentNodes*/ true);
 }
 
-export function doesIntersect(failure: RuleFailure, disabledIntervals: IDisabledInterval[]) {
-    return disabledIntervals.some((interval) => {
-        const maxStart = Math.max(interval.startPosition, failure.getStartPosition().getPosition());
-        const minEnd = Math.min(interval.endPosition, failure.getEndPosition().getPosition());
-        return maxStart <= minEnd;
-    });
-}
-
 /**
  * @returns true if any modifier kinds passed along exist in the given modifiers array
  */