Skip to content

Commit 08656db

Browse files
gyandeepsbtmills
gyandeeps
authored and
btmills
committedSep 18, 2017
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)
·
v9.23.0v4.7.1
1 parent 9226495 commit 08656db

File tree

3 files changed

+147
-57
lines changed

3 files changed

+147
-57
lines changed
 

‎lib/util/apply-disable-directives.js

+58-42
Original file line numberDiff line numberDiff line change
@@ -19,46 +19,11 @@ function compareLocations(itemA, itemB) {
1919
}
2020

2121
/**
22-
* Given a list of directive comments (i.e. metadata about eslint-disable and eslint-enable comments) and a list
23-
* of reported problems, determines which problems should be reported.
24-
* @param {Object} options Information about directives and problems
25-
* @param {{
26-
* type: ("disable"|"enable"|"disable-line"|"disable-next-line"),
27-
* ruleId: (string|null),
28-
* line: number,
29-
* column: number
30-
* }} options.directives Directive comments found in the file, with one-based columns.
31-
* Two directive comments can only have the same location if they also have the same type (e.g. a single eslint-disable
32-
* comment for two different rules is represented as two directives).
33-
* @param {{ruleId: (string|null), line: number, column: number}[]} options.problems
34-
* A list of problems reported by rules, sorted by increasing location in the file, with one-based columns.
35-
* @returns {{ruleId: (string|null), line: number, column: number}[]}
36-
* A list of reported problems that were not disabled by the directive comments.
22+
* This is the same as the exported function, except that it doesn't handle disable-line and disable-next-line directives.
23+
* @param {Object} options options (see the exported function)
24+
* @returns {Problem[]} Filtered problems (see the exported function)
3725
*/
38-
module.exports = options => {
39-
const processedDirectives = lodash.flatMap(options.directives, directive => {
40-
switch (directive.type) {
41-
case "disable":
42-
case "enable":
43-
return [directive];
44-
45-
case "disable-line":
46-
return [
47-
{ type: "disable", line: directive.line, column: 1, ruleId: directive.ruleId },
48-
{ type: "enable", line: directive.line + 1, column: 1, ruleId: directive.ruleId }
49-
];
50-
51-
case "disable-next-line":
52-
return [
53-
{ type: "disable", line: directive.line + 1, column: 1, ruleId: directive.ruleId },
54-
{ type: "enable", line: directive.line + 2, column: 1, ruleId: directive.ruleId }
55-
];
56-
57-
default:
58-
throw new TypeError(`Unrecognized directive type '${directive.type}'`);
59-
}
60-
}).sort(compareLocations);
61-
26+
function applyDirectives(options) {
6227
const problems = [];
6328
let nextDirectiveIndex = 0;
6429
let globalDisableActive = false;
@@ -71,10 +36,10 @@ module.exports = options => {
7136

7237
for (const problem of options.problems) {
7338
while (
74-
nextDirectiveIndex < processedDirectives.length &&
75-
compareLocations(processedDirectives[nextDirectiveIndex], problem) <= 0
39+
nextDirectiveIndex < options.directives.length &&
40+
compareLocations(options.directives[nextDirectiveIndex], problem) <= 0
7641
) {
77-
const directive = processedDirectives[nextDirectiveIndex++];
42+
const directive = options.directives[nextDirectiveIndex++];
7843

7944
switch (directive.type) {
8045
case "disable":
@@ -112,4 +77,55 @@ module.exports = options => {
11277
}
11378

11479
return problems;
80+
}
81+
82+
/**
83+
* Given a list of directive comments (i.e. metadata about eslint-disable and eslint-enable comments) and a list
84+
* of reported problems, determines which problems should be reported.
85+
* @param {Object} options Information about directives and problems
86+
* @param {{
87+
* type: ("disable"|"enable"|"disable-line"|"disable-next-line"),
88+
* ruleId: (string|null),
89+
* line: number,
90+
* column: number
91+
* }} options.directives Directive comments found in the file, with one-based columns.
92+
* Two directive comments can only have the same location if they also have the same type (e.g. a single eslint-disable
93+
* comment for two different rules is represented as two directives).
94+
* @param {{ruleId: (string|null), line: number, column: number}[]} options.problems
95+
* A list of problems reported by rules, sorted by increasing location in the file, with one-based columns.
96+
* @returns {{ruleId: (string|null), line: number, column: number}[]}
97+
* A list of reported problems that were not disabled by the directive comments.
98+
*/
99+
module.exports = options => {
100+
const blockDirectives = options.directives
101+
.filter(directive => directive.type === "disable" || directive.type === "enable")
102+
.sort(compareLocations);
103+
104+
const lineDirectives = lodash.flatMap(options.directives, directive => {
105+
switch (directive.type) {
106+
case "disable":
107+
case "enable":
108+
return [];
109+
110+
case "disable-line":
111+
return [
112+
{ type: "disable", line: directive.line, column: 1, ruleId: directive.ruleId },
113+
{ type: "enable", line: directive.line + 1, column: 0, ruleId: directive.ruleId }
114+
];
115+
116+
case "disable-next-line":
117+
return [
118+
{ type: "disable", line: directive.line + 1, column: 1, ruleId: directive.ruleId },
119+
{ type: "enable", line: directive.line + 2, column: 0, ruleId: directive.ruleId }
120+
];
121+
122+
default:
123+
throw new TypeError(`Unrecognized directive type '${directive.type}'`);
124+
}
125+
}).sort(compareLocations);
126+
127+
const problemsAfterBlockDirectives = applyDirectives({ problems: options.problems, directives: blockDirectives });
128+
const problemsAfterLineDirectives = applyDirectives({ problems: problemsAfterBlockDirectives, directives: lineDirectives });
129+
130+
return problemsAfterLineDirectives.sort(compareLocations);
115131
};

‎tests/lib/linter.js

+44-2
Original file line numberDiff line numberDiff line change
@@ -1833,7 +1833,7 @@ describe("Linter", () => {
18331833
it("should not report a violation", () => {
18341834
const code = [
18351835
"alert('test'); // eslint-disable-line no-alert",
1836-
"console('test'); // eslint-disable-line no-console"
1836+
"console.log('test'); // eslint-disable-line no-console"
18371837
].join("\n");
18381838
const config = {
18391839
rules: {
@@ -1850,7 +1850,7 @@ describe("Linter", () => {
18501850
it("should not report a violation", () => {
18511851
const code = [
18521852
"alert('test') // eslint-disable-line no-alert, quotes, semi",
1853-
"console('test'); // eslint-disable-line"
1853+
"console.log('test'); // eslint-disable-line"
18541854
].join("\n");
18551855
const config = {
18561856
rules: {
@@ -2043,6 +2043,48 @@ describe("Linter", () => {
20432043
assert.equal(messages[0].ruleId, "no-console");
20442044
});
20452045

2046+
it("should report no violation", () => {
2047+
const code = [
2048+
"/*eslint-disable no-unused-vars */",
2049+
"var foo; // eslint-disable-line no-unused-vars",
2050+
"var bar;",
2051+
"/* eslint-enable no-unused-vars */" // here
2052+
].join("\n");
2053+
const config = { rules: { "no-unused-vars": 2 } };
2054+
2055+
const messages = linter.verify(code, config, filename);
2056+
2057+
assert.equal(messages.length, 0);
2058+
});
2059+
2060+
it("should report no violation", () => {
2061+
const code = [
2062+
"var foo1; // eslint-disable-line no-unused-vars",
2063+
"var foo2; // eslint-disable-line no-unused-vars",
2064+
"var foo3; // eslint-disable-line no-unused-vars",
2065+
"var foo4; // eslint-disable-line no-unused-vars",
2066+
"var foo5; // eslint-disable-line no-unused-vars"
2067+
].join("\n");
2068+
const config = { rules: { "no-unused-vars": 2 } };
2069+
2070+
const messages = linter.verify(code, config, filename);
2071+
2072+
assert.equal(messages.length, 0);
2073+
});
2074+
2075+
it("should report no violation", () => {
2076+
const code = [
2077+
"/* eslint-disable quotes */",
2078+
"console.log(\"foo\");",
2079+
"/* eslint-enable quotes */"
2080+
].join("\n");
2081+
const config = { rules: { quotes: 2 } };
2082+
2083+
const messages = linter.verify(code, config, filename);
2084+
2085+
assert.equal(messages.length, 0);
2086+
});
2087+
20462088
it("should report a violation", () => {
20472089
const code = [
20482090
"/*eslint-disable no-alert, no-console */",

‎tests/lib/util/apply-disable-directives.js

+45-13
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,34 @@ describe("apply-disable-directives", () => {
143143
);
144144
});
145145

146+
it("filter out problems if disable all then enable foo and then disable foo", () => {
147+
assert.deepEqual(
148+
applyDisableDirectives({
149+
directives: [
150+
{ type: "disable", line: 1, column: 1, ruleId: null },
151+
{ type: "enable", line: 1, column: 5, ruleId: "foo" },
152+
{ type: "disable", line: 2, column: 1, ruleId: "foo" }
153+
],
154+
problems: [{ line: 3, column: 3, ruleId: "foo" }]
155+
}),
156+
[]
157+
);
158+
});
159+
160+
it("filter out problems if disable all then enable foo and then disable all", () => {
161+
assert.deepEqual(
162+
applyDisableDirectives({
163+
directives: [
164+
{ type: "disable", line: 1, column: 1, ruleId: null },
165+
{ type: "enable", line: 1, column: 5, ruleId: "foo" },
166+
{ type: "disable", line: 2, column: 1, ruleId: null }
167+
],
168+
problems: [{ line: 3, column: 3, ruleId: "foo" }]
169+
}),
170+
[]
171+
);
172+
});
173+
146174
it("keeps problems before the eslint-enable comment if there is no corresponding disable comment", () => {
147175
assert.deepEqual(
148176
applyDisableDirectives({
@@ -298,6 +326,23 @@ describe("apply-disable-directives", () => {
298326
[]
299327
);
300328
});
329+
330+
it("handles consecutive comments appropriately", () => {
331+
assert.deepEqual(
332+
applyDisableDirectives({
333+
directives: [
334+
{ type: "disable-line", line: 1, column: 5, ruleId: "foo" },
335+
{ type: "disable-line", line: 2, column: 5, ruleId: "foo" },
336+
{ type: "disable-line", line: 3, column: 5, ruleId: "foo" },
337+
{ type: "disable-line", line: 4, column: 5, ruleId: "foo" },
338+
{ type: "disable-line", line: 5, column: 5, ruleId: "foo" },
339+
{ type: "disable-line", line: 6, column: 5, ruleId: "foo" }
340+
],
341+
problems: [{ line: 2, column: 1, ruleId: "foo" }]
342+
}),
343+
[]
344+
);
345+
});
301346
});
302347

303348
describe("eslint-disable-next-line comments without rules", () => {
@@ -343,19 +388,6 @@ describe("apply-disable-directives", () => {
343388
[]
344389
);
345390
});
346-
347-
it("keeps problems on the next line if there is an eslint-enable comment before the problem on the next line", () => {
348-
assert.deepEqual(
349-
applyDisableDirectives({
350-
directives: [
351-
{ type: "disable-next-line", line: 1, column: 1, ruleId: null },
352-
{ type: "enable", line: 2, column: 1, ruleId: null }
353-
],
354-
problems: [{ line: 2, column: 2, ruleId: "foo" }]
355-
}),
356-
[{ line: 2, column: 2, ruleId: "foo" }]
357-
);
358-
});
359391
});
360392

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

0 commit comments

Comments
 (0)
Please sign in to comment.