Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-extra-parens never reports double parens #12127

Closed
mdjermanovic opened this issue Aug 19, 2019 · 11 comments · Fixed by #12697
Closed

no-extra-parens never reports double parens #12127

mdjermanovic opened this issue Aug 19, 2019 · 11 comments · Fixed by #12697
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Tell us about your environment

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

same in ESLint 5.16.0, this is not regression.

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, as well as the command that you used to run ESLint.

This is not reported because of the precedence:

/*eslint no-extra-parens: "error"*/

a * ((b + c));

This is not reported because of the option (does the option allow multiple parens or just one pair?):

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

if (((a = b))) {}

There are many examples like this, seems that double parens are never reported whenever a single pair wouldn't be.

eslint index.js

What did you expect to happen?

Errors in both cases, I guess.

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

No errors.

Are you willing to submit a pull request to fix this bug?

Yes, if this is indeed a bug. But this needs some analysis and design first, I'm thinking about an additional selector that would catch all double parens regardless of the context.

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Aug 19, 2019
@platinumazure
Copy link
Member

CC @mysticatea

I'm wondering if this was caused by fee6acb. I did remember seeing some changes in no-extra-parens in that commit...

@platinumazure
Copy link
Member

@mdjermanovic Thanks for reporting. Any chance you can test with ESLint 6.1.x to make sure the issue isn't present there?

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion patch candidate This issue may necessitate a patch release in the next few days rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 19, 2019
@mdjermanovic
Copy link
Member Author

No, it wasn't that commit (tested with 6.1.0 to be sure), that change to a smarter isParenthesised might actually help to solve this, but this probably still needs a lot of work and testing. It's quite possible that these examples didn't work from the start, I believe this isn't a regression.

@platinumazure
Copy link
Member

@mdjermanovic Just to confirm, you're saying the issue you reported also is also reproducible in 6.1.0? Thanks!

@mdjermanovic
Copy link
Member Author

@mdjermanovic Just to confirm, you're saying the issue you reported also is also reproducible in 6.1.0? Thanks!

Yes, sorry about the confusing post. The same issue is also reproducible in 6.1.0

@platinumazure platinumazure removed the patch candidate This issue may necessitate a patch release in the next few days label Aug 19, 2019
@platinumazure
Copy link
Member

Thanks @mdjermanovic, I've removed the "patch candidate" label based on your last statement.

I'm not 100% sure what the intended design of the rule is here. I'm hoping someone else on @eslint/eslint-team might have an idea on how best to handle this issue.

@mdjermanovic
Copy link
Member Author

If this is not the intended design, the problem is with conditionals like this, that doesn't account for double parens:

if (hasExcessParens(node.right) && precedence(node.right) >= precedence(node)) {
       report(node.right);
}

This could be solved by adding || hasDoubleExcessParens() everywhere, or perhaps with a 'big' selector that would report double parens for all nodes, I'm not sure which solution is better. There is also a question of options, do they allow just one pair or more.

@mysticatea
Copy link
Member

I think that to handle this issue as a bug makes sense.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion help wanted The team would welcome a contribution from the community for this issue Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 29, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Nov 18, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@kaicataldo kaicataldo self-assigned this Nov 19, 2019
@kaicataldo kaicataldo removed the auto closed The bot closed this issue label Nov 19, 2019
@kaicataldo
Copy link
Member

Reopening and assigning to myself so that it doesn't get auto closed again.

@platinumazure platinumazure reopened this Nov 20, 2019
@kaicataldo
Copy link
Member

Whoops, thanks @platinumazure

@kaicataldo kaicataldo removed the Hacktoberfest Recommended issue for those participating in Hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 25, 2019
@mdjermanovic mdjermanovic removed the help wanted The team would welcome a contribution from the community for this issue label Jan 12, 2020
kaicataldo pushed a commit that referenced this issue Jan 16, 2020
…12697)

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

* rename function, add comments

* review feedback

* add more test cases

* apply cond-assign-exception to test node only

* add more checking for double parens

* move unaryExpression and exponentiation check

* change comments

* rebase checkClass, add test case for regression

* add invalid test case

* add double parens check for for-of, sequence expression
montmanu pushed a commit to montmanu/eslint that referenced this issue Mar 4, 2020
…2127) (eslint#12697)

* Fix: report double extra parens in no-extra-parens (fixes eslint#12127)

* rename function, add comments

* review feedback

* add more test cases

* apply cond-assign-exception to test node only

* add more checking for double parens

* move unaryExpression and exponentiation check

* change comments

* rebase checkClass, add test case for regression

* add invalid test case

* add double parens check for for-of, sequence expression
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 16, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants