Repository: eslint/eslint
Pull request #9098 (2017-08-11 06:56:11)

Fix: don't autofix with linter.verifyAndFix when `fix: false` is used
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Bug fix

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (http://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**Tell us about your environment**

* **ESLint Version:** master
* **Node Version:** 8.3.0
* **npm Version:** 5.3.0

**What parser (default, Babel-ESLint, etc.) are you using?**

N/A

**Please show your full configuration:**

N/A

**What did you do? Please include the actual source code causing the issue.**

```js
const eslint = require("eslint");
const linter = new eslint.Linter();

linter.verifyAndFix("foo", { rules: { semi: 2 } }, { fix: false });
```

**What did you expect to happen?**

I expected no autofixes to be applied. The `fix` property specifies a filter function for fixes, and can also be set to a boolean (where `true` implies that all problems are fixed, and `false` implies that no problems are fixed). However, `false` currently behaves the same as `true`.

**What actually happened? Please include the actual, raw output from ESLint.**

```
{ fixed: true, messages: [], output: 'foo;' }
```

**What changes did you make? (Give an overview)**

Due to a bug, `linter.verifyAndFix` previously applied all autofixes when the `fix` option was set to `false`, even though the documented behavior was to apply no autofixes. This commit fixes the bug.

This fix was also included as part of https://github.com/eslint/eslint/pull/9090 because the bug was affecting the updated behavior of `CLIEngine`. However, I decided to separate it out into a different fix because it's an independent bug.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


eslintbot: LGTM
gyandeeps: After thinking about his a little bit, I think it should always fix if you call the function `verifyAndFix`. Thats the purpose of the function. Atleast that how I am thinking about this.
platinumazure: > After thinking about his a little bit, I think it should always fix if you call the function `verifyAndFix`. Thats the purpose of the function. Atleast that how I am thinking about this.

This basically makes sense to me, but what should we do if someone passes in `fix: false`? Should we throw an exception for invalid input?
not-an-aardvark: > After thinking about his a little bit, I think it should always fix if you call the function verifyAndFix.

I see where you're coming from, but we already allow a filter function to be provided, so we will still end up not fixing anything if the filter is `() => false`. Since we accept `true` as a documented input, it makes sense to accept `false` as well.

That said, I notice `verifyAndFix` with `fix: false` is identical to `verify`. Maybe we should add a `fix` option to `verify` defaulting to `false`, and deprecate `verifyAndFix`.
not-an-aardvark: @gyandeeps @platinumazure Are you okay with this being merged as-is for now? Since the function is documented to accept a boolean, I think the current behavior is a bug.
platinumazure: @not-an-aardvark I definitely agree we currently have a bug and have no real objection with merging this in.

Patch:
diff --git a/lib/linter.js b/lib/linter.js
index bb17ac216..a000a69a1 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -1210,7 +1210,7 @@ class Linter extends EventEmitter {
             fixed = false,
             passNumber = 0;
         const debugTextDescription = options && options.filename || `${text.slice(0, 10)}...`;
-        const shouldFix = options && options.fix || true;
+        const shouldFix = options && typeof options.fix !== "undefined" ? options.fix : true;
 
         /**
          * This loop continues until one of the following is true:


Test patch:
diff --git a/tests/lib/linter.js b/tests/lib/linter.js
index ba98b0a1f..b0f423ccd 100644
--- a/tests/lib/linter.js
+++ b/tests/lib/linter.js
@@ -3819,6 +3819,16 @@ describe("Linter", () => {
                 output: "var a;"
             });
         });
+
+        it("does not apply autofixes when fix argument is `false`", () => {
+            const fixResult = linter.verifyAndFix("var a", {
+                rules: {
+                    semi: 2
+                }
+            }, { fix: false });
+
+            assert.strictEqual(fixResult.fixed, false);
+        });
     });
 
     describe("Edge cases", () => {