New: allow a fixer to apply multiple fixes simultaneously (fixes #7348)

eslint · Oct 15, 2016 · 48357ea · 48357ea
1 parent 8222004
commit 48357ea
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 32 deletions.
diff --git a/lib/util/source-code-fixer.js b/lib/util/source-code-fixer.js
@@ -66,63 +66,71 @@ SourceCodeFixer.applyFixes = function(sourceCode, messages) {
     }
 
     // clone the array
-    const remainingMessages = [],
-        fixes = [],
-        text = sourceCode.text;
-    let lastFixPos = text.length + 1,
-        prefix = (sourceCode.hasBOM ? BOM : "");
+    const remainingMessages = [];
+    const problems = [];
+    let text = sourceCode.text;
+    let prefix = (sourceCode.hasBOM ? BOM : "");
 
     messages.forEach(function(problem) {
         if (problem.hasOwnProperty("fix")) {
-            fixes.push(problem);
+            problems.push(problem);
         } else {
             remainingMessages.push(problem);
         }
     });
 
-    if (fixes.length) {
+    if (problems.length) {
         debug("Found fixes to apply");
 
-        // sort in reverse order of occurrence
-        fixes.sort(function(a, b) {
-            return b.fix.range[1] - a.fix.range[1] || b.fix.range[0] - a.fix.range[0];
-        });
+        const allFixes = problems.reduce((fixes, problem) => fixes.concat(problem.fix), []).sort((a, b) => a.range[1] - b.range[1] || a.range[0] - b.range[0]);
+        let lastFixPos = Infinity;
+        const fixesToApply = allFixes.reduceRight((fixes, fix) => {
+
+            // If a fix conflicts with a previous fix, don't apply it.
+            if (fix.range[1] < lastFixPos) {
+                fixes.add(fix);
+                lastFixPos = fix.range[0];
+            }
+            return fixes;
+        }, new Set());
+
+        problems.forEach(problem => {
+            const fixes = [].concat(problem.fix);
 
-        // split into array of characters for easier manipulation
-        const chars = text.split("");
+            if (!fixes.every(fix => fixesToApply.has(fix))) {
 
-        fixes.forEach(function(problem) {
-            const fix = problem.fix;
+                // If not every fix for a given problem can be applied, don't apply any of the fixes.
+                fixes.filter(fix => fixesToApply.has(fix)).forEach(fix => fixesToApply.delete(fix));
+                remainingMessages.push(problem);
+            }
+        });
+
+        allFixes.filter(fix => fixesToApply.has(fix)).reverse().forEach(fix => {
             let start = fix.range[0];
             const end = fix.range[1];
             let insertionText = fix.text;
 
-            if (end < lastFixPos) {
-                if (start < 0) {
-
-                    // Remove BOM.
-                    prefix = "";
-                    start = 0;
-                }
+            if (start < 0) {
 
-                if (start === 0 && insertionText[0] === BOM) {
+                // Remove BOM.
+                prefix = "";
+                start = 0;
+            }
 
-                    // Set BOM.
-                    prefix = BOM;
-                    insertionText = insertionText.slice(1);
-                }
+            if (start === 0 && insertionText[0] === BOM) {
 
-                chars.splice(start, end - start, insertionText);
-                lastFixPos = start;
-            } else {
-                remainingMessages.push(problem);
+                // Set BOM.
+                prefix = BOM;
+                insertionText = insertionText.slice(1);
             }
+
+            text = text.slice(0, start) + insertionText + text.slice(end);
         });
 
         return {
             fixed: true,
             messages: remainingMessages.sort(compareMessagesByLocation),
-            output: prefix + chars.join("")
+            output: prefix + text
         };
     } else {
         debug("No fixes to apply");

diff --git a/tests/lib/util/source-code-fixer.js b/tests/lib/util/source-code-fixer.js
@@ -122,6 +122,19 @@ const INSERT_AT_END = {
         message: "nofix2",
         line: 1,
         column: 7
+    },
+    ADD_PARENS = {
+        message: "add-parens",
+        fix: [
+            {
+                range: [13, 13],
+                text: "("
+            },
+            {
+                range: [18, 18],
+                text: ")"
+            }
+        ]
     };
 
 //------------------------------------------------------------------------------
@@ -303,6 +316,14 @@ describe("SourceCodeFixer", function() {
 
                 assert.equal(result1.output, result2.output);
             });
+
+            it("should not apply fixes from the same problem unless all of them can be applied", function() {
+                const result1 = SourceCodeFixer.applyFixes(sourceCode, [ ADD_PARENS, INSERT_IN_MIDDLE ]);
+
+                assert.equal(result1.output, TEST_CODE.replace("6 * 7;", "5 *6 * 7;"));
+            });
+
+            // TODO: (not-an-aardvark) Add more atomicity tests
         });
 
         describe("No Fixes", function() {