Multiple checks are not parenthesis-invariant: matchers don't look through ParenthesizedTree

[Chordrain]

Error Prone version

Error Prone 2.49.0 (javac 21.0.8, --source/--target 21)

Affected checks

LoopConditionChecker, NonAtomicVolatileUpdate, PatternMatchingInstanceof, DuplicateBranches, InlineTrivialConstant, AssignmentExpression

Description

A number of unrelated checks share a single root cause: their AST matchers do not look through ParenthesizedTree. Per JLS §15.8.5, parentheses around an expression do not change its value, type, or constant-expression status — they are semantically inert. Wrapping (or unwrapping) an operand in redundant parentheses is therefore a no-op rewrite, yet across at least six checks it flips the verdict — silencing true findings (false negatives), and in one case manufacturing a false positive.

Each case below is a self-contained, minimal BEFORE/AFTER pair where the only difference is one pair of redundant parentheses. I verified all of them on Error Prone 2.49.0; the per-check counts are shown in the table.

Check	Rewrite	BEFORE	AFTER	Effect
LoopConditionChecker	i++ → (i)++	0	1	false positive
NonAtomicVolatileUpdate	x++ → (x)++	4	0	false negative
PatternMatchingInstanceof	(String) o → (String)(o)	1	0	false negative
DuplicateBranches	1 * 5 → (1 * 5)	1	0	false negative
InlineTrivialConstant	"" → ("")	1	0	false negative
AssignmentExpression	a = b = 0 ↔ a = (b = 0)	2	3	verdict drift (both directions)

The common fix is to skip/unwrap enclosing ParenthesizedTree nodes before matching the relevant operand (e.g. via ASTHelpers.stripParentheses on the operand of the increment/cast/branch/initializer/assignment being inspected).

Reproducers

1. LoopConditionChecker — (i)++ produces a false positive

(i)++ denotes the same variable update as i++, so the loop's condition variable i is still modified in the update. The check reports it as never modified (this manifests when the loop bound is a compile-time constant).

// BEFORE — no finding
public class LoopParen {
  void m() {
    for (int i = 0; i < 10; i++) {
      System.out.println(i);
    }
  }
}

// AFTER — false positive: "condition variable(s) never modified in loop body: i"
public class LoopParen {
  void m() {
    for (int i = 0; i < 10; (i)++) {
      System.out.println(i);
    }
  }
}

2. NonAtomicVolatileUpdate — (x)++ is missed

(x)++ is the same non-atomic read-modify-write of a volatile field as x++.

// BEFORE — 4 findings
public class Vol {
  volatile int x;
  volatile long y;
  void bad() { x++; y++; x--; y--; }
}

// AFTER — 0 findings
public class Vol {
  volatile int x;
  volatile long y;
  void bad() { (x)++; (y)++; (x)--; (y)--; }
}

3. PatternMatchingInstanceof — (String)(o) is missed

Per JLS §15.16 the parentheses around a cast operand are a no-op, but the instanceof-and-cast pattern is no longer recognized.

// BEFORE — 1 finding (suggests `if (o instanceof String s)`)
public class Sample {
  public int f(Object o) {
    if (o instanceof String) {
      String s = (String) o;
      return s.length();
    }
    return o.hashCode();
  }
}

// AFTER — 0 findings
public class Sample {
  public int f(Object o) {
    if (o instanceof String) {
      String s = (String)(o);
      return s.length();
    }
    return o.hashCode();
  }
}

4. DuplicateBranches — (1 * 5) defeats the branch comparison

The two ternary branches remain identical; only one is parenthesized.

// BEFORE — 1 finding
public class Sample {
  int x;
  void m() { x = true ? 1 * 5 : 1 * 5; }
}

// AFTER — 0 findings
public class Sample {
  int x;
  void m() { x = true ? 1 * 5 : (1 * 5); }
}

5. InlineTrivialConstant — ("") is missed

// BEFORE — 1 finding
public class Sample {
  private static final String EMPTY = "";
}

// AFTER — 0 findings
public class Sample {
  private static final String EMPTY = ("");
}

6. AssignmentExpression — verdict is not parenthesis-invariant

The check flags an assignment used inside a larger expression, but the result depends on redundant parentheses around the inner assignment — in both directions: a = b = 0 is not flagged while a = (b = 0) is, whereas result = (bresult = new byte[len]) is flagged while result = bresult = new byte[len] is not.

// BEFORE — 2 findings (on `int i = j = 0;` and `result = (bresult = ...)`)
public class Sample {
  int a, b, j, l;
  byte[] bresult;
  void m1() { a = b = 0; }                        // NOT flagged
  void m2() { if ((a = b = 0) != 1) {} }          // NOT flagged
  void m3() { int i = j = 0; }                    // flagged
  void m4(int len) {
    int k = (l += 1);                             // NOT flagged
    byte[] result;
    result = (bresult = new byte[len]);           // flagged
  }
}

// AFTER — 3 findings; per-site verdicts flip as parentheses are added/removed
public class Sample {
  int a, b, j, l;
  byte[] bresult;
  void m1() { a = (b = 0); }                      // now flagged
  void m2() { if ((a = (b = 0)) != 1) {} }        // now flagged
  void m3() { int i = (j = 0); }                  // flagged
  void m4(int len) {
    int k = l += 1;                               // NOT flagged
    byte[] result;
    result = bresult = new byte[len];             // now NOT flagged
  }
}

Expected behavior

Each check should produce the same findings on the BEFORE and AFTER of every pair above. Redundant parentheses do not change the value, type, or constant-ness of the wrapped expression, so they must not change any check's verdict.

Actual behavior

See the table: each rewrite that only adds or removes one pair of redundant parentheses changes the finding set — five checks drop true findings, one (LoopConditionChecker) raises a false positive, and AssignmentExpression drifts in both directions. The shared cause is that the matchers inspect the raw operand AST instead of unwrapping enclosing ParenthesizedTree nodes.