Skip to content

Fix TypeTable round-trip for Kotlin top-level functions#7422

Merged
steve-aom-elliott merged 5 commits intomainfrom
fix/kotlin-top-level-method-type
Apr 27, 2026
Merged

Fix TypeTable round-trip for Kotlin top-level functions#7422
steve-aom-elliott merged 5 commits intomainfrom
fix/kotlin-top-level-method-type

Conversation

@steve-aom-elliott
Copy link
Copy Markdown
Contributor

Summary

Three interlocking bugs in TypeTable caused Kotlin top-level functions (top-level regular functions and extension functions) to lose method type attribution when KotlinParser received a classpath sourced from a TypeTable-synthesized directory (as mod does for downloaded LSTs) rather than the original jar. The symptom: J.MethodInvocation.getMethodType() returns null for calls like jacksonObjectMapper() (from jackson-module-kotlin), and MethodMatcher/UsesMethod therefore cannot match them.

The three fixes

  1. ACC_SYNTHETIC methods were filtered out. Kotlin extension functions compile to public static final synthetic on a *Kt facade class, so they were being dropped at write time. Now filter on ACC_PRIVATE only (and skip <clinit>), matching what's needed for type attribution.

  2. InnerClasses attribute was not captured. Kotlin FIR silently fails to resolve methods on a class whose @kotlin.Metadata.d1 protobuf references inner classes (lambdas, companion objects, etc.) unless those inner classes are declared in the class's InnerClasses attribute. The Writer's ClassVisitor didn't override visitInnerClass, so every round-tripped class came out with an empty InnerClasses attribute. Now serialize each entry as name,outerName,innerName,access into a new innerClasses TSV column, and replay them on the Reader side before any other class events so ASM writes them back out.

  3. META-INF/*.kotlin_module resources were dropped. The Kotlin compiler uses these files to map package names to their facade classes (com.fasterxml.jackson.module.kotlinExtensionsKt). Without it, the resolver can't find the top-level function's owner. Store .kotlin_module files as resource rows using a classAccess = -2 sentinel with base64-encoded content in the constantValue column, plumbed through a new ResourceConsumer callback on Reader.read() so callers can persist them alongside the synthesized .class files.

Tests

  • MethodMatcherTest#libraryTopLevelFunctionPopulatesMethodType — asserts method type is non-null for jacksonObjectMapper().
  • MethodMatcherTest#usesMethodMatchesLibraryTopLevelFunction — end-to-end UsesMethod match on the Kotlin top-level extension function.
  • TypeTableTypeAnnotationsTest#columnOrderCorrect — updated for the added column.

Both Kotlin tests use KotlinParser.builder().classpath(\"jackson-module-kotlin\"), which resolves via the shipped TypeTable — exactly the code path that was broken.

Test plan

  • ./gradlew :rewrite-java:test --tests "org.openrewrite.java.internal.parser.*"
  • ./gradlew :rewrite-kotlin:test --tests "org.openrewrite.kotlin.MethodMatcherTest"
  • Full CI

Copy link
Copy Markdown
Member

@sambsnyd sambsnyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation makes sense and the fixes look reasonable.

The various places where loosely typed columns are accessed by magic indexes was already there before this PR. But code like:

                            // Resource row — see Writer.Jar.writeResource
                            String resourcePath = fields[4];
                            String base64 = fields.length > 17 ? fields[17] : "";

or

                        String artifactVersion = fields[1] + "-" + fields[2];

would be more readable and maintainable if we had some struct or view which gave each field a name and proper type rather than just an index and String.

Comment on lines +745 to +755
// Resource row: classAccess sentinel -2, className holds the path, content is
// base64-encoded in the constantValue column. All other columns are empty.
out.printf(
"%s\t%s\t%s\t%d\t%s\t%s\t%s\t%s\t%d\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s%n",
groupId, artifactId, version,
-2, resourcePath,
"", "", "",
-1, "", "", "", "", "",
"", "", "",
Base64.getEncoder().encodeToString(content),
"");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with the comment this feels a bit opaque, constants sprinkled amongst empty strings like a magical incantation.
I'd rather see a struct with a builder which has a toString, so this could be written more legibly.

Maybe something like:

out.printf(ResourceRow.builder()
        .groupId(groupId)
        .artifactId(artifactId)
        .verison(version)
        .type(ResourceRow.Type.Resource)
        .path(resourcePath)
        .toString()

Three bugs in TypeTable caused Kotlin top-level extension functions (e.g.
`jacksonObjectMapper()` from `jackson-module-kotlin`) to lose method type
attribution when their classpath was sourced from a synthesized TypeTable
rather than the original jar:

1. Writer filtered out `ACC_SYNTHETIC` methods. Kotlin extension functions
   compile to `public static final synthetic` on a `*Kt` facade class, so
   they were being dropped. Filter on `ACC_PRIVATE` only.

2. Writer did not capture `InnerClasses` attributes. Kotlin FIR silently
   fails to resolve methods on classes whose `@kotlin.Metadata.d1`
   references inner classes (lambdas, etc.) that are not declared in the
   class's `InnerClasses` attribute. Serialize `visitInnerClass` entries
   into a new `innerClasses` TSV column and replay them on the reader.

3. Writer did not preserve `META-INF/*.kotlin_module` resources, which
   the Kotlin compiler uses to map package names to facade classes.
   Store them as resource rows (`classAccess=-2` sentinel, base64 content)
   and expose them through a new `ResourceConsumer` callback on the
   reader.
Introduce a single TsvRow @Value/@builder class that owns the canonical
column order and replaces the printf-based writer call sites and the
fields[N] reader. A Kind enum carries the negative-int sentinels (-1 for
class-only rows, -2 for resource rows) so callers no longer reference
magic constants. The on-disk TSV format and column count are unchanged.
@steve-aom-elliott steve-aom-elliott force-pushed the fix/kotlin-top-level-method-type branch from ef9de3c to e7f823a Compare April 27, 2026 14:42
The Lombok-generated TsvRowBuilder is not visible during javadoc since
javadoc does not run annotation processors. Inline .build() in the
factories so the public signature only references TsvRow.
Centralize the comma-delimited 'name,outerName,innerName,access' format
in one InnerClassRef class so the writer and reader no longer share a
positional split.
@steve-aom-elliott steve-aom-elliott marked this pull request as ready for review April 27, 2026 16:52
@steve-aom-elliott steve-aom-elliott added bug Something isn't working test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail kotlin labels Apr 27, 2026
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Apr 27, 2026
@steve-aom-elliott
Copy link
Copy Markdown
Contributor Author

Going to add one last update to the actual tests that I forgot, but other than that, should be converted

Replace tab-split + magic-index assertions with named-field access via
TsvRow.parse. The columnOrderCorrect test keeps positional indices since
pinning the on-disk column layout is its purpose.
@steve-aom-elliott steve-aom-elliott merged commit eacbe36 into main Apr 27, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Apr 27, 2026
@steve-aom-elliott steve-aom-elliott deleted the fix/kotlin-top-level-method-type branch April 27, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working kotlin test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants