google / google/error-prone

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

Open
#5,845 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.