Fix: don't autofix with linter.verifyAndFix when fix: false is used

[not-an-aardvark]

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix

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.

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 #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

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gyandeeps, @nzakas and @mysticatea to be potential reviewers.

[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.