Repository navigation
Conversation
5478eb1 to
5b4aba4
Compare
Kotlin `property_declaration` inside a class parses as `variable_declaration → simple_identifier (+ user_type)`, which matches neither the `variable_declarator` shape Java uses, the C# `variable_declaration → variable_declarator` wrapper, nor PHP's `property_element` — and the identifier-only fallback misses it too (Kotlin's grammar has no field names). The result: Kotlin class properties produced NO node at all, so a `val x: Foo` receiver's declared type was invisible to the resolver and `x.method()` calls couldn't be typed. Add a Kotlin branch to extractField that reads the name from the `simple_identifier` and the type from its sibling, emitting a `"<Type> <name>"` signature (matching the Java/C# field shape that inferJavaFieldReceiverType already reads). Kotlin nullable `Foo?` is normalized to `Foo` so it resolves like the non-null type. Groundwork for same-name class disambiguation on Kotlin call sites (colbymchenry#314). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
extractJavaImports required a trailing `;`, which Java mandates but Kotlin omits (`import com.example.Foo`). So Kotlin produced ZERO import mappings and the receiver-FQN disambiguation signal (the `importedFqn` passed to resolveMethodOnType) never fired — same-name classes fell back to lexical order. Make the `;` optional; Java's mandatory semicolon still matches unchanged, and a Kotlin `as Alias` suffix still captures the FQN (alias unhandled, as in the existing Java path). Part of colbymchenry#314. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…chenry#314) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5b4aba4 to
96a6fd1
Compare
|
Rebased this branch onto current I re-checked that all three fixes are still relevant and not superseded by recent
@colbymchenry would you mind giving this a review when you get a chance? 🙏 |
…when two share a name When several classes share a name, resolveMethodOnType picks the one the call file imports by matching the import's path against the candidate's file path (com/example/b/FooConverter.kt). That holds for Java, where a public class names its file, but a Kotlin class often lives in a file named otherwise (FooConverter in Converters.kt), so no candidate matched and the call went to whichever class came first. The imported name is now matched against each candidate's qualified name first: import com.example.b.FooConverter expects the method com.example.b::FooConverter::convert, which the package directive gives every JVM declaration whatever its file is called. The file-path match stays as the fallback. The Java calls this once fixed, a Java file calling a Kotlin class it imports (okhttp's Java tests and mockwebserver3.MockWebServer, SystemUI's BlurUtils), are now resolved on main by the receiver's declared type (#1942), so on today's main the change shows in Kotlin. From #539 we take only this change and its test. Its other two parts, indexing Kotlin class properties and reading Kotlin imports without a trailing semicolon, have since landed on main another way. Validation: - resolution.test.ts: the Kotlin same-name case (#314's shape in Converters.kt files) is red on main, green here. - A/B against main 40f5851 (with the Java receiver typing of #1942 and the Kotlin changes of #1933, #1761 and #2258): no edge changes on okhttp, SystemUI, nowinandroid, mihon, jextract, spring-modulith, Minestom, spring-ai, kestra-java, playframework, click or gin; none of them has the test's shape on a resolved call. Against main 4eb8c2a, before #1942, it moved the 8 Java calls above onto the Kotlin class their file imports; main now reaches them at 0.9. - Cost (mihon, median of 3 interleaved indexes, on main 4eb8c2a): CPU 25.7 s on main vs 25.2 s, wall 8.2 s vs 8.3 s. Original PR #539 by @arttttt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @arttttt, and sorry this one waited so long! It landed in #3013, with you as the commit author: a Kotlin call now picks the class its file imports when two packages share a class name, even when that class lives in a file named something else, and your test covers it. Its other two parts had since landed on main another way, and the Java calls it also fixed are now handled by #1942, which types a Java call by its declared type, so we kept just the lookup change and its test. |
Summary
Follow-up to #314 / #472. #472 fixed same-name class disambiguation for Java call sites (
@Autowired FooConverter f; f.convert()resolves to the importedFooConverter, not whichever was found first). The equivalent Kotlin path was still broken — and the root cause turned out to be three stacked gaps, only the last of which #472 addressed.Probed the actual graph to confirm (not guessed): on a synthetic two-module Kotlin repro,
fooConverter.convert()withimport com.example.b.FooConverterresolved tocom.example.a— and there was no field node forfooConverterat all, soinferJavaFieldReceiverTypereturned null and #472'spreferredFqnmachinery never even ran for Kotlin.The three gaps
extractFieldhandled Javavariable_declarator, C#variable_declaration → variable_declarator, and PHPproperty_element— but Kotlinproperty_declarationparses asvariable_declaration → simple_identifier (+ user_type), matching none of them (and the identifier-only fallback misses it too, since Kotlin's grammar has no field names). Result: zero field nodes for Kotlin classes, so aval x: Fooreceiver's declared type was invisible to the resolver.extractJavaImportsrequired a trailing;, which Java mandates but Kotlin omits (import com.example.Foo). So Kotlin produced zero import mappings and the receiver-FQN disambiguation signal never fired.resolveMethodOnTypematched candidates by file-path suffix (com/example/b/FooConverter.kt) — fine for Java, but Kotlin commonly putsFooConverterinConverters.kt, so the suffix never matched. Now it prefers the candidate whose namespace-qualified name (com.example.b::FooConverter, from feat(jvm): resolve Java/Kotlin imports by fully-qualified name #412'spackage-directive wrapper) matches the import, falling back to the file-path suffix for Java's layout.Validation
Synthetic repro (the disambiguation proof): two modules each declaring
class FooConverterinConverters.kt. Swapping only the caller'simportline swaps the resolved target —import …a.FooConverter→a::FooConverter::convert,import …b.FooConverter→b::FooConverter::convert. Import-driven, not lexical-order-driven. Before the fix both resolved toa.Real repos (
nickbutcher/plaid258.kt,android/nowinandroid310.kt), indexed onmainvs this branch:fieldnodescallsedgesThe field-extraction fix is decisively validated — hundreds of previously-missing field nodes, with no node/edge explosion (methods stable, +~4% call edges, not multiples). On nia the +49 edges include call sites now correctly resolving to their test doubles (
NewsResourceDao::getNewsResources→TestNewsResourceDao::getNewsResources,UserDataRepository::…→FakeUserDataRepository::…) — a real precision gain that fell out of seeing the field's declared type.Honest scope note: the import-FQN disambiguation (gap 3) is proven on the synthetic repro. Neither real repo contained a true same-name-class-across-packages collision reached through the
import+ field-receiver shape, so its real-world impact is unmeasured there — it's correct-by-construction and showed zero regressions (no spurious target changes), but I'm not claiming a measured win for it on these two repos. Gaps 1 & 2 are the real-repo-proven part.Test plan
property_declaration→fieldnode with"<Type> <name>"signature (incl. nullableFoo?→Foo).Converters.kt) — call resolves to the imported package's class, not the lexically-first one.tsc --noEmitclean; one pre-existingmcp-rootstinypool/Node-24 worker-crash flake, passes in isolation).🤖 Generated with Claude Code