Replace shallow class mapping in parsers with proper type attribution#7861
Merged
Conversation
Several parser code paths used JavaType.ShallowClass.build(fqn) directly
— for dotted-name expressions where each segment becomes a J.FieldAccess
(rewrite-java-{8,11,17,21,25} + rewrite-groovy), and for dynamic
parameter / catch-block / NoClassDefFoundError fallbacks in rewrite-groovy.
ShallowClass.build allocates a fresh JavaType.Class every call —
bypassing the parser's configured JavaTypeFactory entirely. With a
type-table-backed factory, this means a single parser session emits
multiple non-canonical instances for the same FQN: the symptom that
surfaced this was two distinct java.lang.Object proxies showing up in
one build.gradle parse, and later a JavaType.ShallowClass for
org.gradle.plugins.ide.eclipse.model.Classpath colliding with the
factory-produced JavaType.Class for the same FQN.
Each affected TypeMapping (ReloadableJava{8,11,17,21,25}TypeMapping,
GroovyTypeMapping) now exposes classFor(fqn) which routes through
typeFactory.computeClass — the factory's cache dedupes per signature,
and a type-table-backed factory returns the full body it carries (the
helper is NOT necessarily a "shallow" class, the prior name was
misleading). All parser-side ShallowClass.build call sites switch
over.
The Groovy-side NoClassDefFoundError fallback in GroovyTypeMapping.type
also goes through classFor — same identity-stability reasoning, since
e.getMessage() is the FQN of the missing class and the factory will
synthesize a canonical stub for it.
The previous commit introduced JavaTypeFactory.classFor as a shim that
canonicalized ShallowClass.build call sites into the factory's cache.
That dedup'd identity but didn't fix the underlying issue: ShallowClass
mapping was used in many places where the parser actually had richer
information available — Symbols, ClassNodes, ImportNode chains, etc. —
and was choosing a string-keyed stub instead.
This commit walks each parser and replaces classFor with whichever of
these is appropriate at each call site:
* typeMapping.type(astNode) — when the parser holds a Symbol / FIR
node / IrClass / Groovy ClassNode that the existing classType path
can populate fully
* reflectionTypeMapping.type(Class.forName(fqn)) — when the FQN names
a real Java class we can resolve via reflection (Kotlin's
kotlin.String -> java.lang.String remap, Scala's
Constants.StringTag, Kotlin's synthesized String method parameter)
* typeFactory.computeClass(fqn, flags, kind) { /* empty */ } — when
we need a canonical identity-stable Class for an FQN that has no
backing AST node (JVM file facades, JvmPackagePartSource library
facades, Scala library types we don't want to recurse into)
* JavaType.Unknown.getInstance() — when the thing isn't a class at
all (Kotlin package directives, IrExternalPackageFragment,
unresolvable JavaClassifierType classifiers, the synthetic
"kotlin.Library" placeholder)
After this pass, no caller of classFor remained, so the method is
deleted from JavaTypeFactory and its implementations. TypeTree.build's
3-arg form (introduced in the prior commit) is likewise gone.
Specific notable changes:
* Groovy J.Import qualid now walks the ImportNode's outer-class chain
segment-by-segment, so `import com.foo.Outer.Inner` attributes
Outer and Inner to their real ClassNodes instead of routing the
string through TypeTree.build.
* Kotlin file facade classes (FooKt) now carry their declared
top-level functions on the canonical Class. MethodMatcher patterns
against Kotlin top-level functions work for the first time.
* Java parser visitors lose their buildName helper, which was dead
code reachable only from visitTypeParameter with a single-segment
name.
* KotlinTypeMappingTest test expectations updated where the new
behavior is more correct: arrayOf's declaringType is {undefined}
(no real class, kotlin.Library was a placeholder string), and
operator expressions on Kotlin primitives uniformly surface as the
JVM primitive int rather than splitting between unary (int) and
binary/assignOp (kotlin.Int).
Drops the JavaTemplateParser TYPE_FACTORY_KEY Cursor-message mechanism in favor of a transient typeFactory field on JavaSourceSet. Build pipelines that already attach a JavaSourceSet marker (matching the source file's classpath) attach the factory at the same time; JavaTemplate reads it via the enclosing source file's marker. Eliminates the indirection that required producers to populate a magic key on every visitor's cursor. When the marker is absent or carries no factory (free-floating Groovy/Gradle scripts, V2 LSTs, synthetic test CUs) JavaTemplate falls back to a fresh DefaultJavaTypeFactory, matching prior behavior. Also fixes the licenseTest break introduced in 96a0ec3 by giving rewrite-java-test's template package-info.java the standard Apache header and switching to the project's @NullMarked/@NonNullFields convention.
…s chain `buildImportQualid` aligned the import's class segments by walking `ImportNode.getType().getOuterClass()`. For a resolved nested-class import that chain is unreliable: `getOuterClass()` returns null even for a nested type, so the chain had a single element while the package-segment count was derived from the (multi-segment) package name. The two disagreed, which for `import java.util.Map.Entry` left the leaf `Entry` with a null type (a regression from the prior shallow type) and wrongly placed `Map$Entry`'s type on the `Map` segment. Walk the attributed leaf type's `owningClass` links instead — proper type attribution populates these from the JVM enclosing-class chain. Each class segment now carries its own resolved type, package segments carry none, and unresolvable leaves (asFullyQualified == null) fall back to no type rather than to `<unknown>`. Behavior for flat, static, and star imports is unchanged. Adds a regression test asserting per-segment attribution for a nested import. Also remove the unused `org.openrewrite.java.tree.Flag` import left behind in the five ReloadableJava*TypeMapping files.
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
3 participants
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.
Summary
Two commits, one story: we shipped
JavaTypeFactory.classForas a canonicalizing shim forShallowClass.build, then walked every parser and discovered most of those sites didn't actually need a shallow class — they had a real Symbol,ClassNode,ImportNodechain, FIR/IR element, etc. they could route through proper type attribution instead. The shim is gone after this PR.Per-site routing summary:
typeMapping.type(astNode)→ the existingclassTypepathreflectionTypeMapping.type(Class.forName(fqn))typeFactory.computeClass(fqn, …) { /* empty */ }JavaType.Unknown.getInstance()After this pass,
classForhas no callers and is deleted fromJavaTypeFactory. The 3-argTypeTree.buildform added in the prior commit is also deleted.Notable behavioral wins
FooKt) now carry their declared top-level functions on the canonical Class.MethodMatcherpatterns against Kotlin top-level functions work properly for the first time.import com.foo.Outer.Inner) walkImportNode.getType()'s outer-class chain segment-by-segment instead of going through a string-keyedTypeTree.build, soOuterandInnereach carry their real resolved type.intfor unary, binary, and assignment-operator forms (resolving a documented inconsistency inKotlinTypeMappingTest.operatorOverload).buildNamehelper, reachable only fromvisitTypeParameterwith single-segment names, is gone.Status
Draft because the broader spring-boot build that motivated this work still trips on two pre-existing parser-side bugs (duplicate
TAG_CLASSfororg.gradle.api.Incubating, empty-stub forBuildResult.getOutput) that are independent of the classFor work. Investigation ongoing; expect additional commits.Test plan
:rewrite-java:test:rewrite-java-{8,11,17,21,25}— all compile:rewrite-groovy:test:rewrite-scala:test:rewrite-gradle:test:rewrite-kotlin:test(with test expectation updates noted above)moderne-cliV3 build pipeline end-to-end