Update: report double extra parens in no-extra-parens (fixes #12127)

[yeonjuan]

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

[x] Bug fix (template)

What changes did you make? (Give an overview)
Fix #12127

I tried to solve issue #12127 by adding just || hasDoubleExcessParens() everywhere, but it had a performance issue when checking ArrayExpression's element nodes.

So, I made a new function(hasExcessParensWithConsiderPrecedence) which checks extra parens with considering the precedence of a node. This function checks isParenthesisedTwice only if the node surrounded by at least one extra paren.

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

This pr doesn't change the option's behavior that @mdjermanovic mentioned.

There is also a question of options, do they allow just one pair or more.

[kaicataldo]

Thanks for working on this! This generally looks good to me, but I'd like to have at least one other person review before merging.

[mdjermanovic]

These changes look very good! I'd like to check the whole rule once again.

[mdjermanovic]

This pr doesn't change the option's behavior that @mdjermanovic mentioned.

There is also a question of options, do they allow just one pair or more.

By the docs, it sounds like any number of extra parens should be allowed when something is excepted by an option.

• "conditionalAssign": false allows extra parentheses around assignments in conditional test expressions
• "returnAssign": false allows extra parentheses around assignments in return statements
  ...

There are also cases where double parens are indeed required. For example, conditionalAssign was added to avoid conflicts with no-cond-assign (#3317), and that rule allows assignments in conditional expressions' test conditions only if they're parenthesized twice:

/*eslint no-cond-assign: "error"*/

var a = ((b = c)) ? foo : bar; // ok

var a = (b = c) ? foo : bar; // not ok

So it's probably best to leave that part as is.

[mdjermanovic]

Ooops, ConditionalExpression(node) doesn't actually check conditionalAssign option, while it probably should.

But it does check returnAssign option, it's might be relevant to read #6095.

[yeonjuan]

@mdjermanovic

Ooops, ConditionalExpression(node) doesn't actually check conditionalAssign option, while it probably should.

Oh. Yes. I got it. The problem has not been discovered because the bug(#12127) allows extra twice parens in those cases.

// Error in the current PR
 {
   code: "var a = ((b = c)) ? foo : bar;",
   options: ["all", { conditionalAssign: false }]
 }

I can fix it, but maybe there are some other undiscovered issues. So can I add more test cases for option?

[mdjermanovic]

Oh. Yes. I got it. The problem has not been discovered because the bug(#12127) allows extra twice parens in those cases.

// Error in the current PR
 {
   code: "var a = ((b = c)) ? foo : bar;",
   options: ["all", { conditionalAssign: false }]
 }

Yes, it seems that this wasn't noticed because of #12127 bug.

Now when the bug is going to be fixed, we should ensure that this code stays valid for conditionalAssign: true conditionalAssign: false. Sorry! These options without a prefix are confusing.

I can fix it, but maybe there are some other undiscovered issues. So can I add more test cases for option?

That would be great, thanks!

[yeonjuan]

@mdjermanovic
I fixed it by adding isCondAssignException().
By the way, what do you think about this case? - demo - enforceForArrowConditionals: false

/* eslint no-extra-parens: ["error", "all", { "enforceForArrowConditionals": false }] */

var a = () => (1 ? 2 : 3);
var a = () => ((1 ? 2 : 3));  // Unnecessary parentheses arround expression.

The option(enforceDorArrowConditionals: false) doesn't allow extra twice parens in the actual version.
I think it can be treated in separate PR because it occurs in the actual version and less related to this PR, but I want to know whether it is a bug or not.

By the docs, maybe it is a bug?

"enforceForArrowConditionals": false allows extra parentheses around ternary expressions which are the body of an arrow function

[mdjermanovic]

The option(enforceDorArrowConditionals: false) doesn't allow extra twice parens in the actual version.
I think it can be treated in separate PR because it occurs in the actual version and less related to this PR, but I want to know whether it is a bug or not.

I agree, fixing that behavior can be a separate PR (if it's a bug at all).

It looks like the options are inconsistent about this. Some allow only 1 pair of extra parens while the others allow 1, 2, or more. The documentation isn't very specific about this, although it sounds more like that any number of extra parens should be allowed.

It's hard to tell what makes more sense. Probably only 1, except for the special cases such as conditional expressions where the presence of 2 pairs has a meaning for another rule. On the other hand, an option name such as ignoreJSX might imply that the rule should really completely ignore JSX elements and allow any number of extra parens around them.

[mdjermanovic]

function checkUnaryUpdate(node) might also need to be changed? E.g., -((a + b))

It could be nice to change SequenceExpression(node) as well, although it's untestable at the moment because single parens are always unnecessary with the actual syntax.

[yeonjuan]

@mdjermanovic Yes!. That should be.
And VariableDeclarator needs to be changed also. - ex) var foo = ((bar, baz));
I'll check all the cases once more and commit. thanks!

[yeonjuan]

@mdjermanovic It seems to cover all. To note,

1. updated for checking double parens

• in checkUnaryUpdate
• in SequenceExpression
• in VariableDeclarator

2. updated for refactoring

• in checkSpreadOperator
• in checkClass

Thanks :)

[mdjermanovic]

A couple of notes about **

Maybe we should also fix double parens in MemberExpression(node)?

[mdjermanovic]

Actually, let's do MemberExpression(node) in a separate PR.

This PR has already a lot of changes, and it seems that the logic in MemberExpression(node) needs to be modified (#12740).

[yeonjuan]

@mdjermanovic
Wow. it has really many cases for checking. Thanks, I'll check all once again including what you mentioned

[mdjermanovic]

One edge case: for (a of ((b, c)));

[mdjermanovic]

Just changed the title to Update: because this change produces more warnings.

[mdjermanovic]

LGTM! Great work @yeonjuan, this certainly required a lot of effort. Each line is a separate change and there are so many details.

All cases are covered (MemberExpression needs a separate PR).

All changes have tests (some refactored functions already had double parens tests) + regression tests where it seemed appropriate.

Thanks!

[kaicataldo]

Thanks for sticking with this! LGTM.

[kaicataldo]

Thanks for contributing to ESLint!