Rewrite enable/disable logic

src/configuration.ts

@@ -361,7 +361,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,130 +15,156 @@
  * limitations under the License.
  */
 
+// tslint:disable object-literal-sort-keys
+
 import * as utils from "tsutils";
 import * as ts from "typescript";
-import { IOptions } from "./language/rule/rule";
-
-import { IEnableDisablePosition } from "./ruleLoader";
+import { RuleFailure } from "./language/rule/rule";
 
-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,
-                }]);
-            }
-        }
-    }
+export interface RuleDisabler {
+    /** True if there is a `tslint:disable` in the range of the failure. */
+    isDisabled(failure: RuleFailure): boolean;
+    /** True if there is an explicit `tslint:enable` for a rule even if it is not turned on in tslint.json. */
+    isExplicitlyEnabled(ruleName: string): boolean;
+}
 
-    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);
-        });
+export function getDisabler(sourceFile: ts.SourceFile, enabledRules: string[]): RuleDisabler {
+    const map = getDisableMap(sourceFile, enabledRules);
+    return {
+        isExplicitlyEnabled(ruleName) {
+            return map.has(ruleName);
+        },
+        isDisabled(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);
+            });
+        },
+    };
+}
 
-        return this.enableDisableRuleMap;
-    }
+/**
+ * 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, enabledRules: string[]): ReadonlyMap<string, ts.TextRange[]> {
+    const enabledRulesSet = new Set(enabledRules);
+    const map = new Map<string, ts.TextRange[]>();
 
-    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;
+    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) {
+                for (const ruleToSwitch of (rulesList === "all" ? enabledRules : rulesList)) {
+                    switchRuleState(ruleToSwitch, isEnabled, switchRange.pos, switchRange.end);
+                }
+            }
         }
-        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;
-        }
+    return map;
 
-        ruleStateMap.push({
-            isEnabled,
-            position: start,
-        });
+    function switchRuleState(ruleName: string, isEnable: boolean, start: number, end: number): void {
+        const disableRanges = map.get(ruleName);
 
-        if (end) {
-            // we only get here when rule state changes therefore we can safely use opposite state
-            ruleStateMap.push({
-                isEnabled: !isEnabled,
-                position: 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;
+        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 });
+                    }
+                }
+            } else if (!enabledRulesSet.has(ruleName)) {
+                // This rule is being explicitly enabled. Disable it outside of this particular range.
+                const ranges = [{ pos: 0, end: start }];
+                map.set(ruleName, ranges);
+                if (end !== -1) {
+                    // If enabled just for this line, disable after.
+                    ranges.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;
+/** 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();
 
-        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") {
+    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 {
@@ -53,17 +52,6 @@ export abstract class AbstractRule implements IRule {
     protected applyWithFunction<T>(sourceFile: ts.SourceFile, walkFn: (ctx: WalkContext<T | void>) => void, options?: T): 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

@@ -17,7 +17,10 @@
 
 import * as ts from "typescript";
 
-import {IWalker} from "../walker";
+export interface RuleStatic {
+    metadata: IRuleMetadata;
+    new(options: IOptions): IRule;
+}
 
 export interface IRuleMetadata {
     /**
@@ -92,19 +95,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

@@ -18,21 +18,11 @@
 import * as path from "path";
 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;
-    });
-}
-
 /** @deprecated use forEachToken instead */
 export function scanAllTokens(scanner: ts.Scanner, callback: (s: ts.Scanner) => void) {
     let lastStartPos = -1;