Skip to content

Commit

Permalink
Update: refactor eslint-disable comment processing (#9216)
Browse files Browse the repository at this point in the history
(fixes #6592, fixes #9215)
  • Loading branch information
not-an-aardvark committed Sep 6, 2017
1 parent 583f0b8 commit a32ec36
Show file tree
Hide file tree
Showing 4 changed files with 564 additions and 107 deletions.
138 changes: 41 additions & 97 deletions lib/linter.js
Expand Up @@ -18,6 +18,7 @@ const EventEmitter = require("events").EventEmitter,
ConfigOps = require("./config/config-ops"),
validator = require("./config/config-validator"),
Environments = require("./config/environments"),
applyDisableDirectives = require("./util/apply-disable-directives"),
NodeEventGenerator = require("./util/node-event-generator"),
SourceCode = require("./util/source-code"),
Traverser = require("./util/traverser"),
Expand Down Expand Up @@ -249,68 +250,23 @@ function addDeclaredGlobals(program, globalScope, config, envContext) {
}

/**
* Add data to reporting configuration to disable reporting for list of rules
* starting from start location
* @param {Object[]} reportingConfig Current reporting configuration
* @param {Object} start Position to start
* @param {string[]} rulesToDisable List of rules
* @returns {void}
*/
function disableReporting(reportingConfig, start, rulesToDisable) {

if (rulesToDisable.length) {
rulesToDisable.forEach(rule => {
reportingConfig.push({
start,
end: null,
rule
});
});
} else {
reportingConfig.push({
start,
end: null,
rule: null
});
}
}

/**
* Add data to reporting configuration to enable reporting for list of rules
* starting from start location
* @param {Object[]} reportingConfig Current reporting configuration
* @param {Object} start Position to start
* @param {string[]} rulesToEnable List of rules
* @returns {void}
* Creates a collection of disable directives from a comment
* @param {("disable"|"enable"|"disable-line"|"disable-next-line")} type The type of directive comment
* @param {{line: number, column: number}} loc The 0-based location of the comment token
* @param {string} value The value after the directive in the comment
* comment specified no specific rules, so it applies to all rules (e.g. `eslint-disable`)
* @returns {{
* type: ("disable"|"enable"|"disable-line"|"disable-next-line"),
* line: number,
* column: number,
* ruleId: (string|null)
* }[]} Directives from the comment
*/
function enableReporting(reportingConfig, start, rulesToEnable) {
let i;

if (rulesToEnable.length) {
rulesToEnable.forEach(rule => {
for (i = reportingConfig.length - 1; i >= 0; i--) {
if (!reportingConfig[i].end && reportingConfig[i].rule === rule) {
reportingConfig[i].end = start;
break;
}
}
});
} else {

// find all previous disabled locations if they was started as list of rules
let prevStart;

for (i = reportingConfig.length - 1; i >= 0; i--) {
if (prevStart && prevStart !== reportingConfig[i].start) {
break;
}
function createDisableDirectives(type, loc, value) {
const ruleIds = Object.keys(parseListConfig(value));
const directiveRules = ruleIds.length ? ruleIds : [null];

if (!reportingConfig[i].end) {
reportingConfig[i].end = start;
prevStart = reportingConfig[i].start;
}
}
}
return directiveRules.map(ruleId => ({ type, line: loc.line, column: loc.column + 1, ruleId }));
}

/**
Expand All @@ -321,7 +277,16 @@ function enableReporting(reportingConfig, start, rulesToEnable) {
* @param {ASTNode} ast The top node of the AST.
* @param {Object} config The existing configuration data.
* @param {Linter} linterContext Linter context object
* @returns {{config: Object, problems: Problem[]}} Modified config object, along with any problems encountered
* @returns {{
* config: Object,
* problems: Problem[],
* disableDirectives: {
* type: ("disable"|"enable"|"disable-line"|"disable-next-line"),
* line: number,
* column: number,
* ruleId: (string|null)
* }[]
* }} Modified config object, along with any problems encountered
* while parsing config comments
*/
function modifyConfigsFromComments(filename, ast, config, linterContext) {
Expand All @@ -334,7 +299,7 @@ function modifyConfigsFromComments(filename, ast, config, linterContext) {
};
const commentRules = {};
const problems = [];
const reportingConfig = linterContext.reportingConfig;
const disableDirectives = [];

ast.comments.forEach(comment => {

Expand All @@ -360,11 +325,11 @@ function modifyConfigsFromComments(filename, ast, config, linterContext) {
break;

case "eslint-disable":
disableReporting(reportingConfig, comment.loc.start, Object.keys(parseListConfig(value)));
[].push.apply(disableDirectives, createDisableDirectives("disable", comment.loc.start, value));
break;

case "eslint-enable":
enableReporting(reportingConfig, comment.loc.start, Object.keys(parseListConfig(value)));
[].push.apply(disableDirectives, createDisableDirectives("enable", comment.loc.start, value));
break;

case "eslint": {
Expand All @@ -388,11 +353,9 @@ function modifyConfigsFromComments(filename, ast, config, linterContext) {
}
} else { // comment.type === "Line"
if (match[1] === "eslint-disable-line") {
disableReporting(reportingConfig, { line: comment.loc.start.line, column: 0 }, Object.keys(parseListConfig(value)));
enableReporting(reportingConfig, comment.loc.end, Object.keys(parseListConfig(value)));
[].push.apply(disableDirectives, createDisableDirectives("disable-line", comment.loc.start, value));
} else if (match[1] === "eslint-disable-next-line") {
disableReporting(reportingConfig, comment.loc.start, Object.keys(parseListConfig(value)));
enableReporting(reportingConfig, { line: comment.loc.start.line + 2 }, Object.keys(parseListConfig(value)));
[].push.apply(disableDirectives, createDisableDirectives("disable-next-line", comment.loc.start, value));
}
}
}
Expand All @@ -410,33 +373,11 @@ function modifyConfigsFromComments(filename, ast, config, linterContext) {

return {
config: ConfigOps.merge(config, commentConfig),
problems
problems,
disableDirectives
};
}

/**
* Check if message of rule with ruleId should be ignored in location
* @param {Object[]} reportingConfig Collection of ignore records
* @param {string} ruleId Id of rule
* @param {Object} location 1-indexed location of message
* @returns {boolean} True if message should be ignored, false otherwise
*/
function isDisabledByReportingConfig(reportingConfig, ruleId, location) {

for (let i = 0, c = reportingConfig.length; i < c; i++) {

const ignore = reportingConfig[i];

if ((!ignore.rule || ignore.rule === ruleId) &&
(location.line > ignore.start.line || (location.line === ignore.start.line && location.column > ignore.start.column)) &&
(!ignore.end || (location.line < ignore.end.line || (location.line === ignore.end.line && location.column - 1 <= ignore.end.column)))) {
return true;
}
}

return false;
}

/**
* Normalize ECMAScript version from the initial config
* @param {number} ecmaVersion ECMAScript version from the initial config
Expand Down Expand Up @@ -705,7 +646,6 @@ class Linter {
this.currentConfig = null;
this.scopeManager = null;
this.traverser = null;
this.reportingConfig = [];
this.sourceCode = null;
this.version = pkg.version;

Expand All @@ -721,7 +661,6 @@ class Linter {
this.currentConfig = null;
this.scopeManager = null;
this.traverser = null;
this.reportingConfig = [];
this.sourceCode = null;
}

Expand Down Expand Up @@ -822,13 +761,17 @@ class Linter {

const problems = [];
const sourceCode = this.sourceCode;
let disableDirectives;

// parse global comments and modify config
if (allowInlineConfig !== false) {
const modifyConfigResult = modifyConfigsFromComments(filename, sourceCode.ast, config, this);

config = modifyConfigResult.config;
modifyConfigResult.problems.forEach(problem => problems.push(problem));
disableDirectives = modifyConfigResult.disableDirectives;
} else {
disableDirectives = [];
}

const emitter = new EventEmitter().setMaxListeners(Infinity);
Expand Down Expand Up @@ -985,9 +928,10 @@ class Linter {
}
});

return problems
.filter(problem => !isDisabledByReportingConfig(this.reportingConfig, problem.ruleId, problem))
.sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column);
return applyDisableDirectives({
directives: disableDirectives,
problems: problems.sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column)
});
}

/**
Expand Down
115 changes: 115 additions & 0 deletions lib/util/apply-disable-directives.js
@@ -0,0 +1,115 @@
/**
* @fileoverview A module that filters reported problems based on `eslint-disable` and `eslint-enable` comments
* @author Teddy Katz
*/

"use strict";

const lodash = require("lodash");

/**
* Compares the locations of two objects in a source file
* @param {{line: number, column: number}} itemA The first object
* @param {{line: number, column: number}} itemB The second object
* @returns {number} A value less than 1 if itemA appears before itemB in the source file, greater than 1 if
* itemA appears after itemB in the source file, or 0 if itemA and itemB have the same location.
*/
function compareLocations(itemA, itemB) {
return itemA.line - itemB.line || itemA.column - itemB.column;
}

/**
* Given a list of directive comments (i.e. metadata about eslint-disable and eslint-enable comments) and a list
* of reported problems, determines which problems should be reported.
* @param {Object} options Information about directives and problems
* @param {{
* type: ("disable"|"enable"|"disable-line"|"disable-next-line"),
* ruleId: (string|null),
* line: number,
* column: number
* }} options.directives Directive comments found in the file, with one-based columns.
* Two directive comments can only have the same location if they also have the same type (e.g. a single eslint-disable
* comment for two different rules is represented as two directives).
* @param {{ruleId: (string|null), line: number, column: number}[]} options.problems
* A list of problems reported by rules, sorted by increasing location in the file, with one-based columns.
* @returns {{ruleId: (string|null), line: number, column: number}[]}
* A list of reported problems that were not disabled by the directive comments.
*/
module.exports = options => {
const processedDirectives = lodash.flatMap(options.directives, directive => {
switch (directive.type) {
case "disable":
case "enable":
return [directive];

case "disable-line":
return [
{ type: "disable", line: directive.line, column: 1, ruleId: directive.ruleId },
{ type: "enable", line: directive.line + 1, column: 1, ruleId: directive.ruleId }
];

case "disable-next-line":
return [
{ type: "disable", line: directive.line + 1, column: 1, ruleId: directive.ruleId },
{ type: "enable", line: directive.line + 2, column: 1, ruleId: directive.ruleId }
];

default:
throw new TypeError(`Unrecognized directive type '${directive.type}'`);
}
}).sort(compareLocations);

const problems = [];
let nextDirectiveIndex = 0;
let globalDisableActive = false;

// disabledRules is only used when there is no active global /* eslint-disable */ comment.
const disabledRules = new Set();

// enabledRules is only used when there is an active global /* eslint-disable */ comment.
const enabledRules = new Set();

for (const problem of options.problems) {
while (
nextDirectiveIndex < processedDirectives.length &&
compareLocations(processedDirectives[nextDirectiveIndex], problem) <= 0
) {
const directive = processedDirectives[nextDirectiveIndex++];

switch (directive.type) {
case "disable":
if (directive.ruleId === null) {
globalDisableActive = true;
enabledRules.clear();
} else if (globalDisableActive) {
enabledRules.delete(directive.ruleId);
} else {
disabledRules.add(directive.ruleId);
}
break;

case "enable":
if (directive.ruleId === null) {
globalDisableActive = false;
disabledRules.clear();
} else if (globalDisableActive) {
enabledRules.add(directive.ruleId);
} else {
disabledRules.delete(directive.ruleId);
}
break;

// no default
}
}

if (
globalDisableActive && enabledRules.has(problem.ruleId) ||
!globalDisableActive && !disabledRules.has(problem.ruleId)
) {
problems.push(problem);
}
}

return problems;
};

0 comments on commit a32ec36

Please sign in to comment.