Skip to content

unnecessary_parenthesis should forgive conditional operator and equality #58953

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

Closed
oprypin opened this issue Dec 7, 2022 · 7 comments · Fixed by dart-archive/linter#3887
Closed
Labels
customer-google3 devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive P2 A bug or feature request we're likely to work on

Comments

@oprypin
Copy link
Contributor

oprypin commented Dec 7, 2022

This came up in a Google-internal discussion as something that some people like to write and would hate the linter to drop the parentheses for them.
As such, it's a blocker for internal adoption.

I would like to introduce these rules:

  • (a ? b : c) is always OK
  • (a == b) and (a != b) are OK in assignment-like and return-like statements.

Examples of replacements that I wish didn't happen (copying my own comments from that discussion):

  •   Foo(String? name) : name = (name == null || name.isEmpty ? _generateNodeName() : name),
      Foo(String? name) : name = name == null || name.isEmpty ? _generateNodeName() : name,

    This is a downgrade because my mind jumps to read it as (name = name == null || the rest).

  •       node.hasValue = (value == null);
          node.hasValue = value == null;

    I dislike the fact that Value = value jumps out at me so strongly. Part of it might just be an issue of distance because == is just longer and makes the precedence appear lower than =. The other part is that I want to use the parentheses to stress the fact that I'm assigning the result of a boolean expression, not assigning "value with some other code trailing behind it".


I will immediately open a pull request that would address this issue.

@oprypin
Copy link
Contributor Author

oprypin commented Dec 7, 2022

And one more example of an unfortunate replacement:

  •   [a, ...(x > 0 && foo != null ? foo : [])]
      [a, ...x > 0 && foo != null ? foo : []]

@bwilkerson
Copy link
Member

I agree with the desire to allow parens around conditional expressions, but I'm not fully convinced by the argument that we should allow extra parens on the right of an assignment or a return. In any case, I'd like to leave some time for community feedback on this proposal before landing the PR to make the change.

@bwilkerson
Copy link
Member

I have to wonder whether we want to keep special casing this lint to try to fit some not-yet-defined criteria or whether we want to deprecate the lint because of all the false positives that appear to be associated with it.

@pq @srawlins

@srawlins
Copy link
Member

I'm happy to keep special casing it. We want to enforce it internal to Google, where data of violations is helping to decide which parentheses look unnecessary and which improve readability.

If we chose to delete it though, and let internal-to-Google resurrect it there, I'd be fine with that too.

@srawlins
Copy link
Member

I have to wonder whether we want to keep special casing this lint to try to fit some not-yet-defined criteria

I think the criteria is defined here. It's soft but I think that's ok, and I still consider it to be defined. https://dart-lang.github.io/linter/lints/unnecessary_parenthesis.html

@pq
Copy link
Member

pq commented Dec 13, 2022

As a general rule, I think every special case kind of erodes the value of a lint and this one is pushing a limit IMO. I wonder if there's a way to re-think this to be a little less opinionated, firing in fewer places where none of them is controversial, at the cost of passing over places that would require heuristics. I guess I'm asking, can we make it less soft, more precise with a more limited scope.

@oprypin
Copy link
Contributor Author

oprypin commented Dec 13, 2022

People really have looked over violations in detail and this is the only exception that came up.

There won't be any interest in reworking this rule because it's already really great otherwise.

It's true that this particular exception is fuzzy, but I think the core of the lint is solid, it's just the implementation that needs to have a lot of branches because the AST doesn't have a generic utility for reporting which constructs have parentheses and where.

@pq pq added P2 A bug or feature request we're likely to work on customer-google3 labels Dec 20, 2022
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-google3 devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive P2 A bug or feature request we're likely to work on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants