Fix: preserve formatting when rules are removed from disable directives (#15081)

mdjermanovic · web-flow · commit c9efb5f91937 · 2021-09-24T10:54:07.000+02:00
diff --git 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
@@ -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
