From c9efb5f91937dcb6c8f3d7cb2f59940046d77901 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 24 Sep 2021 10:54:07 +0200 Subject: [PATCH] Fix: preserve formatting when rules are removed from disable directives (#15081) --- lib/linter/apply-disable-directives.js | 91 ++- tests/lib/linter/apply-disable-directives.js | 12 +- tests/lib/linter/linter.js | 558 +++++++++++++++++++ 3 files changed, 644 insertions(+), 17 deletions(-) diff --git a/lib/linter/apply-disable-directives.js b/lib/linter/apply-disable-directives.js index 20085ed4fe9..e5f2e528ef8 100644 --- a/lib/linter/apply-disable-directives.js +++ b/lib/linter/apply-disable-directives.js @@ -46,26 +46,95 @@ function groupByParentComment(directives) { * @returns {{ description, fix, position }[]} Details for later creation of output Problems. */ function createIndividualDirectivesRemoval(directives, commentToken) { - const listOffset = /^\s*\S+\s+/u.exec(commentToken.value)[0].length; + + /* + * `commentToken.value` starts right after `//` or `/*`. + * All calculated offsets will be relative to this index. + */ + const commentValueStart = commentToken.range[0] + "//".length; + + // Find where the list of rules starts. `\S+` matches with the directive name (e.g. `eslint-disable-line`) + const listStartOffset = /^\s*\S+\s+/u.exec(commentToken.value)[0].length; + + /* + * Get the list text without any surrounding whitespace. In order to preserve the original + * formatting, we don't want to change that whitespace. + * + * // eslint-disable-line rule-one , rule-two , rule-three -- comment + * ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + */ const listText = commentToken.value - .slice(listOffset) // remove eslint-* - .split(/\s-{2,}\s/u)[0] // remove -- directive comment - .trimRight(); - const listStart = commentToken.range[0] + 2 + listOffset; + .slice(listStartOffset) // remove directive name and all whitespace before the list + .split(/\s-{2,}\s/u)[0] // remove `-- comment`, if it exists + .trimRight(); // remove all whitespace after the list + + /* + * We can assume that `listText` contains multiple elements. + * Otherwise, this function wouldn't be called - if there is + * only one rule in the list, then the whole comment must be removed. + */ return directives.map(directive => { const { ruleId } = directive; - const match = new RegExp(String.raw`(?:^|,)\s*${escapeRegExp(ruleId)}\s*(?:$|,)`, "u").exec(listText); - const ruleOffset = match.index; - const ruleEndOffset = ruleOffset + match[0].length; - const ruleText = listText.slice(ruleOffset, ruleEndOffset); + + const regex = new RegExp(String.raw`(?:^|\s*,\s*)${escapeRegExp(ruleId)}(?:\s*,\s*|$)`, "u"); + const match = regex.exec(listText); + const matchedText = match[0]; + const matchStartOffset = listStartOffset + match.index; + const matchEndOffset = matchStartOffset + matchedText.length; + + const firstIndexOfComma = matchedText.indexOf(","); + const lastIndexOfComma = matchedText.lastIndexOf(","); + + let removalStartOffset, removalEndOffset; + + if (firstIndexOfComma !== lastIndexOfComma) { + + /* + * Since there are two commas, this must one of the elements in the middle of the list. + * Matched range starts where the previous rule name ends, and ends where the next rule name starts. + * + * // eslint-disable-line rule-one , rule-two , rule-three -- comment + * ^^^^^^^^^^^^^^ + * + * We want to remove only the content between the two commas, and also one of the commas. + * + * // eslint-disable-line rule-one , rule-two , rule-three -- comment + * ^^^^^^^^^^^ + */ + removalStartOffset = matchStartOffset + firstIndexOfComma; + removalEndOffset = matchStartOffset + lastIndexOfComma; + + } else { + + /* + * This is either the first element or the last element. + * + * If this is the first element, matched range starts where the first rule name starts + * and ends where the second rule name starts. This is exactly the range we want + * to remove so that the second rule name will start where the first one was starting + * and thus preserve the original formatting. + * + * // eslint-disable-line rule-one , rule-two , rule-three -- comment + * ^^^^^^^^^^^ + * + * Similarly, if this is the last element, we've already matched the range we want to + * remove. The previous rule name will end where the last one was ending, relative + * to the content on the right side. + * + * // eslint-disable-line rule-one , rule-two , rule-three -- comment + * ^^^^^^^^^^^^^ + */ + removalStartOffset = matchStartOffset; + removalEndOffset = matchEndOffset; + } return { description: `'${ruleId}'`, fix: { range: [ - listStart + ruleOffset + (ruleText.startsWith(",") && ruleText.endsWith(",") ? 1 : 0), - listStart + ruleEndOffset + commentValueStart + removalStartOffset, + commentValueStart + removalEndOffset ], text: "" }, diff --git a/tests/lib/linter/apply-disable-directives.js b/tests/lib/linter/apply-disable-directives.js index f88150a38b0..72708bcbddc 100644 --- a/tests/lib/linter/apply-disable-directives.js +++ b/tests/lib/linter/apply-disable-directives.js @@ -1451,7 +1451,7 @@ describe("apply-disable-directives", () => { line: 1, column: 24, fix: { - range: [24, 33], + range: [23, 32], text: "" }, severity: 2, @@ -1492,7 +1492,7 @@ describe("apply-disable-directives", () => { line: 1, column: 18, fix: { - range: [18, 25], + range: [18, 26], text: "" }, severity: 2, @@ -1581,7 +1581,7 @@ describe("apply-disable-directives", () => { line: 1, column: 18, fix: { - range: [18, 27], + range: [18, 28], text: "" }, severity: 2, @@ -1593,7 +1593,7 @@ describe("apply-disable-directives", () => { line: 1, column: 28, fix: { - range: [27, 37], + range: [26, 36], text: "" }, severity: 2, @@ -1648,7 +1648,7 @@ describe("apply-disable-directives", () => { line: 1, column: 18, fix: { - range: [18, 27], + range: [18, 28], text: "" }, severity: 2, @@ -1660,7 +1660,7 @@ describe("apply-disable-directives", () => { line: 1, column: 28, fix: { - range: [27, 37], + range: [26, 36], text: "" }, severity: 2, diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 8eedeabd6c2..c2bb5fdd746 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -3404,6 +3404,564 @@ var a = "test2"; ] ); }); + + describe("autofix", () => { + const alwaysReportsRule = { + create(context) { + return { + Program(node) { + context.report({ message: "bad code", loc: node.loc.end }); + } + }; + } + }; + + const neverReportsRule = { + create() { + return {}; + } + }; + + const ruleCount = 3; + const usedRules = Array.from( + { length: ruleCount }, + (_, index) => `used${index ? `-${index}` : ""}` // "used", "used-1", "used-2" + ); + const unusedRules = usedRules.map(name => `un${name}`); // "unused", "unused-1", "unused-2" + + const config = { + reportUnusedDisableDirectives: true, + rules: { + ...Object.fromEntries(usedRules.map(name => [name, "error"])), + ...Object.fromEntries(unusedRules.map(name => [name, "error"])) + } + }; + + beforeEach(() => { + linter.defineRules(Object.fromEntries(usedRules.map(name => [name, alwaysReportsRule]))); + linter.defineRules(Object.fromEntries(unusedRules.map(name => [name, neverReportsRule]))); + }); + + const tests = [ + + //----------------------------------------------- + // Removing the entire comment + //----------------------------------------------- + + { + code: "// eslint-disable-line unused", + output: " " + }, + { + code: "foo// eslint-disable-line unused", + output: "foo " + }, + { + code: "// eslint-disable-line ,unused,", + output: " " + }, + { + code: "// eslint-disable-line unused-1, unused-2", + output: " " + }, + { + code: "// eslint-disable-line ,unused-1,, unused-2,, -- comment", + output: " " + }, + { + code: "// eslint-disable-next-line unused\n", + output: " \n" + }, + { + code: "// eslint-disable-next-line unused\nfoo", + output: " \nfoo" + }, + { + code: "/* eslint-disable \nunused\n*/", + output: " " + }, + + //----------------------------------------------- + // Removing only individual rules + //----------------------------------------------- + + // content before the first rule should not be changed + { + code: "//eslint-disable-line unused, used", + output: "//eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused, used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused, used", + output: "// eslint-disable-line used" + }, + { + code: "/*\neslint-disable unused, used*/", + output: "/*\neslint-disable used*/" + }, + { + code: "/*\n eslint-disable unused, used*/", + output: "/*\n eslint-disable used*/" + }, + { + code: "/*\r\neslint-disable unused, used*/", + output: "/*\r\neslint-disable used*/" + }, + { + code: "/*\u2028eslint-disable unused, used*/", + output: "/*\u2028eslint-disable used*/" + }, + { + code: "/*\u00A0eslint-disable unused, used*/", + output: "/*\u00A0eslint-disable used*/" + }, + { + code: "// eslint-disable-line unused, used", + output: "// eslint-disable-line used" + }, + { + code: "/* eslint-disable\nunused, used*/", + output: "/* eslint-disable\nused*/" + }, + { + code: "/* eslint-disable\n unused, used*/", + output: "/* eslint-disable\n used*/" + }, + { + code: "/* eslint-disable\r\nunused, used*/", + output: "/* eslint-disable\r\nused*/" + }, + { + code: "/* eslint-disable\u2028unused, used*/", + output: "/* eslint-disable\u2028used*/" + }, + { + code: "/* eslint-disable\u00A0unused, used*/", + output: "/* eslint-disable\u00A0used*/" + }, + + // when removing the first rule, the comma and all whitespace up to the next rule (or next lone comma) should also be removed + { + code: "// eslint-disable-line unused,used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused, used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused , used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused, used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused ,used", + output: "// eslint-disable-line used" + }, + { + code: "/* eslint-disable unused\n,\nused */", + output: "/* eslint-disable used */" + }, + { + code: "/* eslint-disable unused \n \n,\n\n used */", + output: "/* eslint-disable used */" + }, + { + code: "/* eslint-disable unused\u2028,\u2028used */", + output: "/* eslint-disable used */" + }, + { + code: "// eslint-disable-line unused\u00A0,\u00A0used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused,,used", + output: "// eslint-disable-line ,used" + }, + { + code: "// eslint-disable-line unused, ,used", + output: "// eslint-disable-line ,used" + }, + { + code: "// eslint-disable-line unused,, used", + output: "// eslint-disable-line , used" + }, + { + code: "// eslint-disable-line unused,used ", + output: "// eslint-disable-line used " + }, + { + code: "// eslint-disable-next-line unused,used\n", + output: "// eslint-disable-next-line used\n" + }, + + // when removing a rule in the middle, one comma and all whitespace between commas should also be removed + { + code: "// eslint-disable-line used-1,unused,used-2", + output: "// eslint-disable-line used-1,used-2" + }, + { + code: "// eslint-disable-line used-1, unused,used-2", + output: "// eslint-disable-line used-1,used-2" + }, + { + code: "// eslint-disable-line used-1,unused ,used-2", + output: "// eslint-disable-line used-1,used-2" + }, + { + code: "// eslint-disable-line used-1, unused ,used-2", + output: "// eslint-disable-line used-1,used-2" + }, + { + code: "/* eslint-disable used-1,\nunused\n,used-2 */", + output: "/* eslint-disable used-1,used-2 */" + }, + { + code: "/* eslint-disable used-1,\n\n unused \n \n ,used-2 */", + output: "/* eslint-disable used-1,used-2 */" + }, + { + code: "/* eslint-disable used-1,\u2028unused\u2028,used-2 */", + output: "/* eslint-disable used-1,used-2 */" + }, + { + code: "// eslint-disable-line used-1,\u00A0unused\u00A0,used-2", + output: "// eslint-disable-line used-1,used-2" + }, + + // when removing a rule in the middle, content around commas should not be changed + { + code: "// eslint-disable-line used-1, unused ,used-2", + output: "// eslint-disable-line used-1,used-2" + }, + { + code: "// eslint-disable-line used-1,unused, used-2", + output: "// eslint-disable-line used-1, used-2" + }, + { + code: "// eslint-disable-line used-1 ,unused,used-2", + output: "// eslint-disable-line used-1 ,used-2" + }, + { + code: "// eslint-disable-line used-1 ,unused, used-2", + output: "// eslint-disable-line used-1 , used-2" + }, + { + code: "// eslint-disable-line used-1 , unused , used-2", + output: "// eslint-disable-line used-1 , used-2" + }, + { + code: "/* eslint-disable used-1\n,unused,\nused-2 */", + output: "/* eslint-disable used-1\n,\nused-2 */" + }, + { + code: "/* eslint-disable used-1\u2028,unused,\u2028used-2 */", + output: "/* eslint-disable used-1\u2028,\u2028used-2 */" + }, + { + code: "// eslint-disable-line used-1\u00A0,unused,\u00A0used-2", + output: "// eslint-disable-line used-1\u00A0,\u00A0used-2" + }, + { + code: "// eslint-disable-line , unused ,used", + output: "// eslint-disable-line ,used" + }, + { + code: "/* eslint-disable\n, unused ,used */", + output: "/* eslint-disable\n,used */" + }, + { + code: "/* eslint-disable used-1,\n,unused,used-2 */", + output: "/* eslint-disable used-1,\n,used-2 */" + }, + { + code: "/* eslint-disable used-1,unused,\n,used-2 */", + output: "/* eslint-disable used-1,\n,used-2 */" + }, + { + code: "/* eslint-disable used-1,\n,unused,\n,used-2 */", + output: "/* eslint-disable used-1,\n,\n,used-2 */" + }, + { + code: "// eslint-disable-line used, unused,", + output: "// eslint-disable-line used," + }, + { + code: "// eslint-disable-next-line used, unused,\n", + output: "// eslint-disable-next-line used,\n" + }, + { + code: "// eslint-disable-line used, unused, ", + output: "// eslint-disable-line used, " + }, + { + code: "// eslint-disable-line used, unused, -- comment", + output: "// eslint-disable-line used, -- comment" + }, + { + code: "/* eslint-disable used, unused,\n*/", + output: "/* eslint-disable used,\n*/" + }, + + // when removing the last rule, the comma and all whitespace up to the previous rule (or previous lone comma) should also be removed + { + code: "// eslint-disable-line used,unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used, unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used ,unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used , unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used, unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used ,unused", + output: "// eslint-disable-line used" + }, + { + code: "/* eslint-disable used\n,\nunused */", + output: "/* eslint-disable used */" + }, + { + code: "/* eslint-disable used \n \n,\n\n unused */", + output: "/* eslint-disable used */" + }, + { + code: "/* eslint-disable used\u2028,\u2028unused */", + output: "/* eslint-disable used */" + }, + { + code: "// eslint-disable-line used\u00A0,\u00A0unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used,,unused", + output: "// eslint-disable-line used," + }, + { + code: "// eslint-disable-line used, ,unused", + output: "// eslint-disable-line used," + }, + { + code: "/* eslint-disable used,\n,unused */", + output: "/* eslint-disable used, */" + }, + { + code: "/* eslint-disable used\n, ,unused */", + output: "/* eslint-disable used\n, */" + }, + + // content after the last rule should not be changed + { + code: "// eslint-disable-line used,unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used,unused ", + output: "// eslint-disable-line used " + }, + { + code: "// eslint-disable-line used,unused ", + output: "// eslint-disable-line used " + }, + { + code: "// eslint-disable-line used,unused -- comment", + output: "// eslint-disable-line used -- comment" + }, + { + code: "// eslint-disable-next-line used,unused\n", + output: "// eslint-disable-next-line used\n" + }, + { + code: "// eslint-disable-next-line used,unused \n", + output: "// eslint-disable-next-line used \n" + }, + { + code: "/* eslint-disable used,unused\u2028*/", + output: "/* eslint-disable used\u2028*/" + }, + { + code: "// eslint-disable-line used,unused\u00A0", + output: "// eslint-disable-line used\u00A0" + }, + + // multiply rules to remove + { + code: "// eslint-disable-line used, unused-1, unused-2", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused-1, used, unused-2", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused-1, unused-2, used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used-1, unused-1, used-2, unused-2", + output: "// eslint-disable-line used-1, used-2" + }, + { + code: "// eslint-disable-line unused-1, used-1, unused-2, used-2", + output: "// eslint-disable-line used-1, used-2" + }, + { + code: ` + /* eslint-disable unused-1, + used-1, + unused-2, + used-2 + */ + `, + output: ` + /* eslint-disable used-1, + used-2 + */ + ` + }, + { + code: ` + /* eslint-disable + unused-1, + used-1, + unused-2, + used-2 + */ + `, + output: ` + /* eslint-disable + used-1, + used-2 + */ + ` + }, + { + code: ` + /* eslint-disable + used-1, + unused-1, + used-2, + unused-2 + */ + `, + output: ` + /* eslint-disable + used-1, + used-2 + */ + ` + }, + { + code: ` + /* eslint-disable + used-1, + unused-1, + used-2, + unused-2, + */ + `, + output: ` + /* eslint-disable + used-1, + used-2, + */ + ` + }, + { + code: ` + /* eslint-disable + ,unused-1 + ,used-1 + ,unused-2 + ,used-2 + */ + `, + output: ` + /* eslint-disable + ,used-1 + ,used-2 + */ + ` + }, + { + code: ` + /* eslint-disable + ,used-1 + ,unused-1 + ,used-2 + ,unused-2 + */ + `, + output: ` + /* eslint-disable + ,used-1 + ,used-2 + */ + ` + }, + { + code: ` + /* eslint-disable + used-1, + unused-1, + used-2, + unused-2 + + -- comment + */ + `, + output: ` + /* eslint-disable + used-1, + used-2 + + -- comment + */ + ` + }, + + // duplicates in the list + { + code: "// eslint-disable-line unused, unused, used", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line unused, used, unused", + output: "// eslint-disable-line used" + }, + { + code: "// eslint-disable-line used, unused, unused, used", + output: "// eslint-disable-line used, used" + } + ]; + + for (const { code, output } of tests) { + // eslint-disable-next-line no-loop-func -- `linter` is getting updated in beforeEach() + it(code, () => { + assert.strictEqual( + linter.verifyAndFix(code, config).output, + output + ); + }); + } + }); }); describe("when evaluating code with comments to change config when allowInlineConfig is disabled", () => {