Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix: Handle nested disable directive correctly (fixes #9318) (#9322)
* Fix: Handle nested disable directive correctly (fixes #9318)

* add unit test for enabled scenarios

* add more test and identifier for all rules

* Chore: Simply line-comment handling in fix for #9318 (#9323)

* Chore: handle disable-line comments in a separate pass (#9324)
  • Loading branch information
gyandeeps authored and btmills committed Sep 18, 2017
1 parent 9226495 commit 08656db
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 57 deletions.
100 changes: 58 additions & 42 deletions lib/util/apply-disable-directives.js
Expand Up @@ -19,46 +19,11 @@ function compareLocations(itemA, itemB) {
}

/**
* 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.
* This is the same as the exported function, except that it doesn't handle disable-line and disable-next-line directives.
* @param {Object} options options (see the exported function)
* @returns {Problem[]} Filtered problems (see the exported function)
*/
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);

function applyDirectives(options) {
const problems = [];
let nextDirectiveIndex = 0;
let globalDisableActive = false;
Expand All @@ -71,10 +36,10 @@ module.exports = options => {

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

switch (directive.type) {
case "disable":
Expand Down Expand Up @@ -112,4 +77,55 @@ module.exports = options => {
}

return problems;
}

/**
* 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 blockDirectives = options.directives
.filter(directive => directive.type === "disable" || directive.type === "enable")
.sort(compareLocations);

const lineDirectives = lodash.flatMap(options.directives, directive => {
switch (directive.type) {
case "disable":
case "enable":
return [];

case "disable-line":
return [
{ type: "disable", line: directive.line, column: 1, ruleId: directive.ruleId },
{ type: "enable", line: directive.line + 1, column: 0, 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: 0, ruleId: directive.ruleId }
];

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

const problemsAfterBlockDirectives = applyDirectives({ problems: options.problems, directives: blockDirectives });
const problemsAfterLineDirectives = applyDirectives({ problems: problemsAfterBlockDirectives, directives: lineDirectives });

return problemsAfterLineDirectives.sort(compareLocations);
};
46 changes: 44 additions & 2 deletions tests/lib/linter.js
Expand Up @@ -1833,7 +1833,7 @@ describe("Linter", () => {
it("should not report a violation", () => {
const code = [
"alert('test'); // eslint-disable-line no-alert",
"console('test'); // eslint-disable-line no-console"
"console.log('test'); // eslint-disable-line no-console"
].join("\n");
const config = {
rules: {
Expand All @@ -1850,7 +1850,7 @@ describe("Linter", () => {
it("should not report a violation", () => {
const code = [
"alert('test') // eslint-disable-line no-alert, quotes, semi",
"console('test'); // eslint-disable-line"
"console.log('test'); // eslint-disable-line"
].join("\n");
const config = {
rules: {
Expand Down Expand Up @@ -2043,6 +2043,48 @@ describe("Linter", () => {
assert.equal(messages[0].ruleId, "no-console");
});

it("should report no violation", () => {
const code = [
"/*eslint-disable no-unused-vars */",
"var foo; // eslint-disable-line no-unused-vars",
"var bar;",
"/* eslint-enable no-unused-vars */" // here
].join("\n");
const config = { rules: { "no-unused-vars": 2 } };

const messages = linter.verify(code, config, filename);

assert.equal(messages.length, 0);
});

it("should report no violation", () => {
const code = [
"var foo1; // eslint-disable-line no-unused-vars",
"var foo2; // eslint-disable-line no-unused-vars",
"var foo3; // eslint-disable-line no-unused-vars",
"var foo4; // eslint-disable-line no-unused-vars",
"var foo5; // eslint-disable-line no-unused-vars"
].join("\n");
const config = { rules: { "no-unused-vars": 2 } };

const messages = linter.verify(code, config, filename);

assert.equal(messages.length, 0);
});

it("should report no violation", () => {
const code = [
"/* eslint-disable quotes */",
"console.log(\"foo\");",
"/* eslint-enable quotes */"
].join("\n");
const config = { rules: { quotes: 2 } };

const messages = linter.verify(code, config, filename);

assert.equal(messages.length, 0);
});

it("should report a violation", () => {
const code = [
"/*eslint-disable no-alert, no-console */",
Expand Down
58 changes: 45 additions & 13 deletions tests/lib/util/apply-disable-directives.js
Expand Up @@ -143,6 +143,34 @@ describe("apply-disable-directives", () => {
);
});

it("filter out problems if disable all then enable foo and then disable foo", () => {
assert.deepEqual(
applyDisableDirectives({
directives: [
{ type: "disable", line: 1, column: 1, ruleId: null },
{ type: "enable", line: 1, column: 5, ruleId: "foo" },
{ type: "disable", line: 2, column: 1, ruleId: "foo" }
],
problems: [{ line: 3, column: 3, ruleId: "foo" }]
}),
[]
);
});

it("filter out problems if disable all then enable foo and then disable all", () => {
assert.deepEqual(
applyDisableDirectives({
directives: [
{ type: "disable", line: 1, column: 1, ruleId: null },
{ type: "enable", line: 1, column: 5, ruleId: "foo" },
{ type: "disable", line: 2, column: 1, ruleId: null }
],
problems: [{ line: 3, column: 3, ruleId: "foo" }]
}),
[]
);
});

it("keeps problems before the eslint-enable comment if there is no corresponding disable comment", () => {
assert.deepEqual(
applyDisableDirectives({
Expand Down Expand Up @@ -298,6 +326,23 @@ describe("apply-disable-directives", () => {
[]
);
});

it("handles consecutive comments appropriately", () => {
assert.deepEqual(
applyDisableDirectives({
directives: [
{ type: "disable-line", line: 1, column: 5, ruleId: "foo" },
{ type: "disable-line", line: 2, column: 5, ruleId: "foo" },
{ type: "disable-line", line: 3, column: 5, ruleId: "foo" },
{ type: "disable-line", line: 4, column: 5, ruleId: "foo" },
{ type: "disable-line", line: 5, column: 5, ruleId: "foo" },
{ type: "disable-line", line: 6, column: 5, ruleId: "foo" }
],
problems: [{ line: 2, column: 1, ruleId: "foo" }]
}),
[]
);
});
});

describe("eslint-disable-next-line comments without rules", () => {
Expand Down Expand Up @@ -343,19 +388,6 @@ describe("apply-disable-directives", () => {
[]
);
});

it("keeps problems on the next line if there is an eslint-enable comment before the problem on the next line", () => {
assert.deepEqual(
applyDisableDirectives({
directives: [
{ type: "disable-next-line", line: 1, column: 1, ruleId: null },
{ type: "enable", line: 2, column: 1, ruleId: null }
],
problems: [{ line: 2, column: 2, ruleId: "foo" }]
}),
[{ line: 2, column: 2, ruleId: "foo" }]
);
});
});

describe("eslint-disable-next-line comments with rules", () => {
Expand Down

0 comments on commit 08656db

Please sign in to comment.