Do not treat missing type attribution as proof of semantic equality - #8333
Merged
Conversation
`SemanticallyEqual` considered `this.x` and a bare `x` equal whenever both sides lacked a `JavaType.Variable`, because `TypeUtils.isOfType(null, null)` is `true`. On LSTs with incomplete type attribution that made a constructor's `this.x = x` look like a self-assignment, and `RemoveSelfAssignment` deleted it; where the field is `private final` the result no longer compiles. `this.x` is equivalent to `x` only when `x` resolves to the field rather than to a shadowing parameter or local, which cannot be established without type information in any language. Both directions of the `J.FieldAccess` / `J.Identifier` comparison now require a field type on both sides, keeping the carve-out for type references such as `java.util.regex.Pattern` vs `Pattern`, where a null field type is expected rather than missing. Comparisons between two identifiers still fall back to name equality, which is what languages that never attribute field types rely on. See openrewrite/rewrite-static-analysis#953
sambsnyd
approved these changes
Aug 4, 2026
timtebeek
added a commit
to openrewrite/rewrite-static-analysis
that referenced
this pull request
Aug 5, 2026
* RemoveSelfAssignment: cover incomplete type attribution Regression tests for openrewrite/rewrite#8333, which stopped SemanticallyEqual treating a null fieldType on both sides as proof of equality. The Groovy case needs no harness setup, since the Groovy parser passes null for fieldType at essentially every reference site; the Java case strips attribution to simulate an LST where it did not complete. See #953 * Do not delete expressions that may have side effects Four recipes gated deletion on a comparison that ignores what the deleted code does, so removing it changed behaviour: - RemoveUnconditionalValueOverwrite dropped the receiver, key and value the overwritten call would have evaluated, so `m.put(k, register())` lost the register() call. - AllBranchesIdentical discarded the condition when collapsing a chain, so `if (it.next() != null) p("x"); else p("x");` stopped advancing the iterator. - RemoveDuplicateConditions treated syntactically identical conditions as unreachable, dropping a branch that a non-idempotent condition would reach. - AvoidBoxedBooleanExpressions rewrote `!b` outside any control position, where visitExpression already guards on isControlExpression but visitUnary did not, turning a would-be NullPointerException into false. The purity check SimplifyRedundantLogicalExpression already carried moves to SideEffects so all of them share it. Fixes #953
eoliphan
added a commit
to eoliphan/rewrite
that referenced
this pull request
Aug 18, 2026
NOT A MERGE CANDIDATE. This exists to price the options and to surface the coupling a design discussion would otherwise find late. The representation is a decision about the type model and is not made here. A declared `any`, a declared `unknown`, and a genuine attribution failure all produced the same payload-free unknown value. TypeScript already distinguishes them — `intrinsicName` is read to isolate `'error'` — so only a destination was missing. This maps the two declared forms to reserved-name classes, following the existing `FUNCTION_TYPE_NAME` convention. A `Type.Class` crosses the RPC boundary as the already-known `JavaType$Class` string, so no new Java type and no Java model change is required. The open question this raises is the reason it is a prototype rather than a patch. `MethodMatcher` degrades an unknown type to `"*"`, a wildcard that matches anything, so a parameter declared `any` currently matches by wildcard and under any nominal representation would stop matching. Whether `any` is a wildcard or a nominal type is therefore the real decision, and `MethodMatcher` has already answered it for the unknown type. Two further interactions worth weighing: - `SemanticallyEqual.isTypeReference` requires a fully-qualified type that is not the unknown type, so it flips to true for identifiers typed `any`. This makes a previously uninformative value informative, which is the concern openrewrite#8333 raised. - `FindMissingTypes` would stop reporting a declared `any` as missing. That is the intended outcome. Known gap: serialization compatibility is not addressed. A patched parser and an unpatched consumer disagree, and nothing validates or rejects that. Relates to openrewrite#8534
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SemanticallyEqualhalf of Deletion-gating recipes lack purity guards, and untyped identifier comparisons remain unsound rewrite-static-analysis#953.Problem
SemanticallyEqualconsideredthis.xand a barexequal whenever both sides lacked aJavaType.Variable, becauseTypeUtils.isOfType(null, null)istrue. On an LST with incomplete type attribution a constructor'sthis.x = xtherefore looks like a self-assignment, andRemoveSelfAssignmentdeletes it — 852 files across 83 repositories in the Netflix + Spring + Apache run, and where the field isprivate finalthe output no longer compiles.Two paths reach it:
visitFieldAccess(the orderRemoveSelfAssignmentuses) guarded onfieldType != null && !fieldType.hasFlags(Static), so a null field type skipped the guard entirely.thisshortcut invisitIdentifier, which recurses into an identifier-vs-identifier comparison that distinguishes a field from a parameter only by field type.Fix
this.xis equivalent toxonly whenxresolves to the field rather than to a shadowing parameter or local. That cannot be established without type information in any language, so both directions of theJ.FieldAccess/J.Identifiercomparison now require a field type on both sides.The carve-out that matters: a null field type on a
J.FieldAccessname also means "type reference" (java.util.regex.PatternvsPattern,java.lang.String.classvsString.class), where null is expected rather than missing. Those keep comparing by name.Comparisons between two identifiers are untouched and still fall back to name equality — that is what languages which never attribute field types rely on, so this is not a coverage regression for them.
Why this is not Java-only
The Groovy parser passes
nullforfieldTypeat essentially every reference site (GroovyParserVisitorvisitVariableExpression, parameters,visitPropertyExpression), so Groovy hit this bug unconditionally — a plainthis.readOnly = readOnlyin a Groovy constructor compared equal before this change. The newrewrite-groovytest covers both that case and the identifier-vs-identifier fallback that must keep working.Tests
SemanticallyEqualTest.fieldNotEqualToShadowedNameVariableWithoutTypeAttribution(rewrite-java-test) — stripsfieldType/typeoff identifiers to simulate an LST where attribution did not complete, sinceJavaParserin the test harness always attributes fully. This is why the existingRemoveSelfAssignmenttest suite could not reproduce the bug.org.openrewrite.groovy.search.SemanticallyEqualTest(new) — the natural reproduction, plus the fallback that must staytrue.Both fail on
mainand pass here.:rewrite-java:test,:rewrite-java-test:test,:rewrite-java-tck:test,:rewrite-groovy:testand:rewrite-kotlin:testare green.Follow-ups (not in this PR)
Tracked downstream in openrewrite/rewrite-static-analysis#953; none of this belongs in rewrite-java.
RemoveSelfAssignmentneeds a real LST. Green unit tests are not evidence here —RewriteTest/JavaParseralways attribute fully, which is why the recipe's owndoNotChangeFieldAssignedFromParameterpassed the whole time the bug was live. The check that works ismod buildover a repo with unresolved dependencies and readingfix.patch(apache/geronimo-specs: 193 files, expected 0 after this change), or a unit test that strips attribution the way the new test here does.CombineSemanticallyEqualCatchBlocks$CommentVisitordoes not need this change, despite the "apply bug fixes here too" note onSemanticallyEqual's javadoc. ItsvisitFieldAccess/visitIdentifierrequire both sides to be the same node type, so it has neither the cross-shape branch nor the implicit-thisshortcut that were unsound here.NoMissingTypesprecondition on the deletion-gating recipes would help — 17 recipes in rewrite-static-analysis callSemanticallyEqualand none has one. Worth triaging by exposure rather than applying uniformly:FindMissingTypeshas no language gate, so such a precondition flags Groovy identifiers en masse and silently disables the recipe for Groovy, and for C#/Python LSTs parsed without a semantic model ortyserver.SourceFilenorJavaSourceFileexposes a language id, no parser emits a "types unavailable" marker, andTreeVisitor#getLanguage()lives on the visitor (andJavaScriptVisitorreturns"org/openrewrite/javascript", which looks like a bug). Both the Python and C# parsers already know at parse time that they are degrading to all-null and could record it.