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

[java] UselessParentheses false positive with conditional operator #1673

Closed
Vampire opened this issue Feb 19, 2019 · 6 comments · Fixed by #2900
Closed

[java] UselessParentheses false positive with conditional operator #1673

Vampire opened this issue Feb 19, 2019 · 6 comments · Fixed by #2900
Labels
a:false-positive PMD flags a piece of code that is not problematic
Milestone

Comments

@Vampire
Copy link
Contributor

Vampire commented Feb 19, 2019

Affects PMD Version: 6.11.0
Rule: UselessParentheses

Code Sample demonstrating the issue:

throw new IllegalArgumentException(String.format(
        "Unsupported client with class '%s' given",
        (discordClient == null) ? null : discordClient.getClass()));

Additionally, should we add properties to allow whitelisting some scenarios, such as this one?

while ((result instanceof CompletionException) && (cause != null)) {
    result = cause;
    cause = result.getCause();
}

Running PMD through: Gradle

@jsotuyod
Copy link
Member

@Vampire the rule will detect any pair of parenthesis that can be removed without affecting the code's behavior. As such, this being reported is effectively ok.

As for the enhancement… I guess the thing is how far we need to go… I mean, I know people tend to have trouble remembering wether && or || takes precedence first, but how far are we gonna take it? ternaries? bit-wise operations? comparations? arithmetic operations?

I'm unsure what a sane set of properties and what scenarios they allow will be intuitive for users and reasonable to maintain.

@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

Then the rule has a false-negative instead.
In

throw new IllegalArgumentException(String.format(
        "Unsupported client with class '%s' given",
        (discordClient == null ? null : discordClient.getClass())));

useless parantheses are found, in

throw new IllegalArgumentException(String.format(
        "Unsupported client with class '%s' given",
        (discordClient == null) ? null : discordClient.getClass()));

they are not.

While I also here want the parentheses to stay for easier reading.

True, finding a sane set of what to report and what not that works for all is probably impossible. :-(

@oowekyala
Copy link
Member

For comparison, the IntelliJ inspection has a property to ignore parentheses when the inner and outer operators have different precedences. That's is not very configurable, but it's simple and makes sense.

@jsotuyod jsotuyod changed the title [java] possible false-positive in UselessParentheses [java] false-positive in UselessParentheses Feb 20, 2019
@jsotuyod jsotuyod added the a:false-positive PMD flags a piece of code that is not problematic label Feb 20, 2019
@jsotuyod
Copy link
Member

Ok, that one is a FP, so I'm updating the issue. I think the enhancement needs some further discussion, so maybe we should create a separate ticket.

@Vampire
Copy link
Contributor Author

Vampire commented Feb 20, 2019

Maybe better a separate ticket for the FP to have the discussion so far here and rename this to the enhancement request?

@adangel adangel changed the title [java] false-positive in UselessParentheses [java] UselessParentheses false positive with conditional operator Nov 6, 2020
@adangel adangel linked a pull request Nov 6, 2020 that will close this issue
4 tasks
@adangel adangel added this to the 7.0.0 milestone Nov 6, 2020
@adangel
Copy link
Member

adangel commented Nov 6, 2020

Fixed with PMD 7

@adangel adangel closed this as completed Nov 6, 2020
@adangel adangel mentioned this issue Jan 23, 2023
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-positive PMD flags a piece of code that is not problematic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants