Multiple checks are not parenthesis-invariant: matchers don't look through ParenthesizedTree
- Dominant language
- Java
- Stars
- 7.2k
- Forks
- 820
- Avg merge
- 5h 9m
- Merged PRs (30d)
- 50
Description
### 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).
```java
// BEFORE — no finding
public class LoopParen {
void m() {
for (int i = 0; i < 10; i++) {
System.out.println(i);
}
}
}
```
```java
// 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++`.
```java
// BEFORE — 4 findings
public class Vol {
volatile int x;
volatile long y;
void bad() { x++; y++; x--; y--; }
}
```
```java
// 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.
```java
// 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();
}
}
```
```java
// 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.
```java
// BEFORE — 1 finding
public class Sample {
int x;
void m() { x = true ? 1 * 5 : 1 * 5; }
}
```
```java
// AFTER — 0 findings
public class Sample {
int x;
void m() { x = true ? 1 * 5 : (1 * 5); }
}
```
#### 5. `InlineTrivialConstant` — `("")` is missed
```java
// BEFORE — 1 finding
public class Sample {
private static final String EMPTY = "";
}
```
```java
// 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.
```java
// 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
}
}
```
```java
// 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.
Contributor guide
Assessment
This issue has not been assessed yet.