Skip to content

Do not treat missing type attribution as proof of semantic equality - #8333

Merged
sambsnyd merged 1 commit into
mainfrom
tim/semanticallyequal-missing-types
Aug 4, 2026
Merged

Do not treat missing type attribution as proof of semantic equality#8333
sambsnyd merged 1 commit into
mainfrom
tim/semanticallyequal-missing-types

Conversation

@timtebeek

@timtebeek timtebeek commented Jul 27, 2026

Copy link
Copy Markdown
Member

Problem

SemanticallyEqual considered this.x and a bare x equal whenever both sides lacked a JavaType.Variable, because TypeUtils.isOfType(null, null) is true. On an LST with incomplete type attribution a constructor's this.x = x therefore looks like a self-assignment, and RemoveSelfAssignment deletes it — 852 files across 83 repositories in the Netflix + Spring + Apache run, and where the field is private final the output no longer compiles.

Two paths reach it:

  • visitFieldAccess (the order RemoveSelfAssignment uses) guarded on fieldType != null && !fieldType.hasFlags(Static), so a null field type skipped the guard entirely.
  • the implicit-this shortcut in visitIdentifier, which recurses into an identifier-vs-identifier comparison that distinguishes a field from a parameter only by field type.

Fix

this.x is equivalent to x only when x resolves 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 the J.FieldAccess / J.Identifier comparison now require a field type on both sides.

The carve-out that matters: a null field type on a J.FieldAccess name also means "type reference" (java.util.regex.Pattern vs Pattern, java.lang.String.class vs String.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 null for fieldType at essentially every reference site (GroovyParserVisitor visitVariableExpression, parameters, visitPropertyExpression), so Groovy hit this bug unconditionally — a plain this.readOnly = readOnly in a Groovy constructor compared equal before this change. The new rewrite-groovy test covers both that case and the identifier-vs-identifier fallback that must keep working.

Tests

  • SemanticallyEqualTest.fieldNotEqualToShadowedNameVariableWithoutTypeAttribution (rewrite-java-test) — strips fieldType/type off identifiers to simulate an LST where attribution did not complete, since JavaParser in the test harness always attributes fully. This is why the existing RemoveSelfAssignment test suite could not reproduce the bug.
  • org.openrewrite.groovy.search.SemanticallyEqualTest (new) — the natural reproduction, plus the fallback that must stay true.

Both fail on main and pass here. :rewrite-java:test, :rewrite-java-test:test, :rewrite-java-tck:test, :rewrite-groovy:test and :rewrite-kotlin:test are green.

Follow-ups (not in this PR)

Tracked downstream in openrewrite/rewrite-static-analysis#953; none of this belongs in rewrite-java.

  • Verifying RemoveSelfAssignment needs a real LST. Green unit tests are not evidence here — RewriteTest/JavaParser always attribute fully, which is why the recipe's own doNotChangeFieldAssignedFromParameter passed the whole time the bug was live. The check that works is mod build over a repo with unresolved dependencies and reading fix.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$CommentVisitor does not need this change, despite the "apply bug fixes here too" note on SemanticallyEqual's javadoc. Its visitFieldAccess/visitIdentifier require both sides to be the same node type, so it has neither the cross-shape branch nor the implicit-this shortcut that were unsound here.
  • A residual hole remains that this PR cannot close. Two bare identifiers with the same name and no type information on either side are genuinely indistinguishable, so a field and a same-named local still compare equal. That is where a NoMissingTypes precondition on the deletion-gating recipes would help — 17 recipes in rewrite-static-analysis call SemanticallyEqual and none has one. Worth triaging by exposure rather than applying uniformly: FindMissingTypes has 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 or ty server.
  • The real fix for "missing vs systemically absent" is a signal on the LST. Today there is none: neither SourceFile nor JavaSourceFile exposes a language id, no parser emits a "types unavailable" marker, and TreeVisitor#getLanguage() lives on the visitor (and JavaScriptVisitor returns "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.

`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
@github-project-automation github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Aug 4, 2026
@sambsnyd
sambsnyd merged commit 5e871f9 into main Aug 4, 2026
1 check passed
@sambsnyd
sambsnyd deleted the tim/semanticallyequal-missing-types branch August 4, 2026 22:40
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants