Fix: no-extra-boolean-cast invalid autofix for Boolean() without args

[mdjermanovic]

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

[X] Bug fix

Tell us about your environment

• ESLint Version: 6.1.0
• Node Version: 10.16.0
• npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration

module.exports = {
  parserOptions: {
    ecmaVersion: 2015,
  },
};

What did you do? Please include the actual source code causing the issue.

/*eslint no-extra-boolean-cast: "error"*/

// Bug #1

var foo = Boolean() ? bar() : baz();

if (Boolean()) {
    foo();
}

while (Boolean()) {
  foo();
}

// Bug #2

var foo = typeof!Boolean();

Demo link

What did you expect to happen?

Not to change behavior and/or remove the code.

What actually happened? Please include the actual, raw output from ESLint.

/*eslint no-extra-boolean-cast: "error"*/

// Bug #1

var foo = true;

true

true

// Bug #2

var foo = typeoftrue;

What changes did you make? (Give an overview)

Fixed bugs:

• Bug#1 - code was assuming it's always !Boolean() and the fix was replacing parent with true.
• Bug#2 - didn't check can the tokens be adjacent.

Is there anything you'd like reviewers to focus on?

[kaicataldo]

This change makes sense to me. I know that this is the current behavior, but the fixer changing

if (Boolean()) {
    foo();
}

while (Boolean()) {
    foo();
}

to

true;

true;

feels like the autofixer overreaching to me. Now that we're fixing

var foo = Boolean() ? bar() : baz();

to

var foo = false ? bar() : baz();

, I wonder if we should be fixing the first example above to

if (false) {
    foo();
}

while (false) {
    foo();
}

as well? Additionally, seems like it should fix the following:

if (!Boolean()) {
    foo();
}

while (!Boolean()) {
    foo();
}

to

if (true) {
    foo();
}

while (true) {
    foo();
}

I'm curious what the rest of the team has to say about this - there might be some historical context around this that I'm not aware of.

[mdjermanovic]

This change makes sense to me. I know that this is the current behavior, but the fixer changing

if (Boolean()) {
    foo();
}

while (Boolean()) {
    foo();
}

to

true;

true;

feels like the autofixer overreaching to me.

I think this is a bug from #7977. It was assumed that this rule warns on Boolean() only when it's in !, but it can appear in other 'boolean contexts'.

While these two don't change behavior (just delete a lot of unreachable code, which I guess is also not okay), the first example does:

var foo = Boolean() ? bar() : baz();

to

var foo = true;

This is certainly a bug, foo = baz() is changed to foo = true.

Now that we're fixing

var foo = Boolean() ? bar() : baz();

to

var foo = false ? bar() : baz();

, I wonder if we should be fixing the first example above to

if (false) {
    foo();
}

while (false) {
    foo();
}

as well?

It's working exactly like that with this fix:

{
    code: "if (Boolean()) {}",
    output: "if (false) {}",
    errors: [{
        messageId: "unexpectedCall",
        type: "CallExpression"
    }]
},

Additionally, seems like it should fix the following:

if (!Boolean()) {
    foo();
}

while (!Boolean()) {
    foo();
}

to

if (true) {
    foo();
}

while (true) {
    foo();
}

It was already working like this, and this shouldn't be changed by this fix. But to be sure, I'll add these test cases.

[mdjermanovic]

Added 3 test cases and modified 1 to match the example, and everything works as you described :)

[kaicataldo]

LGTM. Thanks for contributing!

[platinumazure]

Curious to know what happens in this case: void ! /* Comment */ Boolean(). I'm thinking sourceCode.getTokenBefore should find the correct token since you didn't specify { comments: true }, but I think we should make sure the comment is preserved. That might mean no autofix in that case (we have other rules that choose not to fix if it is difficult to preserve comments).

Otherwise, changes look good to me. Thanks for contributing!

[mdjermanovic]

Can't verify in the PR version at the moment, but I'm pretty sure that this comment will be removed, as it is within the node which is supposed to be replaced.

It was already working like that (Demo link). I'll fix that in this PR, autofix shouldn't delete user's comments.

I've already noticed that many rules are deleting comments (which should never happen I guess) and was thinking about an additional method in the FixTracker which would set a flag to check for comments within the range before the fix. I'll open an issue with that idea if it makes sense.

[platinumazure]

I've already noticed that many rules are deleting comments (which should never happen I guess) and was thinking about an additional method in the FixTracker which would set a flag to check for comments within the range before the fix. I'll open an issue with that idea if it makes sense.

That sounds like a great idea!

[mdjermanovic]

Comments should be safe now in these and other autofix cases in this rule, please check is it ok.

[platinumazure]

LGTM. Thanks for adding all the extra tests!

[kaicataldo]

Great call on handling comments 👍 This autofixer is in much better shape than it was previously!

[aladdin-add]

LGTM, thanks for the good tests!!