Fix: no-extra-parens incorrect precedence (fixes #7978)
#7999
Conversation
|
@alberto, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @not-an-aardvark and @kaicataldo to be potential reviewers. |
|
LGTM |
| function isNewExpressionWithParens(newExpression) { | ||
| const lastToken = sourceCode.getLastToken(newExpression); | ||
|
|
||
| return lastToken.value === ")"; |
There was a problem hiding this comment.
This function will incorrectly return true for
new (Foo)As a result, the rule still incorrectly reports the following code, even with this patch:
(new (Foo || Bar))()I think the correct way to do this is to check the second-to-last token as well, like the implementation in new-parens.
| @@ -120,6 +121,8 @@ ruleTester.run("no-extra-parens", rule, { | |||
| "a(b)(c)", | |||
| "a((b, c))", | |||
| "new new A", | |||
| { code: "1 ** 2 ** 3", parserOptions: { ecmaVersion: 7 } }, | |||
| { code: "1 ** (2 ** 3)", parserOptions: { ecmaVersion: 7 } }, | |||
There was a problem hiding this comment.
This should be invalid -- the parens are unnecessary in that case because ** is right-associative.
2 ** 3 ** 4
// is equivalent to
2 ** (3 ** 4)So the existing rule also behaves incorrectly in this case:
(2 ** 3) ** 4 // this should be valid
|
LGTM |
|
Good catch @not-an-aardvark, thanks! |
| const shouldSkipRight = NESTED_BINARY && (node.right.type === "BinaryExpression" || node.right.type === "LogicalExpression"); | ||
|
|
||
| if (!shouldSkipLeft && hasExcessParens(node.left) && precedence(node.left) >= prec) { | ||
| if (!shouldSkipLeft && hasExcessParens(node.left) && (precedence(node.left) > prec || (precedence(node.left) === prec && node.operator !== "**"))) { |
There was a problem hiding this comment.
Instead of adding a special case here for **, I think it would be better to update the precedence function in ast-utils to give ** a higher precedence. (The function already gives * and / a higher precedence than + and -.)
There was a problem hiding this comment.
Oh, I thought precendence was already set for exponentiation. I added the conditions to deal with the right associative nature of **.
| @@ -395,13 +413,14 @@ module.exports = { | |||
| */ | |||
| function dryBinaryLogical(node) { | |||
| const prec = precedence(node); | |||
| const shouldSkipLeft = NESTED_BINARY && (node.left.type === "BinaryExpression" || node.left.type === "LogicalExpression"); | |||
| const shouldSkipLeft = (NESTED_BINARY && (node.left.type === "BinaryExpression" || node.left.type === "LogicalExpression")) || | |||
| node.left.type === "UnaryExpression" && node.operator === "**"; | |||
There was a problem hiding this comment.
@not-an-aardvark or did you mean here?
There was a problem hiding this comment.
I mean I don't see how adding a precedence for ** (which I will anyways, I'm not arguing it should be there) would change the code I added to deal with associativity, (the examples you mentioned).
There was a problem hiding this comment.
Before updating the precedence function, the rule incorrectly considered this code valid:
async function foo(){
(await foo) ** bar;
}edit: Actually, never mind that example -- removing the parens there is a syntax error.
|
LGTM |
|
LGTM |
| @@ -673,6 +673,8 @@ module.exports = { | |||
| case "/": | |||
| case "%": | |||
| return 13; | |||
| case "**": | |||
| return 15; | |||
There was a problem hiding this comment.
It doesn't really matter, but is it intentional that you skipped 14 here?
not-an-aardvark
added
accepted
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Fix bug #7978
Is there anything you'd like reviewers to focus on?
No.