New: Allow passing a function as fix option (fixes #8039) (#8730)

* New: Allow passing a function as `fix` option (fixes #8039)

* Pass fix in options, instead of separate arg

* Simplify conditional logic

* Return source code if shouldFix is false

This way, the code that uses this doesn’t need to necessarily check
the value of `fixed`.

* Clarify that fixesWereApplied is always true here

If we’ve gotten to this point, at least one fix will have been applied.

* Add a test to ensure that fix functions are pure

Meaning, that they cannot access the `this` value of SourceCodeFixer.

* Add test with conditional shouldFix

This is to verify that the problem can be used to return true or false
conditionally, and that eslint will only apply fixes when true is
returned.

* Account for options not being provided

This is to account for #8809

docs/developer-guide/nodejs-api.md

@@ -215,7 +215,7 @@ The `CLIEngine` is a constructor, and you can create a new instance by passing i
 * `cwd` - Path to a directory that should be considered as the current working directory.
 * `envs` - An array of environments to load (default: empty array). Corresponds to `--env`.
 * `extensions` - An array of filename extensions that should be checked for code. The default is an array containing just `".js"`. Corresponds to `--ext`. It is only used in conjunction with directories, not with filenames or glob patterns.
-* `fix` - True indicates that fixes should be included with the output report, and that errors and warnings should not be listed if they can be fixed. However, the files on disk will not be changed. To persist changes to disk, call [`outputFixes()`](#outputfixes).
+* `fix` - This can be a boolean or a function which will be provided each linting message and should return a boolean. True indicates that fixes should be included with the output report, and that errors and warnings should not be listed if they can be fixed. However, the files on disk will not be changed. To persist changes to disk, call [`outputFixes()`](#outputfixes).
 * `globals` - An array of global variables to declare (default: empty array). Corresponds to `--global`.
 * `ignore` - False disables use of `.eslintignore`, `ignorePath` and `ignorePattern` (default: true). Corresponds to `--no-ignore`.
 * `ignorePath` - The ignore file to use instead of `.eslintignore` (default: null). Corresponds to `--ignore-path`.

lib/cli-engine.js

@@ -45,7 +45,7 @@ const debug = require("debug")("eslint:cli-engine");
  * @property {string} cwd The value to use for the current working directory.
  * @property {string[]} envs An array of environments to load.
  * @property {string[]} extensions An array of file extensions to check.
- * @property {boolean} fix Execute in autofix mode.
+ * @property {boolean|Function} fix Execute in autofix mode. If a function, should return a boolean.
  * @property {string[]} globals An array of global variables to declare.
  * @property {boolean} ignore False disables use of .eslintignore.
  * @property {string} ignorePath The ignore file to use instead of .eslintignore.
@@ -135,7 +135,7 @@ function calculateStatsPerRun(results) {
  * @param {string} text The source code to check.
  * @param {Object} configHelper The configuration options for ESLint.
  * @param {string} filename An optional string representing the texts filename.
- * @param {boolean} fix Indicates if fixes should be processed.
+ * @param {boolean|Function} fix Indicates if fixes should be processed.
  * @param {boolean} allowInlineConfig Allow/ignore comments that change config.
  * @param {Linter} linter Linter context
  * @returns {LintResult} The results for linting on this text.
@@ -195,7 +195,8 @@ function processText(text, configHelper, filename, fix, allowInlineConfig, linte
         if (fix) {
             fixedResult = linter.verifyAndFix(text, config, {
                 filename,
-                allowInlineConfig
+                allowInlineConfig,
+                fix
             });
             messages = fixedResult.messages;
         } else {

lib/linter.js

@@ -1197,6 +1197,7 @@ class Linter extends EventEmitter {
      * @param {string} options.filename The filename from which the text was read.
      * @param {boolean} options.allowInlineConfig Flag indicating if inline comments
      *      should be allowed.
+     * @param {boolean|Function} options.fix Determines whether fixes should be applied
      * @returns {Object} The result of the fix operation as returned from the
      *      SourceCodeFixer.
      */
@@ -1206,6 +1207,7 @@ class Linter extends EventEmitter {
             fixed = false,
             passNumber = 0;
         const debugTextDescription = options && options.filename || `${text.slice(0, 10)}...`;
+        const shouldFix = options && options.fix || true;
 
         /**
          * This loop continues until one of the following is true:
@@ -1223,7 +1225,7 @@ class Linter extends EventEmitter {
             messages = this.verify(text, config, options);
 
             debug(`Generating fixed text for ${debugTextDescription} (pass ${passNumber})`);
-            fixedResult = SourceCodeFixer.applyFixes(this.getSourceCode(), messages);
+            fixedResult = SourceCodeFixer.applyFixes(this.getSourceCode(), messages, shouldFix);
 
             // stop if there are any syntax errors.
             // 'fixedResult.output' is a empty string.

lib/util/source-code-fixer.js

@@ -55,10 +55,10 @@ function SourceCodeFixer() {
  * smart about the fixes and won't apply fixes over the same area in the text.
  * @param {SourceCode} sourceCode The source code to apply the changes to.
  * @param {Message[]} messages The array of messages reported by ESLint.
+ * @param {boolean|Function} [shouldFix=true] Determines whether each message should be fixed
  * @returns {Object} An object containing the fixed text and any unfixed messages.
  */
-SourceCodeFixer.applyFixes = function(sourceCode, messages) {
-
+SourceCodeFixer.applyFixes = function(sourceCode, messages, shouldFix) {
     debug("Applying fixes");
 
     if (!sourceCode) {
@@ -70,6 +70,15 @@ SourceCodeFixer.applyFixes = function(sourceCode, messages) {
         };
     }
 
+    if (shouldFix === false) {
+        debug("shouldFix parameter was false, not attempting fixes");
+        return {
+            fixed: false,
+            messages,
+            output: sourceCode.text
+        };
+    }
+
     // clone the array
     const remainingMessages = [],
         fixes = [],
@@ -78,6 +87,34 @@ SourceCodeFixer.applyFixes = function(sourceCode, messages) {
     let lastPos = Number.NEGATIVE_INFINITY,
         output = bom;
 
+    /**
+     * Try to use the 'fix' from a problem.
+     * @param   {Message} problem The message object to apply fixes from
+     * @returns {boolean}         Whether fix was successfully applied
+     */
+    function attemptFix(problem) {
+        const fix = problem.fix;
+        const start = fix.range[0];
+        const end = fix.range[1];
+
+        // Remain it as a problem if it's overlapped or it's a negative range
+        if (lastPos >= start || start > end) {
+            remainingMessages.push(problem);
+            return false;
+        }
+
+        // Remove BOM.
+        if ((start < 0 && end >= 0) || (start === 0 && fix.text.startsWith(BOM))) {
+            output = "";
+        }
+
+        // Make output to this fix.
+        output += text.slice(Math.max(0, lastPos), Math.max(0, start));
+        output += fix.text;
+        lastPos = end;
+        return true;
+    }
+
     messages.forEach(problem => {
         if (problem.hasOwnProperty("fix")) {
             fixes.push(problem);
@@ -88,32 +125,23 @@ SourceCodeFixer.applyFixes = function(sourceCode, messages) {
 
     if (fixes.length) {
         debug("Found fixes to apply");
+        let fixesWereApplied = false;
 
         for (const problem of fixes.sort(compareMessagesByFixRange)) {
-            const fix = problem.fix;
-            const start = fix.range[0];
-            const end = fix.range[1];
+            if (typeof shouldFix !== "function" || shouldFix(problem)) {
+                attemptFix(problem);
 
-            // Remain it as a problem if it's overlapped or it's a negative range
-            if (lastPos >= start || start > end) {
+                // The only time attemptFix will fail is if a previous fix was
+                // applied which conflicts with it.  So we can mark this as true.
+                fixesWereApplied = true;
+            } else {
                 remainingMessages.push(problem);
-                continue;
-            }
-
-            // Remove BOM.
-            if ((start < 0 && end >= 0) || (start === 0 && fix.text.startsWith(BOM))) {
-                output = "";
             }
-
-            // Make output to this fix.
-            output += text.slice(Math.max(0, lastPos), Math.max(0, start));
-            output += fix.text;
-            lastPos = end;
         }
         output += text.slice(Math.max(0, lastPos));
 
         return {
-            fixed: true,
+            fixed: fixesWereApplied,
             messages: remainingMessages.sort(compareMessagesByLocation),
             output
         };

tests/lib/util/source-code-fixer.js

@@ -10,6 +10,7 @@
 
 const assert = require("chai").assert,
     espree = require("espree"),
+    sinon = require("sinon"),
     SourceCode = require("../../../lib/util/source-code"),
     SourceCodeFixer = require("../../../lib/util/source-code-fixer");
 
@@ -162,6 +163,69 @@ describe("SourceCodeFixer", () => {
             assert.equal(result.output.length, 0);
         });
 
+        describe("shouldFix parameter", () => {
+
+            beforeEach(() => {
+                sourceCode = new SourceCode(TEST_CODE, TEST_AST);
+            });
+
+            it("Should not perform any fixes if 'shouldFix' is false", () => {
+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_END], false);
+
+                assert.isFalse(result.fixed);
+                assert.equal(result.output, sourceCode.text);
+            });
+
+            it("Should perform fixes if 'shouldFix' is not provided", () => {
+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_END]);
+
+                assert.isTrue(result.fixed);
+            });
+
+            it("should call a function provided as 'shouldFix' for each message", () => {
+                const shouldFixSpy = sinon.spy();
+
+                SourceCodeFixer.applyFixes(sourceCode, [INSERT_IN_MIDDLE, INSERT_AT_START, INSERT_AT_END], shouldFixSpy);
+                assert.isTrue(shouldFixSpy.calledThrice);
+            });
+
+            it("should provide a message object as an argument to 'shouldFix'", () => {
+                const shouldFixSpy = sinon.spy();
+
+                SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START], shouldFixSpy);
+                assert.equal(shouldFixSpy.firstCall.args[0], INSERT_AT_START);
+            });
+
+            it("should not perform fixes if 'shouldFix' function returns false", () => {
+                const shouldFixSpy = sinon.spy(() => false);
+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START], shouldFixSpy);
+
+                assert.isFalse(result.fixed);
+            });
+
+            it("should return original text as output if 'shouldFix' function prevents all fixes", () => {
+                const shouldFixSpy = sinon.spy(() => false);
+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START], shouldFixSpy);
+
+                assert.equal(result.output, TEST_CODE);
+            });
+
+            it("should only apply fixes for which the 'shouldFix' function returns true", () => {
+                const shouldFixSpy = sinon.spy(problem => problem.message === "foo");
+                const result = SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START, REPLACE_ID], shouldFixSpy);
+
+                assert.equal(result.output, "var foo = 6 * 7;");
+            });
+
+            it("is called without access to internal eslint state", () => {
+                const shouldFixSpy = sinon.spy();
+
+                SourceCodeFixer.applyFixes(sourceCode, [INSERT_AT_START], shouldFixSpy);
+
+                assert.isUndefined(shouldFixSpy.thisValues[0]);
+            });
+        });
+
         describe("Text Insertion", () => {
 
             it("should insert text at the end of the code", () => {