Deletion-gating recipes lack purity guards, and untyped identifier comparisons remain unsound

[timtebeek]

The original report here — RemoveSelfAssignment deleting this.x = x constructor parameter assignments on incompletely attributed LSTs — was fixed upstream by openrewrite/rewrite#8333, which stopped SemanticallyEqual treating a null fieldType on both sides as proof of equality. Verified against rewrite-java 8.89.0-20260804.233037-32: the apache/geronimo-specs reproduction (193 files, 514 this.x = … deletions) no longer occurs, and the full rewrite-static-analysis suite is green.

This issue now tracks only what that fix did not address.

1. Missing purity guards

Four recipes delete statements or branches whose removal changes behaviour, because the comparison that gates the deletion ignores side effects in the code being dropped. These are independent of type attribution — each reproduces with full type information, so they are ordinary missing purity guards rather than instances of the original bug. All four re-verified as reproducing today on 2.41.0-SNAPSHOT.

RemoveUnconditionalValueOverwrite drops a side-effecting argument

void test(Map<String, Integer> m) {
    m.put("k", register());   // deleted; register() is never called
    m.put("k", 2);
}

The first statement is removed because its value is unconditionally overwritten, but the removed expression is not pure.

AllBranchesIdentical drops a side-effecting condition

void test(Iterator<String> it) {
    if (it.next() != null) {  // condition discarded; the iterator never advances
        p("x");
    } else {
        p("x");
    }
}

Collapsing to p("x"); is only valid when the condition is pure.

RemoveDuplicateConditions assumes a repeated call is idempotent

boolean advance() { return calls++ == 1; }

void test() {
    if (advance()) {
        p("a");
    } else if (advance()) {   // branch dropped as a "duplicate condition"
        p("b");
    }
}

The two calls are syntactically identical but not semantically equal. The original prints b; the result prints nothing.

AvoidBoxedBooleanExpressions rewrites outside any control position

void test(Boolean b) {
    boolean x = !b;   // becomes Boolean.FALSE.equals(b)
}

visitExpression gates its rewrite on isControlExpression (AvoidBoxedBooleanExpressions.java:51), but visitUnary (AvoidBoxedBooleanExpressions.java:60) has no such guard. In an if or ternary the substitution preserves meaning; in an assignment it converts a would-be NullPointerException into false.

None of these four is currently wired into CommonStaticAnalysis or any other recipe collection, so exposure is limited to direct invocation — but they are published and runnable.

2. Residual: two untyped identifiers still compare equal

openrewrite/rewrite#8333 closed the J.FieldAccess vs J.Identifier case. Two bare identifiers with the same name and no type information on either side remain genuinely indistinguishable, so a field and a same-named local still compare equal. In Java this is hard to reach — a bare x = x always resolves both sides to the same declaration — but it is reachable in languages whose parsers never attribute field types.

17 recipes in this module call SemanticallyEqual and none carries a NoMissingTypes precondition. Worth noting that applying one uniformly is not the answer: FindMissingTypes has no language gate, so such a precondition flags Groovy identifiers en masse and would silently disable those recipes for Groovy, and for C#/Python LSTs parsed without a semantic model. Triage by exposure — the risk concentrates where SemanticallyEqual gates deletion of a statement or branch.

3. There is still no LST signal for "types missing" vs "types systemically absent"

Neither SourceFile nor JavaSourceFile exposes a language id, no parser emits a "types unavailable" marker, and TreeVisitor#getLanguage() lives on the visitor. Both the Python and C# parsers already know at parse time that they are degrading to all-null and could record it. Without that signal, recipes cannot distinguish an LST where attribution failed from one where it was never available, which is what makes item 2 hard to guard correctly.

Verification note

Green unit tests are a weak oracle for the type-attribution class of bug: RewriteTest/JavaParser always attribute fully, which is why RemoveSelfAssignment's own doNotChangeFieldAssignedFromParameter passed the entire time the original bug was live. Two things that do work: a Groovy test case (the Groovy parser passes null for fieldType at essentially every reference site, so the condition arises naturally), or stripping fieldType/type in a mapBeforeRecipe hook. Note also that the Moderne CLI runs recipes on its own bundled rewrite-java, so validating a rewrite-java-level fix through mod run requires a CLI release carrying it.