Do not treat missing type attribution as proof of semantic equality - #8333
Open
timtebeek wants to merge 1 commit into
Open
Do not treat missing type attribution as proof of semantic equality#8333timtebeek wants to merge 1 commit into
timtebeek wants to merge 1 commit into
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
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 RemoveSelfAssignment deletesthis.x = xconstructor parameter assignments when type attribution is incomplete 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.