diff --git a/docs/usage/rule-flags/index.md b/docs/usage/rule-flags/index.md index bf2d5739d97..2237dd673d4 100644 --- a/docs/usage/rule-flags/index.md +++ b/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. diff --git a/src/enableDisableRules.ts b/src/enableDisableRules.ts index 6c819f7ff44..031ad1af737 100644 --- a/src/enableDisableRules.ts +++ b/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,24 +41,35 @@ 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, @@ -66,7 +77,7 @@ export class EnableDisableRulesWalker extends RuleWalker { }); 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, @@ -74,83 +85,62 @@ export class EnableDisableRulesWalker extends RuleWalker { } } - 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); } } } diff --git a/src/linter.ts b/src/linter.ts index 322a2d0f915..e1a7c007176 100644 --- a/src/linter.ts +++ b/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)); diff --git a/test/rules/_integration/enable-disable/test.ts.lint b/test/rules/_integration/enable-disable/test.ts.lint index fd0dd3694df..98d9462aca7 100644 --- a/test/rules/_integration/enable-disable/test.ts.lint +++ b/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] diff --git a/test/rules/_integration/enable-disable/tslint.json b/test/rules/_integration/enable-disable/tslint.json index 6153bd7842b..7038f339ba2 100644 --- a/test/rules/_integration/enable-disable/tslint.json +++ b/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" + ] } }