Fix: add parentheses in no-extra-boolean-cast autofixer (fixes #7912)

[sprzybylski]

What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix for #7912

What changes did you make? (Give an overview)
I've change autofixer in no-extra-boolean-cast rule for "Redundant Boolean call" to include brackets if the inner part is an expression.

[jsf-clabot]

All committers have signed the CLA.

[mention-bot]

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

[eslintbot]

Thanks for the pull request, @sprzybylski! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[eslintbot]

Thanks for the pull request, @sprzybylski! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[not-an-aardvark]

Thanks for the pull request! I have a suggestion for how the fix can be improved.

[not-an-aardvark]

This will resolve the specific case in the bug report, but I think the problem will still exist for arguments that aren't LogicalExpression nodes. For example, this fix would still be incorrect:

!Boolean(1 + 2);

// gets fixed to:

!1 + 2;

I think best solution would be to check the precedence of the argument. The fixer should only add parentheses if the argument has lower precedence than the !(...) node, which is a UnaryExpression. (If the argument has higher precedence, the ! operator will get applied to the entire argument anyway.)

For example, a UnaryExpression has a higher precedence than a LogicalExpression, so !foo && bar gets parsed as (!foo) && bar. On the other hand, a CallExpression has lower precedence than a UnaryExpression, so !foo() will get parsed as !(foo()) rather than (!foo)().

A function to get the precedence of a node can be found here in ast-utils.js.

[eslintbot]

LGTM

[sprzybylski]

Thanks @not-an-aardvark for the suggestion! I'm not 100% sure if I get it right.
In this case node precedence will always be the same because its CallExpression every time. So parentheses will be skipped only for NewExpressions.

[not-an-aardvark]

It looks like this is using the precedence of the Boolean() call expression, but remember that the call expression won't exist anymore when Boolean is removed. Instead, I think we want to compare it to node.parent, which is a UnaryExpression.

To double-check, I think it would be a good idea to add cases like these to the tests:

!Boolean(foo + bar);

// fixed to

!(foo + bar);

// ---

!Boolean(+foo);

// fixed to

!+foo;

// ---

!Boolean(foo());

// fixed to

!foo();

// ---

!Boolean(foo = bar);

// fixed to

!(foo = bar);

[sprzybylski]

Updated PR. Thank you!

[eslintbot]

LGTM

[not-an-aardvark]

LGTM, thanks!

[platinumazure]

Awesome! LGTM, thanks for contributing!

[vitorbal]

LGTM. Thanks a lot!