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

False positives for OperatorPrecedence bug pattern #4153

Closed
davido opened this issue Oct 19, 2023 · 3 comments
Closed

False positives for OperatorPrecedence bug pattern #4153

davido opened this issue Oct 19, 2023 · 3 comments

Comments

@davido
Copy link
Contributor

davido commented Oct 19, 2023

We are trying to migrate to the latest rules_java 7.0.6 version in Bazel, and seeing this 4 false positive breakages: [1].

The common pattern seems to be logical condition combined with ternary operator:

public void preferred(String e) {
    this.preferred = e != null && e.equals(email) ? true : null;
  }

that error prone suggesting to change like this:

public void preferred(String e) {
    this.preferred = (e != null && e.equals(email)) ? true : null;
  }

or:

info.canDelete =
              perm.ref(ref.getName()).testOrFalse(RefPermission.DELETE)
                      && projectState.statePermitsWrite()
                  ? true
                  : null;

that error prone suggesting to change like this:

info.canDelete =
              (permissionBackend.currentUser().ref(name).testOrFalse(RefPermission.DELETE)
                      && rsrc.getProjectState().statePermitsWrite())
                  ? true
                  : null;

To reproduce, clone Gerrit Code Review recursively with this CL applied: [2], and run:

  $> bazel build java/com/google/gerrit/server/...

[1] https://gerrit-review.googlesource.com/c/gerrit/+/390116
[2] https://gerrit-review.googlesource.com/c/gerrit/+/387360

@cushon
Copy link
Collaborator

cushon commented Oct 19, 2023

The change to OperatorPrecedence was 39d2884

Can you say more about why this is a false positive for you?

The suggest change is behaviour-preserving. The intent is that it makes it easier to read, which is of course subjective, the Google style guide suggest adding parens unless "there is no reasonable chance the code will be misinterpreted without them".

@davido
Copy link
Contributor Author

davido commented Oct 19, 2023

Thanks for the pointer, makes sense to me. May be worth extending the documentation, after 39d2884 and also mention ternary operator in addition: [1]?

Suggestion:

Use grouping parentheses to disambiguate expressions that contain
ternary operator with single || and &&,
both || and &&,
or both shift and arithmetic operators.

WDYT?

[1] https://docs.oracle.com/javase/tutorial/java/nutsandbolts/operators.html

@cushon
Copy link
Collaborator

cushon commented Oct 19, 2023

Updating the docs SG. I wonder about just providing some code examples, instead of trying to use prose to describe the cases it supports (or committing to which exact examples it will suggest parens for).

What do you think about something like

Use grouping parentheses to disambiguate expressions that could be
misinterpreted.

For example, consider this:

boolean d = (a && b) || c;",
boolean e = (a || b) ? c : d;",
int z = (x + y) << 2;",

Instead of this:

boolean r = a && b || c;",
boolean e = a || b ? c : d;",
int z = x + y << 2;",

copybara-service bot pushed a commit that referenced this issue Oct 19, 2023
#4153

PiperOrigin-RevId: 574878098
copybara-service bot pushed a commit that referenced this issue Oct 19, 2023
#4153

PiperOrigin-RevId: 574883084
@cushon cushon closed this as completed Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants