Drop receivers only when they have no observable effect

[martinfrancois]

Suggested review order: 28 of 52 (Score: 3.5)
Review first: openrewrite/rewrite-static-analysis#975

What's changed?

ChangeMethodTargetToStatic now rewrites a call only when the receiver can be deleted without the running program noticing. On main every matched receiver is replaced by the target type name, whatever it does.

Java requires the receiver to be evaluated: per JLS 15.12.4.1 the qualifier of Primary.method(...) is evaluated and its value discarded even when the invoked method is static, and per JLS 15.13.3 the qualifier of a bound method reference is evaluated and null checked when the reference is created. Main's output still compiles and still returns the same value, so only what the program does while it runs changes.

A receiver is still deleted when it is a type name, this (including Outer.this), a literal, a non-volatile variable read, a non-volatile field read qualified by a type name or this, or a new A(...) with no anonymous body, no enclosing instance and arguments that are themselves droppable. Every other form now stops the rewrite: an unmatched invocation, a field access on another expression such as other.field, an array access, a cast.

For a member reference the rule is stricter: the qualifier is replaced only when it is a type name, this, or a call this recipe rewrites away itself, since creating the reference already evaluates and null checks it.

Chains still collapse - with a.A value() retargeted to b.B, legacy.value().value() becomes B.value() - but only when the collapsed qualifier's own receiver and arguments are droppable. legacy.combine(argument()).value() is now left alone rather than losing argument().

What's your motivation?

Recipe: org.openrewrite.java.ChangeMethodTargetToStatic.

Before

return modifier().isPublic(1);

Actual after the recipe

return Modifier.isPublic(1);

Expected after the recipe

(unchanged)

Anything in particular you'd like reviewers to focus on?

No existing test expectation changed, so the existing tests show nothing of the rewrites the recipe no longer performs. Those are a real loss of coverage: a bound method reference on a variable, a::value, is no longer retargeted to B::value, because creating a::value null checks a while B::value does not.

Three recipes call this one, and all three keep working. MockUtilsToStatic in rewrite-testing-frameworks rewrites new MockUtil().isMock(x) and a MockUtil held in a local or a field - an instantiation and a variable read, both still droppable. RemovedToolProviderConstructor and RemovedModifierAndConstantBootstrapsConstructors, inside UpgradeToJava17 in rewrite-migrate-java, target javax.tools.ToolProvider, java.lang.reflect.Modifier and java.lang.invoke.ConstantBootstraps, whose usages take the same two shapes. The one shape they lose is the receiver that is itself a call, modifier().isPublic(1), which is the defect above.

Two receivers are still deleted although deleting them can be observed. Main deletes them too, so neither is a regression:

• new A().staticMethod() still loses the instantiation, though a constructor can run arbitrary code and throw. This is deliberate: it is the shape the recipe exists to rewrite, so treating an instantiation as undeletable would switch the recipe off for its main use.
• A bare static field name, INSTANCE.stat(), is a variable read and is still deleted, losing the class initialization JLS 12.4.1 requires on first use of a non-constant static field.

Have you considered any alternatives or workarounds?

One alternative is to keep the receiver and emit it as a separate receiver(); statement before the rewritten call. That is a much larger change: it has to decide whether the call sits somewhere a statement can be inserted at all - a field initializer or a ternary branch offers nowhere to put one - and for a member reference it has to generate an explicit null check, because the implicit one is lost once the qualifier becomes a type name. That work can follow this change; say so if you prefer it.

Any additional context

Adds 15 tests to ChangeMethodTargetToStaticTest, taking it from 12 to 27. Nine fail without the code change; the rest cover rewrites the recipe must still perform, showing the change does not narrow it too far.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch