From d4c19a513c7590ced74853aade2484d63410c4d9 Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Wed, 16 Sep 2026 09:58:21 +0200 Subject: [PATCH 1/7] docs: extension-registry follow-up cluster plan (draft 2) Plan for the 5 items parked out of PR #321: resolve_chain tail reorder for KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS names (List/MutableList/ MutableSet/MutableMap/MutableCollection/MutableIterable/MutableIterator losing a tie-break or a unique-match shortcut to a kotlin-stdlib JAR decoy), Kotlin-default-import-package awareness for extension-scope checking (isNotEmpty/forEach rejected as out of scope), the identical first-match bug PR #321 already fixed elsewhere still present in implicit_receiver_extension_match, the same family in find_extension_fn_return_type_scoped plus a separate early-abort bug, and a closed decision on rename.rs's new ambiguous-refusal behavior. Independently critiqued (blocker found: Task 4's original test fixtures were false-green and one was unfixable as designed) and revised with the blocker's fix verified by an actual red-then-green test run before finalizing, not just designed. Co-Authored-By: Claude Sonnet 5 --- .../2026-09-16-extension-follow-ups-plan.md | 1811 +++++++++++++++++ 1 file changed, 1811 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md diff --git a/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md b/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md new file mode 100644 index 00000000..6359b73d --- /dev/null +++ b/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md @@ -0,0 +1,1811 @@ +# Extension-registry follow-up cluster (post-PR #321) — verified diagnosis, design, and implementation plan + +> **Draft 2, 2026-09-16.** Revised after an independent review that re-derived every claim from +> scratch and *ran* several of the plan's own tests. One blocker (Task 4's tests were false-green), +> nine smaller findings, all addressed — see **Appendix D** for the change log and **Appendix C** for +> the red-then-green evidence that now backs Task 4. Everything the review confirmed is retained +> unchanged. +> +> Written against `/home/ocel/Work/lsp/.worktrees/extension-follow-ups` +> (branch `fix/extension-registry-follow-ups`, tip `878cc4ea`, i.e. `main` after PR #321 plus two +> unrelated `chore:` commits). Measured against the real Moneta corpus at +> `/home/ocel/Work/Moneta/android`. +> +> Source: the controller's context brief `follow-ups-context.md`, items 1–5. **Every claim in that +> brief was re-checked against this worktree's actual source before being used here.** Four of its +> claims were wrong or incomplete; those corrections are Part 0, and they change the design of items +> 1, 2 and 4 materially — read Part 0 before reading the tasks. +> +> **Every claim marked VERIFIED was reproduced by running real code.** The instrument was a +> throwaway `throwaway_probe(&index, root)` block injected into `run_resolution_accuracy` +> (`src/cli/resolution_accuracy_poc.rs`), gated on a `KMP_PROBE` env var and placed *after* the +> benchmark's own index setup (`build_index(root, /* no_stdlib */ true)` + `scan_gradle_jars` + +> `detect_android_r_class_jars` + `index_jars`), so it observed the exact index state the +> `resolution-accuracy` benchmark observes. `mod platform_types;` was temporarily widened to +> `pub(crate) mod platform_types;` to reach the fallback from the CLI. Appendix A has the full probe. +> +> **Task 4's tests, and the Task 3/Task 4 production diffs they depend on, were additionally written +> into this worktree, compiled, and run** — real red, then real green — rather than only designed. +> See Appendix C. Every scratch edit has been reverted: `git status` shows only this plan file +> staged, and `cargo test --bin kmp-lsp` is back to the **1956 passed, 0 failed, 3 ignored** +> baseline. + +--- + +## Part 0 — What the context brief got wrong + +### 0.1 — Item 1 is **not** a "false-unique-win". It is `default_kotlin_import_tie_break` picking a decoy. + +The brief states: *"`indexer.lookup_definitions("List")` returns exactly **one** candidate +corpus-wide — a decompiled/synthetic `List` symbol inside `kotlin-stdlib-2.4.10.jar`. Because +`lookup_definitions` returns exactly one entry, `ambiguity_safe_tail_with_denylist` treats it as an +unambiguous win."* + +**VERIFIED FALSE.** On the real corpus `lookup_definitions("List")` returns **32** candidates, +including the correct `file:///…/android-36.1/java/util/List.java:138`. There is no unique-match +shortcut. What actually happens is one hop deeper inside +`ambiguity_safe_tail_with_denylist` (`src/resolver/tie_break.rs:75-107`): the 32 candidates survive +the denylist and module-scope stages, and then **`default_kotlin_import_tie_break` +(`src/resolver/tie_break.rs:135-156`) narrows them to the single `kotlin.collections`-packaged +candidate** — the body-less kotlin-stdlib decoy — because `kotlin.collections` is in +`KOTLIN_DEFAULT_IMPORT_PACKAGES`. That leaves exactly one location, the tail returns non-empty, and +`resolve_kotlin_builtin_type_platform_equivalent` is never reached. + +The irony is on the record: that tie-break's own doc comment (`tie_break.rs:129-134`) names +*"an ambiguous `java.util.List` vs. `kotlin.collections.List`"* as the shape it must not get wrong +from a **Java** origin — and from a **Kotlin** origin it gets exactly that pair wrong, in the +opposite direction, for the one name family where the Kotlin candidate is a decoy with no `.class` +body. + +This matters for the design: a fix aimed at "unique-match shortcuts" would have missed `List` +entirely. + +### 0.2 — Only **7** of the 22 table names actually resolve wrong, and `String` is not among them. + +The brief worries the reorder *"changes resolution for every name in that table (10+ names)"* and +asks whether `String` still resolves correctly. **VERIFIED, all 22 names, one probe run.** Comparing +`resolve_symbol_index_only(name, None, )` against +`resolve_kotlin_builtin_type_platform_equivalent(name)`: + +| name | `lookup_definitions` count | `resolve_symbol_index_only` today | platform equivalent | agree? | +|---|---|---|---|---| +| `String` | 38 | `…/java/lang/String.java:143` | `…/java/lang/String.java:143` | ✅ | +| `CharSequence` | 3 | `…/java/lang/CharSequence.java:59` | same | ✅ | +| **`List`** | **32** | **`jar:…/kotlin-stdlib-2.4.10.jar:2788`** | `…/java/util/List.java:138` | ❌ | +| **`MutableList`** | **2** | **`jar:…/kotlin-stdlib-2.4.10.jar:2789`** | `…/java/util/List.java:138` | ❌ | +| `Set` | 4 | `…/java/util/Set.java:115` | same | ✅ | +| **`MutableSet`** | **1** | **`jar:…/kotlin-gradle-plugin-2.4.10-gradle813.jar:27426`** | `…/java/util/Set.java:115` | ❌ | +| `Map` | 6 | `…/java/util/Map.java:163` | same | ✅ | +| **`MutableMap`** | **1** | **`jar:…/kotlin-gradle-plugin…:27427`** | `…/java/util/Map.java:163` | ❌ | +| `Collection` | 14 | `…/java/util/Collection.java:258` | same | ✅ | +| **`MutableCollection`** | **1** | **`jar:…/kotlin-gradle-plugin…:27423`** | `…/java/util/Collection.java:258` | ❌ | +| `Iterable` | 8 | `…/java/lang/Iterable.java:42` | same | ✅ | +| **`MutableIterable`** | **1** | **`jar:…/kotlin-gradle-plugin…:27422`** | `…/java/lang/Iterable.java:42` | ❌ | +| `Iterator` | 19 | `…/java/util/Iterator.java:58` | same | ✅ | +| **`MutableIterator`** | **1** | **`jar:…/kotlin-gradle-plugin…:27420`** | `…/java/util/Iterator.java:58` | ❌ | +| `Int` | 6 | `…/java/lang/Integer.java:1244` | same | ✅ | +| `Long` | 10 | `…/java/lang/Long.java:1375` | same | ✅ | +| `Double` | 7 | `…/java/lang/Double.java:745` | same | ✅ | +| `Float` | 8 | `…/java/lang/Float.java:673` | same | ✅ | +| `Boolean` | 10 | `…/java/lang/Boolean.java:133` | same | ✅ | +| `Byte` | 5 | `…/java/lang/Byte.java:380` | same | ✅ | +| `Short` | 8 | `…/java/lang/Short.java:383` | same | ✅ | +| `Char` | 4 | `…/java/lang/Character.java:9014` | same | ✅ | + +**15 of 22 already agree with the platform equivalent, so reordering is provably a no-op for them on +this corpus. 7 change, and each change is a decoy → real-declaration improvement.** `String` is safe, +confirmed on current `main`, not assumed. Note also that the two failure shapes are *different*: +`List`/`MutableList` lose the tie-break to a `kotlin-stdlib` decoy, while the other five +`Mutable*` names genuinely are unique-match wins over a `kotlin-gradle-plugin` decoy — so both +halves of 0.1 matter and a single reorder covers both. + +### 0.3 — `extension_is_in_scope` has **six** callers, not two, and most are not extension-specific. + +The brief says *"its only two callers, `resolve_extension_in_scope` in `extension.rs` and +`implicit_receiver_extension_match`"*. **VERIFIED FALSE.** Current callers: + +| # | Call site | Is it about extensions? | +|---|---|---| +| 1 | `src/resolver/extension.rs:91` — `resolve_extension_in_scope` | yes | +| 2 | `src/resolver/extension.rs:178` — `implicit_receiver_extension_match` | yes | +| 3 | `src/resolver/infer.rs:1936` — `find_extension_fn_return_type_scoped` | yes | +| 4 | `src/features/nullable_call_diagnostics.rs:275` — `extension_in_scope_here` | yes, but with its own stricter rule | +| 5 | `src/resolver/infer.rs:1626` — `candidate_declaration_is_reachable` | **no** — a receiver-less by-name reachability check over *all* workspace definitions | +| 6 | `src/indexer.rs:1099` — `Indexer::jar_candidate_is_reachable` | **no** — the same, over *all* JAR definitions | + +The function's own doc comment at `infer.rs:1604-1607` admits it: *"its body is not actually +extension-specific"*. **Consequence: the brief's "add a default-import-package check to +`extension_is_in_scope`" would silently change bare-name candidate preference for every JAR symbol in +`kotlin.*`/`java.lang` corpus-wide** (call site 6 gates which JAR candidates are "reachable" for +*any* name). That is a much larger, differently-shaped blast radius than the brief credits, and it is +not what item 2 is trying to fix. The design below therefore adds a *new* entry-shaped wrapper used +by the three extension-registry call sites only, and leaves the shared predicate alone. + +### 0.4 — `find_extension_fn_return_type_scoped` and `_global` do **not** have the same bug shape. + +The brief asks to *"verify whether the same bug exists in both"*. Read in full +(`src/resolver/infer.rs:1894-1977` and `:1979-2009`): + +- **`_scoped`** iterates `ExtensionEntry`s from `extension_by_receiver`, so it *does* have the + PR #321 family bug — but only on its truncated-`detail` fallback path (`:1959-1970`), where a + single `.find(|s| extension_declaration_matches(…))` picks the first declaration in the file + regardless of which `entry` the loop is on. On the primary path (`:1947-1949`) it reads + `entry.detail` directly, which *is* per-overload correct; what it lacks there is any way to choose + *between* overloads. +- **`_global`** has **no `ExtensionEntry` at all**. It walks `find_in_workspace_defs(method_name, …)` + and scans each file's own `symbols` by `(name, kind, extension_receiver)`. There is no `detail` to + match against, no registry entry, and no scope check. **`select_extension_symbol_range`'s two-step + selection cannot be applied to it** — there is no second step to apply. It is also effectively + dead in production: its only production caller chain + (`Resolver::method_return_type` → `find_method_return_type` → `find_extension_fn_return_type`) is + reached from `Indexer::find_method_return_type_for_type` (`src/indexer.rs:507`), which always + passes `Some(uri)`. + +Two further findings the brief did not have, both from reading the full body: + +- **Real early-abort bug, not just a first-match bug.** In `_scoped`'s truncated-detail fallback, + both `indexer.files.get(&entry.file_uri)…?` (`:1955-1958`) and `.find(…)?` (`:1962-1969`) return + `None` from the **whole function**, not from the current loop iteration. One entry whose declaring + file is not loaded, or whose declaration cannot be located, aborts the search over every remaining + entry. This is a `continue`-vs-`?` bug and it is cheap to fix. +- **Neither variant has a `CallShape`, and neither does any caller up the chain.** `find_method_return_type`, + `Resolver::method_return_type` (`src/resolver/api.rs:162-167`) and + `Indexer::find_method_return_type_for_type` are all arity-blind. "Add arity awareness" is therefore + not a local fix — it is an API change through the `Resolver` trait. Decision D4 below scopes that + out with evidence. + +### 0.5 — Two corrections that strengthen, rather than contradict, the brief + +- The brief calls `jar_extension_for_type_root` a "compounding factor" that makes the + `CharSequence` case "work" by coincidence. **VERIFIED and sharper than stated:** that scope-blind + fallback is reachable **only** for a `QualifierRoot::TypePath` root + (`src/resolver/qualified.rs:126-129` — `if let QualifierRoot::TypePath { root, nested } = parsed`). + A real call site like `text.isNotEmpty()` has a **value** root and never reaches it. So the scope + rejection in item 2 is *fatal* for exactly the shape users write, and only invisible when a test or + probe spells the type name out. This is why item 2 is worth doing and why a naive end-to-end test + of it will be green before the fix (see Task 2 Step 1's trap note). +- Every other line number and code excerpt in the brief re-verified as correct: + `resolve.rs:411/421/459/462-470/473-481`, `platform_types.rs:50-86/88-93/110`, + `infer.rs:1791-1850/1857-1866`, `extension.rs:28-54/162-219`, `qualified.rs:513`, + `rename.rs:100-110`. + +--- + +## Part 1 — Design + +### Global constraints + +- **Repo house rules (`AGENTS.md`):** never commit to `main`; `cargo test` and + `cargo clippy --all-targets -- -D warnings` clean after every change; `cargo fmt` before committing; + **no abbreviated names** (`symbol`, `location`, `indexer`, `package` — never `sym`, `loc`, `idx`, + `pkg`) in any new code, tests included; **no `and` in a function name**; no `unwrap()`/`expect()` in + production code; no hardcoded tree-sitter node-kind strings. `.git/hooks/pre-push` gates five + checks (those four plus "no direct `index_workspace_full`/`index_workspace_prioritized` calls + outside the actor"). **Do not treat the hook as the definition of the rules:** its abbreviation + check is a fixed token list (`\b([sc])\b` plus a specific suffix list) and misses `fd`, `pkg` and + friends entirely, so AGENTS.md is the authority and the hook is a partial backstop. Every new name + in this plan (`extension_entry_is_in_scope`, `select_extension_symbol`, + `android_sdk_source_roots`) was checked against all five rules by hand. +- **Serena:** call `mcp__serena__activate_project` with **this worktree's** path + (`/home/ocel/Work/lsp/.worktrees/extension-follow-ups`) before any symbolic edit. One active project + at a time — parallel tasks in this worktree must not both drive Serena. +- **Attribution:** every commit ends with + `Co-Authored-By: Claude Sonnet 5 ` and **nothing else** — no + `Claude-Session:` line, in any commit or PR description. +- **No interactive rebase.** `git rebase -i` is unavailable in this session's tooling. Use + `git cherry-pick` / `git commit-tree` plumbing for any history edit. +- **Baseline:** `cargo test --bin kmp-lsp` → **1956 passed, 0 failed, 3 ignored** at `878cc4ea`. + Each task states its own expected post-count. +- **No cache-version bump is needed by any task here.** Nothing in this cluster changes a persisted + key or field — `JAR_MANIFEST_CACHE_VERSION` and `CACHE_VERSION` both stay put. (Stated explicitly + because the PR #298–300 memo's stale-cache trap applies only to derivation changes, and this + cluster has none.) +- **Measurement discipline:** `resolution-accuracy` recall is noisy at ±0.3–0.6pp on an unchanged + binary. Judge a task by whether its named Gap entries move, not by the aggregate percentage. +- **Line numbers below are against `878cc4ea`.** Navigate by symbol name if they drift. + +### Fix 1 — Try the Kotlin built-in platform equivalent *before* the global-definitions tail + +`resolve_chain` (`src/resolver/resolve.rs:248-484`) has four IO-policy tails that all end +`; if non-empty return it; else resolve_kotlin_builtin_type_platform_equivalent`. +Swap the two, in all four: + +| policy | today | after | +|---|---|---| +| `Full` (step 5.4 / 5.5) | `:403-414` then `:421` | platform equivalent at `:415`, then the existing `:403-414` block | +| `NoRg` | `:450-458` then `:459` | platform equivalent first | +| `IndexOnly` | `:462-469` then `:470` | platform equivalent first | +| `HierarchyAmbiguitySafe` | `:473-480` then `:481` | platform equivalent first | +| `ScopedOnly` | no tail at all | **unchanged** — "no tail" is its documented contract | + +This sits **below** steps 1–4.5 (local declaration, local line scan, explicit imports, Swift fast +path, same package, star imports, class hierarchy), all of which still win first. A workspace file +that really declares its own `class Double` is still found by its own callers through steps 1/3 — +verified present on the corpus (`…/InitUseCase.kt:73` declares a `Double`) and unaffected, because +that path never reaches the tail for an in-package caller. (The corpus does contain a workspace +declaration literally named `Double` — `…/InitUseCase.kt:73` — though it is a `data class Double` +**nested** inside `sealed class IntRates`, not a top-level declaration, so it is weaker corroboration +than the first draft of this plan claimed. The guard test below uses its own top-level fixture and +does not depend on this citation.) + +`resolve_kotlin_builtin_type_platform_equivalent`'s doc comment (`platform_types.rs:88-93`) states a +founding assumption that Part 0.1 disproves — *"every normal resolution step already failed by the +time any tail fallback calls this … so this can only ever turn an existing decline into a correct +resolve"*. It must be rewritten to state the real rule: **for the 22 compiler-intrinsic mapped types +in `KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`, the real platform declaration outranks any same-named +index/JAR candidate, because those names have no compiled `.class` anywhere and every same-named +candidate is therefore a decoy.** Also update the test-module banner at +`src/resolver/tests.rs:9272-9280`, which repeats the "last-resort" framing. + +#### Decision D1 — memoize the SDK-source-root probe in the same task, keyed on the root path + +*(Revised after review. The first draft deferred this behind a wall-clock measurement; that was +over-built for the problem and left a log-spam regression shipping in the meantime. Adopting the +reviewer's simpler design.)* + +`resolve_kotlin_builtin_type_platform_equivalent` calls +`crate::workspace_json::detect_android_sdk_source_paths(&workspace_root)` +(`platform_types.rs:132`), which is **not memoized** — every call does a `local.properties` read (or +env lookup), an `is_dir`, and a `read_dir` of `sdk/sources/` (`src/workspace_json.rs:666-686` → +`resolve_android_sdk_root`/`sdk_dir_from_local_properties`, `:849-869`). Today that only runs when +the whole chain already declined, which for `String`/`Int` never happens. After the reorder it runs +on every tail resolution of a built-in name — the hottest name class in any Kotlin corpus, on the +hover/inlay path. This project has been burned by exactly this class of regression before (the +hover/inlay multi-second stall from bare-name global scans). + +**And the cost is not only wall clock.** `detect_android_sdk_source_paths` does +`log::info!("android-sdk: auto-detected sources at …")` on **every successful detection** +(`workspace_json.rs:681`). Unmemoized after the reorder, that line fires once per built-in-name tail +resolution — it was observable as dozens of repeated lines per second in this plan's own probe run. +That is a user-visible log regression regardless of how the timing comes out, so "ship it and see" +is not available as an option here. + +**Decision: memoize in Task 1, keyed on the workspace root path itself.** Path-keyed rather than +`OnceLock`, because `WorkspaceRoot::set` (`src/indexer/workspace_root.rs:35-42`) can change the root +mid-session — it bumps a generation counter for exactly that reason. Comparing the key is simpler +than threading that generation through and is self-invalidating: + +```rust +use std::sync::Mutex; + +/// Memoized [`crate::workspace_json::detect_android_sdk_source_paths`], keyed +/// on the workspace root it was computed for. +/// +/// That function does real filesystem work on every call — a +/// `local.properties` read, an `is_dir`, and a `read_dir` of `sdk/sources/` — +/// plus a `log::info!` on every success. Harmless while the built-in-type +/// fallback only ran after the whole resolution chain had already declined; +/// not harmless now that it runs ahead of the tail for every name in +/// [`KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`], which is the hottest name +/// class in a Kotlin corpus and sits on the hover/inlay path. +/// +/// Keyed on the root path rather than cached once, because +/// `WorkspaceRoot::set` can change the root mid-session. An SDK installed +/// *after* the first probe is not picked up until the root changes — the same +/// staleness the workspace scan already has, since it reads these paths once +/// at startup (`src/workspace/mod.rs:133`, `src/cli/run.rs:284`). +fn android_sdk_source_roots(workspace_root: &Path) -> Vec { + static DETECTED_ROOTS: Mutex)>> = Mutex::new(None); + let Ok(mut detected) = DETECTED_ROOTS.lock() else { + return crate::workspace_json::detect_android_sdk_source_paths(workspace_root); + }; + if let Some((cached_root, cached_paths)) = detected.as_ref() { + if cached_root == workspace_root { + return cached_paths.clone(); + } + } + let paths = crate::workspace_json::detect_android_sdk_source_paths(workspace_root); + *detected = Some((workspace_root.to_path_buf(), paths.clone())); + paths +} +``` + +Note the poisoned-lock arm falls through to the uncached call rather than `unwrap()`ing — the +pre-push hook forbids `unwrap`/`expect` in production code, and "slower but correct" is the right +behaviour for a cache anyway. + +**Placement: `src/resolver/platform_types.rs`, not `workspace_json.rs`.** The two other callers of +`detect_android_sdk_source_paths` run once each at scan time; memoizing inside the shared function +would change their behaviour for no benefit. Only the hot caller needs it. + +**Why this needs no test of its own:** the suite already contains eight tests that call +`resolve_kotlin_builtin_type_platform_equivalent` against *different* `tempfile::tempdir()` roots +(`src/resolver/tests.rs:9303`, `:9337`, `:9359`, `:9384`, `:9427`, `:9490`, `:9518`, `:10168`, +`:10190`). A memo that ignored its key would serve one test's SDK layout to another and they would go +red. That is real coverage of the only thing that can go wrong here; adding a ninth test that asserts +the cache-key comparison directly would be testing the implementation, not the behaviour. + +Wall clock is still recorded in Task 1 Step 5 — not as a gate, just so the effect of the memo is on +the record. + +#### Expected effect of Fix 1, as a falsifiable prediction + +- `List`, `MutableList`, `MutableSet`, `MutableMap`, `MutableCollection`, `MutableIterable`, + `MutableIterator` stop anchoring on a decoy and start anchoring on a real `java.util.*`/`java.lang.*` + declaration **with a walkable supertype chain**. +- The other 15 table names are unchanged — measured, not hoped. +- Downstream: `type_path_anchors` (`src/resolver/qualified.rs`) now anchors `List` on + `java/util/List.java`, so the supertype walk `List → Collection → Iterable` becomes reachable for + the first time on this corpus. +- **`firstOrNull` will probably NOT leave the Gap top-20 on Fix 1 alone.** VERIFIED why: even with a + correct `List` anchor, the `Iterable`-keyed `firstOrNull` overloads are rejected by + `extension_is_in_scope` (probe: `Iterable.firstOrNull` → `in_scope=false`, twice). Fix 1 and Fix 2 + are **jointly** necessary for the trailing-lambda `firstOrNull` shape. Fix 2 alone should recover + the 0-arg `List`-keyed overload. Do not read "`firstOrNull` still present" after Task 1 as Task 1 + failing. + +### Fix 2 — Kotlin's default-import packages count as in-scope for an extension entry + +**Do not touch `extension_is_in_scope`** (Part 0.3). Add one entry-shaped wrapper next to it in +`src/resolver/infer.rs`: + +```rust +/// Whether extension registry `entry` is callable from the file at `from_uri`. +/// +/// [`extension_is_in_scope`]'s package/import rules, plus the one rule they +/// cannot express: a Kotlin file implicitly imports every name declared +/// directly in Kotlin's own default-import packages (see +/// [`crate::resolver::imports::KOTLIN_DEFAULT_IMPORT_PACKAGES`]), so +/// `kotlin.text`'s `isNotEmpty` or `kotlin.collections`'s `firstOrNull` is in +/// scope at every call site with no `import` line anywhere — and no real file +/// ever writes one. +/// +/// Gated on the CALLING file's language, for the same reason +/// [`crate::resolver::tie_break`]'s `default_kotlin_import_tie_break` gates +/// its own use of the same set: Kotlin's default imports are a fact about +/// Kotlin source files, and `resolve_qualified` runs over indexed `.java` and +/// `.swift` files too. +/// +/// Measured on the Moneta corpus: the registry holds 24897 entries across 2360 +/// receiver buckets; 3641 of those entries live in a default-import package, +/// 3638 of them top-level, and every one was rejected here for every caller. +/// +/// Deliberately NOT folded into [`extension_is_in_scope`] itself: four of that +/// function's six callers are not about extensions at all +/// (`candidate_declaration_is_reachable`, `Indexer::jar_candidate_is_reachable` +/// and `nullable_call_diagnostics`' stricter own rule), and widening the shared +/// predicate would change bare-name JAR candidate preference corpus-wide. +pub(crate) fn extension_entry_is_in_scope( + entry: &crate::types::ExtensionEntry, + from_uri: &Url, + caller_file_data: Option<&FileData>, +) -> bool { + if extension_is_in_scope( + entry.package.as_ref(), + &entry.name, + entry.container.as_ref(), + entry.visibility, + entry.file_uri == from_uri.as_str(), + caller_file_data, + ) { + return true; + } + let caller_is_kotlin = + crate::Language::from_path(from_uri.as_str()) == crate::Language::Kotlin; + let entry_is_default_imported = entry + .package + .as_ref() + .is_some_and(|package| crate::resolver::imports::is_default_import_package(package)); + caller_is_kotlin && entry_is_default_imported +} +``` + +Reuse, not reinvention: `is_default_import_package` already exists at `src/resolver/imports.rs:46-48` +and needs only to widen from private `fn` to `pub(super) fn` (it is in a `mod imports;` private to +`resolver`, so `pub(super)` makes it visible to `resolver::infer` exactly as +`KOTLIN_DEFAULT_IMPORT_PACKAGES` already is to `resolver::tie_break`). + +Then replace the three extension-registry call sites with it. Each shrinks from a seven-line call to +one line: + +| site | today | after | +|---|---|---| +| `src/resolver/extension.rs:91-98` (`resolve_extension_in_scope`) | 8-line `extension_is_in_scope(…)` | `extension_entry_is_in_scope(entry, from_uri, caller_file_data_ref)` | +| `src/resolver/extension.rs:178-185` (`implicit_receiver_extension_match`) | same | same | +| `src/resolver/infer.rs:1936-1945` (`find_extension_fn_return_type_scoped`) | same | same | + +**Explicitly not changed:** `src/features/nullable_call_diagnostics.rs:275`. That call site +deliberately passes `None`/`Public`/`false` and requires *stronger* evidence than ordinary scope +(its own doc comment at `:252-272` explains why a member extension needs the declaring file itself). +Widening it would relax a diagnostic's suppression rule with no measurement behind it; out of scope +here, named so it is not an oversight. + +#### Decision D2 — the language gate is required, not speculative + +The brief left this open. **VERIFIED: non-Kotlin origins really do reach these paths.** +`resolve_extension_in_scope` is called from `candidates_on` (`qualified.rs:404`) and +`resolve_extension_via_supertype_hierarchy` (`qualified.rs:740`), both under `resolve_qualified`, +which is language-agnostic; `resolve_implicit_receiver_callee` is called from +`src/indexer/lookup.rs:126` and `src/features/references.rs:228`, likewise. And the +`resolution-accuracy` benchmark itself scans **`.kt` and `.java`** workspace files +(`src/cli/resolution_accuracy_poc.rs:99`), driving `classify_cursor` + `resolve_identity` over both. +So a Java-origin call site can reach the extension registry today, and marking every `kotlin.*` entry +in-scope for it would be wrong in precisely the way `default_kotlin_import_tie_break`'s Copilot +finding described. Gate on Kotlin, and pin it with a test. + +#### Expected effect of Fix 2 + +Registry-wide, measured: 2360 receiver buckets / 24897 entries, of which **3641 are in a +default-import package (3638 top-level)** and are rejected today for every caller. Spot-checked +individually, all `in_scope=false` today: + +``` +CharSequence.isNotEmpty : pkg=kotlin.text fun CharSequence.isNotEmpty(): Boolean +Iterable.firstOrNull : pkg=kotlin.collections fun Iterable.firstOrNull(): T? +Iterable.firstOrNull : pkg=kotlin.collections fun Iterable.firstOrNull(predicate: (T) -> Boolean): T? +Iterable.forEach : pkg=kotlin.collections fun Iterable.forEach(action: (T) -> Unit) +List.firstOrNull : pkg=kotlin.collections fun List.firstOrNull(): T? +``` + +Prediction: `isNotEmpty`, `forEach` and the 0-arg `firstOrNull` shape become resolvable through the +**value-root** path (`text.isNotEmpty()`), which — per Part 0.5 — has no scope-blind fallback to hide +behind today. This is the largest single change in the cluster and the one most likely to move the +aggregate number. It is also the one most likely to raise `FilteredCandidate`, since it hands the +shape filter more candidates; watch that counter. + +### Fix 3 — `implicit_receiver_extension_match` selects the symbol for *its own* entry + +`src/resolver/extension.rs:162-219`. Verified exactly as the brief describes: for each `entry`, the +inner `fd.symbols.iter().find(|s| extension_declaration_matches(s, name, receiver_base, entry.container))` +(`:197-207`) matches on `(name, extension_receiver, container)` only — identical across every overload +— so every iteration of the loop picks the **same** first-declared overload, shape-checks that one at +`:210-216`, and a differently-shaped call is rejected on every iteration. The corpus makes this +concrete: **2589 of 4211 multi-entry `(receiver, name)` registry groups have two or more entries +declared in the same file**, e.g. + +``` +HttpClientCall.receive : ["suspend fun HttpClientCall.receive(): T", + "suspend fun HttpClientCall.receive(info: TypeInfo): Any"] +Writer.write : ["fun Writer.write(document: Document, prettyPrint: Boolean = ...): Writer", + "fun Writer.write(element: Element, prettyPrint: Boolean = ...): Writer"] +``` + +**Fix: extract PR #321's proven two-step selection into a symbol-returning sibling**, as the brief's +option (a). `select_extension_symbol_range` (`extension.rs:28-54`) already does filter-by- +`extension_declaration_matches` then prefer-exact-`detail`-match, with a first-match fallback; it just +throws the symbol away and keeps the `Range`. Split it: + +```rust +/// The declaring symbol for one extension registry `entry`, inside its +/// declaring file's already-parsed symbol table. +/// +/// A file may declare several overloads of the same extension — same name, +/// same receiver, same container — and `extension_declaration_matches` alone +/// cannot tell them apart, so prefer the declaration whose `detail` (full +/// signature text) is exactly this entry's own. `SymbolEntry::detail` and +/// `ExtensionEntry::detail` are the same string by construction on both +/// derivation paths (JAR: `src/indexer/jar.rs`; source: `src/indexer/apply.rs`). +/// When nothing matches exactly — a JAR-derived Java method can carry a +/// `"(...)"` placeholder detail, the PR #311 shape — fall back to the first +/// shape-matching declaration rather than returning nothing. +pub(super) fn select_extension_symbol<'file>( + file_data: &'file FileData, + name: &str, + receiver_base: &str, + container: Option<&String>, + detail: &str, +) -> Option<&'file crate::types::SymbolEntry> { + let declaring_symbols: Vec<&crate::types::SymbolEntry> = file_data + .symbols + .iter() + .filter(|symbol| { + crate::resolver::infer::extension_declaration_matches( + symbol, + name, + receiver_base, + container, + ) + }) + .collect(); + let exact_signature_match = declaring_symbols + .iter() + .find(|symbol| symbol.detail == detail); + exact_signature_match + .or_else(|| declaring_symbols.first()) + .copied() +} + +pub(super) fn select_extension_symbol_range( + file_data: &FileData, + name: &str, + receiver_base: &str, + container: Option<&String>, + detail: &str, +) -> Range { + select_extension_symbol(file_data, name, receiver_base, container, detail) + .map(|symbol| symbol.selection_range) + .unwrap_or_default() +} +``` + +`implicit_receiver_extension_match` then replaces its `.find(…).cloned()` with +`select_extension_symbol(&file_data, name, receiver_base, entry.container.as_ref(), &entry.detail).cloned()` +and keeps the rest of the loop (vararg exemption, `shape.accepts`) untouched. Its two existing +callers of `select_extension_symbol_range` (`extension.rs:110`, `qualified.rs:537`) are unaffected. + +**Blast radius:** one entry point (`resolve_implicit_receiver_callee`) and its two callers +(`src/indexer/lookup.rs:126`, `src/features/references.rs:228`). No corpus-wide risk. No measurement +required beyond a spot-check. + +### Fix 4 — `find_extension_fn_return_type_scoped`: per-entry selection, and stop aborting the loop + +Two changes in `src/resolver/infer.rs:1955-1974`, both inside the truncated-`detail` fallback: + +1. Replace `.find(|s| extension_declaration_matches(…))?` with + `select_extension_symbol(file_data, method_name, receiver_base, entry.container.as_ref(), &entry.detail)` + — the same helper Fix 3 extracts, reused rather than reimplemented. +2. Replace both `?` early-returns with `continue`, so one entry with an unloaded declaring file or an + unlocatable declaration no longer aborts the search over every remaining entry (Part 0.4). + +`find_extension_fn_return_type_global` is **left alone**, with a short comment saying why: no +`ExtensionEntry`, no `detail`, nothing for the two-step selection to select *between*, and no +production caller reaches it (`Indexer::find_method_return_type_for_type` always passes `Some(uri)`). + +#### Decision D4 — arity-aware return-type inference is scoped OUT, with the evidence that justifies a future plan + +The reviewer's original finding was *"returns the FIRST matching entry's return type with no arity +awareness at all"*. That is real, and the corpus shows it is not rare: of **4211** `(receiver, name)` +registry groups with two or more entries, **482 have entries with differing return types** — + +``` +Array.max : Double? | Float? | T? | Double | Float | T (6 overloads, 3 distinct returns) +Array.minBy : T? | T +Array.sumOf : BigDecimal | BigInteger | Double | Int | Long | UInt | ULong +Array.toMap : Map | M +Error.renderError : (no return) | String +``` + +**But there is no arity to select with, at any point in the chain.** +`find_extension_fn_return_type(indexer, receiver_base, method_name, from_uri)`, +`find_method_return_type(…)` (`infer.rs:1739`), `Resolver::method_return_type(…)` +(`api.rs:162-167`) and `Indexer::find_method_return_type_for_type(…)` (`indexer.rs:483`) are all +arity-blind. Making this right means threading a `CallShape` through the `Resolver` trait — which has +precedent (`find_method_params_text` already takes one, `src/indexer.rs:1109`) but is a trait-surface +change with its own callers, its own regression surface, and no way to attribute its effect inside +this cluster's measurements. + +**Decision: fix the two mechanical defects above in Task 4, and open the arity work as its own plan, +citing the 482-group measurement as its motivation.** Do not attempt it here. Also note the honest +limit: because 482 is a count of *registry groups*, not of *call sites*, the real user-visible +impact is still unquantified — quantifying it (how many hover/inlay-hint sites actually land on a +differing-return overload set) is the first step of that future plan, not an assumption it may start +from. + +### Decision D5 — item 5 (`rename.rs`): ship as-is, close the item, no code change + +`src/features/rename.rs:100-110`, verified current: + +```rust +let identity = resolve_identity(&symbol, indexer, uri); +let NavigationSource::CstResolved(definitions) = identity else { + return Err(refusal("identity is ambiguous — could not resolve a single definition")); +}; +if definitions.len() != 1 { + return Err(refusal("identity is ambiguous — matches more than one definition")); +} +``` + +**Decision: (a) — already-correct behaviour, ship as-is, close the item. No UX change, no code, no +test.** Reasons, in order of weight: + +1. The pre-#321 behaviour was *unsound*: a same-arity `Modifier.weight` collision resolved to one + arbitrary candidate and `rename` silently rewrote that one declaration and its references — a + destructive, irreversible edit to an arbitrarily-chosen one of two genuinely distinct symbols. + Refusing is strictly safer, and the safety direction is the one that matters for a mutating + operation. +2. It is not a new code path invented by #321. `rename_impl` already had "ambiguous → refuse" as its + documented contract for every other ambiguity; #321 merely stopped hiding one case from it. + Shipping it means the contract is now uniform, which is worth more than a special case. +3. The obvious "improvement" — naming both candidates in the message — is probably not free. + **ASSUMPTION, not verified:** the refusal string is a `tower_lsp::jsonrpc::Error` message, and the + reasoning here assumes LSP clients surface such an error as a plain toast with no file/line + affordance, making a message that lists two URIs noise rather than navigation. That was *not* + checked against a real client (neither VS Code nor nvim was exercised), and it is the load-bearing + reason this decision picks (a) over (b). If someone does check and finds a client that renders the + message navigably, reason 3 collapses and (b) becomes worth reconsidering — reasons 1, 2 and 4 + still stand on their own, so the decision would survive, but the cost/benefit would shift. +4. Nothing in the cluster's measurements touches rename, so a change here would ship unmeasured. + +The one thing to do is **write the decision down where it will be found**: a `//` line comment +immediately above the `definitions.len() != 1` refusal — it sits inside `rename_impl`'s body, so it +cannot be a `///` doc comment — recording that a same-arity extension collision reaches it by design +after PR #321, and that a disambiguating UX needs a client-side picker. That is a comment-only edit; +fold it into Task 3's commit (the task that already touches the extension-selection code this refusal +is downstream of) rather than opening a task for it. + +### Task ordering and file-collision map + +| task | item | production files touched | test files | +|---|---|---|---| +| 1 | 1 | `src/resolver/resolve.rs`, `src/resolver/platform_types.rs` | `src/resolver/tests.rs` | +| 2 | 2 | `src/resolver/infer.rs`, `src/resolver/imports.rs`, `src/resolver/extension.rs` | `src/resolver/tests.rs` | +| 3 | 3 | `src/resolver/extension.rs`, `src/features/rename.rs` (one `//` line comment in a function body, no behaviour change) | `src/resolver/tests.rs` | +| 4 | 4 | `src/resolver/infer.rs` | `src/resolver/infer_tests.rs` | +| 5 | 5 | — (Decision D5, folded into task 3) | — | + +- **Task 1 is fully independent** of 2/3/4 — no shared production file. It can run in parallel with + any of them. +- **Task 2 must precede Tasks 3 and 4.** It touches `extension.rs` (which Task 3 rewrites) and + `infer.rs` (which Task 4 rewrites), and Task 3 extracts the helper Task 4 consumes. +- **Tasks 3 and 4 are independent of each other** once Task 2 has landed — different files + (`extension.rs` vs `infer.rs`) — **except** that Task 4 calls `select_extension_symbol`, which Task + 3 creates. So: **Task 3 before Task 4**, or Task 4 rebases onto Task 3. +- Recommended shape: **Task 1 ∥ (Task 2 → Task 3 → Task 4)**, four PRs. Three tasks append to + `src/resolver/tests.rs` (Tasks 1–3), in different regions; Task 4 appends to + `src/resolver/infer_tests.rs` instead and so collides with none of them. A pre-flight conflict scan + before any parallel dispatch is still the executor's job. +- Corpus measurements are serial regardless (one release build, one long benchmark run at a time), so + Tasks 1 and 2 must not have their measurement runs interleaved. + +--- + +## Part 2 — Implementation plan + +> **For agentic workers:** REQUIRED SUB-SKILL: `superpowers:test-driven-development` for each task, +> `superpowers:subagent-driven-development` to run them. Each task is independently testable, +> independently revertable, and ships as its own PR. Steps use checkbox (`- [ ]`) syntax. +> +> Re-read the **Global constraints** block above before writing any code. In particular: full words +> for every identifier in new code including tests (`indexer`, not `idx`), and +> `Co-Authored-By: Claude Sonnet 5 ` with no `Claude-Session:` line. + +--- + +### Task 1: Prefer the Kotlin built-in platform equivalent over a same-named decoy + +**Why first:** independent of every other task, corpus-verified with an exact before/after +enumeration (Part 0.2), and its measurement is the one most at risk of being confounded by Task 2's. + +**Files:** +- Modify: `src/resolver/resolve.rs` — four tails (`:403-421` Full, `:449-460` NoRg, `:461-471` + IndexOnly, `:472-482` HierarchyAmbiguitySafe) +- Modify: `src/resolver/platform_types.rs` — doc comment on + `resolve_kotlin_builtin_type_platform_equivalent` (`:88-109`); new private + `android_sdk_source_roots` memo (Decision D1) called from `:132` +- Modify: `src/resolver/tests.rs` — 3 new tests; update the module banner at `:9272-9280` + +**Interfaces:** no public signature changes. `resolve_kotlin_builtin_type_platform_equivalent` keeps +`(&Indexer, &str) -> Vec`; `android_sdk_source_roots` is private to `platform_types`. + +- [ ] **Step 1: Write the failing tests** + +Append to `src/resolver/tests.rs`, next to the existing platform-equivalent block (`:9272+`), reusing +its `write_fake_android_sdk_source` helper. + +```rust +/// Part 0.1's real corpus mechanism, in miniature: bare `List` has MANY +/// same-named index candidates, `ambiguity_safe_tail_with_denylist` survives +/// the denylist and module-scope stages, and then +/// `default_kotlin_import_tie_break` narrows to the single +/// `kotlin.collections`-packaged one -- a kotlin-stdlib decoy with no +/// compiled body and no supertypes. The real `java.util.List` must win +/// instead, or `type_path_anchors` anchors every `List` receiver on a +/// declaration with no walkable supertype chain. +#[test] +fn a_builtin_type_name_prefers_the_platform_declaration_over_a_default_imported_decoy() { + use crate::types::FileData; + use std::sync::Arc; + + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + write_fake_android_sdk_source( + root, + "java/util/List.java", + "package java.util;\npublic interface List extends Collection {\n}\n", + ); + + let indexer = Indexer::new(); + indexer.workspace_root.set(root.to_path_buf()); + + let stdlib_decoy_uri = "jar:file:///kotlin-stdlib.jar!/kotlin/collections/List.class"; + let unrelated_decoy_uri = "jar:file:///unrelated.jar!/com/example/List.class"; + for decoy_uri in [stdlib_decoy_uri, unrelated_decoy_uri] { + indexer + .jar_definitions + .entry("List".to_owned()) + .or_default() + .push(tower_lsp::lsp_types::Location { + uri: Url::parse(decoy_uri).unwrap(), + range: Default::default(), + }); + } + indexer.jar_files.insert( + stdlib_decoy_uri.to_owned(), + Arc::new(FileData { + package: Some("kotlin.collections".to_owned()), + ..Default::default() + }), + ); + indexer.jar_files.insert( + unrelated_decoy_uri.to_owned(), + Arc::new(FileData { + package: Some("com.example".to_owned()), + ..Default::default() + }), + ); + + let caller_source = "package app\nfun use(items: List) { }\n"; + let caller_path = root.join("Caller.kt"); + std::fs::write(&caller_path, caller_source).unwrap(); + let caller_uri = Url::from_file_path(&caller_path).unwrap(); + indexer.index_content(&caller_uri, caller_source); + + let locations = resolve_symbol_index_only(&indexer, "List", None, &caller_uri); + assert_eq!( + locations.len(), + 1, + "expected exactly the platform declaration, got {locations:?}" + ); + assert!( + locations[0].uri.path().ends_with("java/util/List.java"), + "expected the real java.util.List, got {:?}", + locations[0].uri + ); +} + +/// The second, different failure shape from Part 0.2: `MutableSet` and its +/// four `Mutable*` siblings have exactly ONE same-named index candidate (a +/// kotlin-gradle-plugin decoy), so they never reach a tie-break at all -- +/// the unique-match arm returns the decoy outright. The platform equivalent +/// must still win, and note the target's simple name differs from the +/// Kotlin one (`MutableSet` -> `java.util.Set`). +#[test] +fn a_mutable_builtin_type_name_prefers_the_platform_declaration_over_a_unique_decoy() { + use crate::types::FileData; + use std::sync::Arc; + + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + write_fake_android_sdk_source( + root, + "java/util/Set.java", + "package java.util;\npublic interface Set extends Collection {\n}\n", + ); + + let indexer = Indexer::new(); + indexer.workspace_root.set(root.to_path_buf()); + + let decoy_uri = "jar:file:///kotlin-gradle-plugin.jar!/kotlin/collections/MutableSet.class"; + indexer.jar_definitions.insert( + "MutableSet".to_owned(), + vec![tower_lsp::lsp_types::Location { + uri: Url::parse(decoy_uri).unwrap(), + range: Default::default(), + }], + ); + indexer.jar_files.insert( + decoy_uri.to_owned(), + Arc::new(FileData { + package: Some("kotlin.collections".to_owned()), + ..Default::default() + }), + ); + + let caller_source = "package app\nfun use(items: MutableSet) { }\n"; + let caller_path = root.join("Caller.kt"); + std::fs::write(&caller_path, caller_source).unwrap(); + let caller_uri = Url::from_file_path(&caller_path).unwrap(); + indexer.index_content(&caller_uri, caller_source); + + let locations = resolve_symbol_index_only(&indexer, "MutableSet", None, &caller_uri); + assert_eq!( + locations.len(), + 1, + "expected exactly the platform declaration, got {locations:?}" + ); + assert!( + locations[0].uri.path().ends_with("java/util/Set.java"), + "expected the real java.util.Set, got {:?}", + locations[0].uri + ); +} + +/// Decoy guard, expected GREEN both before and after: the reorder lives in +/// `resolve_chain`'s TAIL, below steps 1-4.5, so a workspace file that really +/// declares its own `Double` must still win for a caller in its own package. +/// (Not purely hypothetical: the Moneta corpus declares a `Double` at +/// feature/credit_card/clip/.../InitUseCase.kt:73, though as a NESTED data +/// class rather than a top-level one. This fixture uses a top-level +/// declaration, which is the shape the tail ordering actually has to respect.) +#[test] +fn a_workspace_declaration_still_outranks_the_builtin_platform_equivalent() { + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + write_fake_android_sdk_source( + root, + "java/lang/Double.java", + "package java.lang;\npublic final class Double extends Number {\n}\n", + ); + + let indexer = Indexer::new(); + indexer.workspace_root.set(root.to_path_buf()); + + let declaration_source = "package app\nclass Double(val value: String)\n"; + let declaration_path = root.join("Double.kt"); + std::fs::write(&declaration_path, declaration_source).unwrap(); + let declaration_uri = Url::from_file_path(&declaration_path).unwrap(); + indexer.index_content(&declaration_uri, declaration_source); + + let caller_source = "package app\nfun use(value: Double) { }\n"; + let caller_path = root.join("Caller.kt"); + std::fs::write(&caller_path, caller_source).unwrap(); + let caller_uri = Url::from_file_path(&caller_path).unwrap(); + indexer.index_content(&caller_uri, caller_source); + + let locations = resolve_symbol_index_only(&indexer, "Double", None, &caller_uri); + assert_eq!( + locations, + vec![tower_lsp::lsp_types::Location { + uri: declaration_uri, + range: locations + .first() + .map(|location| location.range) + .unwrap_or_default(), + }], + "the same-package workspace declaration must still win, got {locations:?}" + ); +} +``` + +- [ ] **Step 2: Verify the tests fail for the right reason** + +``` +cargo test --bin kmp-lsp a_builtin_type_name_prefers_the_platform_declaration # FAIL: kotlin-stdlib decoy +cargo test --bin kmp-lsp a_mutable_builtin_type_name_prefers_the_platform # FAIL: kotlin-gradle-plugin decoy +cargo test --bin kmp-lsp a_workspace_declaration_still_outranks_the_builtin # PASS (guard) +``` + +Read the failure text, not just "FAILED". Test 1 must fail showing the +`kotlin/collections/List.class` URI — if it fails showing `com/example/List.class` instead, the +default-import tie-break is not being exercised and the fixture is wrong; if it fails empty, the +fake SDK was not picked up (check `local.properties` landed at `root`). + +Also re-run the two tie-break tests that touch these names before implementing, to record they are +green now: `kotlin_default_import_tie_break_does_not_apply_from_a_java_origin_file` +(`src/resolver/tests.rs:11025`) and the `com.android.internal` denylist test just above it. Both use +`Indexer::new()` with **no** `workspace_root`, so the platform fallback short-circuits at +`platform_types.rs:128` and they must stay green after the reorder. If either goes red, the reorder +was placed above steps 1–4.5 by mistake. + +- [ ] **Step 3: Implement the reorder** + +In `src/resolver/resolve.rs`, in each of the four tails, move the +`resolve_kotlin_builtin_type_platform_equivalent(indexer, name)` call *above* the global-definitions +lookup and return it when non-empty: + +```rust +// 5.4 ── Kotlin built-in-type platform equivalent ────────────────────── +// Ahead of the global definitions tail, not behind it: for the ~22 +// compiler-intrinsic mapped types in +// KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS there is no compiled `.class` +// anywhere, so every same-named index/JAR candidate is a decoy -- and a +// decoy wins today, either outright (MutableSet: one candidate) or via +// default_kotlin_import_tie_break, which prefers the `kotlin.collections` +// decoy out of `List`'s 32 candidates. Anchoring `List` on a body-less +// decoy leaves it with no walkable supertype chain, so no member or +// extension lookup downstream can ever reach `Iterable`. +let platform_equivalent = resolve_kotlin_builtin_type_platform_equivalent(indexer, name); +if !platform_equivalent.is_empty() { + return platform_equivalent; +} +``` + +`ScopedOnly` keeps its `vec![]` arm untouched. Keep the existing global-definitions blocks exactly as +they are, now reached only when the platform fallback declines (which is every name outside the +22-entry table, and every project without Android SDK sources). + +Rewrite `resolve_kotlin_builtin_type_platform_equivalent`'s doc comment (`platform_types.rs:88-93`) +per Fix 1 above — the current "every normal resolution step already failed … can only ever turn a +decline into a resolve" claim is false and must not survive this change. Update the test-module +banner at `src/resolver/tests.rs:9272-9280` the same way. + +Add the `android_sdk_source_roots` memo from Decision D1 to `platform_types.rs` and call it at +`:132` in place of `crate::workspace_json::detect_android_sdk_source_paths`. This is part of the same +task, not a follow-up: without it the reorder ships a `log::info!` on every built-in-name resolution. + +- [ ] **Step 4: Verify** + +``` +cargo test --bin kmp-lsp # expect 1956 + 3 new = 1959 passed, 0 failed, 3 ignored +cargo clippy --all-targets -- -D warnings +cargo fmt -- --check +``` + +- [ ] **Step 5: Measure on the real corpus** + +``` +cargo build --release +time ./target/release/kmp-lsp resolution-accuracy /home/ocel/Work/Moneta/android +``` + +Record: member recall before/after, bare recall, `FilteredCandidate` total, the member Gap top-20 +diff, **and wall-clock time both runs** (Decision D1). + +Success criteria, in order: +1. **No regression.** Member recall must not fall outside the ±0.3–0.6pp noise band. Baseline to beat: + 91.7% (151280/164972) member, 73.5% bare, FilteredCandidate 6234, member Gap 7458. +2. Gap names that depend on a `List`/`Map`/`Set`-typed receiver having real supertypes should thin + out, but see the Fix 1 prediction — `firstOrNull` specifically is expected to **stay** until Task 2 + lands. Do not treat that as failure. +3. Wall clock recorded for both runs — not a gate (the D1 memo ships in this same task), just so the + reorder's cost is on the record. Also confirm the `"android-sdk: auto-detected sources at …"` + log line appears a small, constant number of times in the run's stderr, not once per built-in-name + resolution: that is the cheapest direct check that the memo is actually working. + +- [ ] **Step 6: Commit and open a PR** + +The PR description must state: the real mechanism (default-import tie-break preferring a body-less +decoy, not a unique-match shortcut), the exact 7-of-22 before/after table from Part 0.2, and the D1 +memo with the measured wall clock. + +```bash +git commit -m "fix(resolve): prefer a built-in type's platform declaration over a same-named decoy + +Co-Authored-By: Claude Sonnet 5 " +``` + +--- + +### Task 2: Kotlin's default-import packages count as in-scope for an extension entry + +**Depends on nothing; must precede Tasks 3 and 4** (it edits `extension.rs` and `infer.rs`, which +they rewrite). + +**Files:** +- Modify: `src/resolver/imports.rs` — widen `is_default_import_package` (`:46`) to `pub(super)` +- Modify: `src/resolver/infer.rs` — new `extension_entry_is_in_scope` next to `extension_is_in_scope` + (`:1791-1850`); swap the call at `:1936-1945` +- Modify: `src/resolver/extension.rs` — swap the calls at `:91-98` and `:178-185` +- Modify: `src/resolver/tests.rs` — 4 new tests + +**Interfaces:** `extension_is_in_scope` keeps its signature and all six callers keep compiling; three +of them switch to the new wrapper. New `pub(crate) fn extension_entry_is_in_scope(&ExtensionEntry, +&Url, Option<&FileData>) -> bool`. + +- [ ] **Step 1: Write the failing tests** + +> **Trap — do not write this as a naive end-to-end test.** Per Part 0.5, +> `find_definition_qualified_index_only("isNotEmpty", Some("CharSequence"), …)` spells the *type name* +> out, which makes the qualifier a `QualifierRoot::TypePath`, which reaches the **scope-blind** +> `jar_extension_for_type_root` fallback (`qualified.rs:126-129`) and resolves **green before the +> fix**. The rejection this task fixes is only fatal on a *value* root. Test at the +> `resolve_extension_in_scope` layer, where the behaviour actually lives. + +Append to `src/resolver/tests.rs`: + +```rust +/// Builds an indexer whose registry holds one `kotlin.text`-packaged +/// extension on `CharSequence`, plus a caller file in an unrelated package +/// with no imports at all -- the shape of every real Kotlin call site, since +/// nobody writes `import kotlin.text.isNotEmpty`. +fn indexer_with_a_default_imported_extension(caller_path: &str) -> (Indexer, Url) { + use crate::types::{ExtensionEntry, FileData, SourceSet, SymbolEntry, Visibility}; + use std::sync::Arc; + use tower_lsp::lsp_types::{Position, Range, SymbolKind}; + + let indexer = Indexer::new(); + let jar_uri = "jar:file:///kotlin-stdlib.jar!/kotlin/text/StringsKt.class".to_owned(); + let declaration_range = Range { + start: Position { + line: 7, + character: 0, + }, + end: Position { + line: 7, + character: 10, + }, + }; + let detail = "fun CharSequence.isNotEmpty(): Boolean".to_owned(); + + indexer.jar_files.insert( + jar_uri.clone(), + Arc::new(FileData { + symbols: vec![SymbolEntry { + name: "isNotEmpty".to_owned(), + kind: SymbolKind::FUNCTION, + visibility: Visibility::Public, + range: declaration_range, + selection_range: declaration_range, + detail: detail.clone(), + container: None, + params: String::new(), + param_counts: (0, 0), + cold: crate::types::pack_cold_fields( + vec![], + "CharSequence".to_owned(), + String::new(), + String::new(), + ), + trailing_lambda: false, + deprecated: false, + }], + source_set: SourceSet::Library, + package: Some("kotlin.text".to_owned()), + lines: Arc::new(vec![]), + ..Default::default() + }), + ); + indexer + .extension_by_receiver + .entry("CharSequence".to_owned()) + .or_default() + .push(ExtensionEntry { + file_uri: jar_uri, + name: "isNotEmpty".to_owned(), + kind: SymbolKind::FUNCTION, + detail, + visibility: Visibility::Public, + package: Some("kotlin.text".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }); + + let caller_uri = uri(caller_path); + indexer.index_content(&caller_uri, "package app\nclass Caller\n"); + (indexer, caller_uri) +} + +/// Kotlin's default-import packages (`kotlin`, `kotlin.text`, +/// `kotlin.collections`, `java.lang`, ...) are in scope in every Kotlin file +/// with no `import` line -- and no real file writes one. Before this fix +/// `extension_is_in_scope` knew only about same-package and explicit +/// imports, so it rejected all 3638 top-level default-import-packaged +/// registry entries for every caller on the Moneta corpus. +#[test] +fn a_default_import_packaged_extension_is_in_scope_without_an_explicit_import() { + let (indexer, caller_uri) = indexer_with_a_default_imported_extension("/app/Caller.kt"); + + let locations = super::extension::resolve_extension_in_scope( + &indexer, + "CharSequence", + "isNotEmpty", + &caller_uri, + ); + assert_eq!( + locations.len(), + 1, + "kotlin.text.isNotEmpty must be in scope for an unimporting Kotlin \ + caller, got {locations:?}" + ); + assert_eq!( + locations[0].range.start.line, 7, + "expected the real declaration range, got {:?}", + locations[0].range + ); +} + +/// Decision D2's guard: `resolve_qualified` runs over indexed `.java` files +/// too (the resolution-accuracy benchmark scans both `.kt` and `.java`), and +/// a Java file never implicitly imports `kotlin.*`. Same reasoning, and the +/// same Copilot review finding, that already gates +/// `default_kotlin_import_tie_break` on the origin file's language. +#[test] +fn a_default_import_packaged_extension_stays_out_of_scope_for_a_java_caller() { + let (indexer, caller_uri) = indexer_with_a_default_imported_extension("/app/Caller.java"); + + let locations = super::extension::resolve_extension_in_scope( + &indexer, + "CharSequence", + "isNotEmpty", + &caller_uri, + ); + assert!( + locations.is_empty(), + "a Java origin must not pick up Kotlin's default imports, got {locations:?}" + ); +} + +/// The new rule must NOT leak to a package that merely starts with a +/// default-import prefix: `kotlin.text.regex` is not `kotlin.text`, and +/// `is_default_import_package` is an exact-membership check, not a prefix +/// match. Guard against a future "fix" that swaps it for `starts_with`. +/// +/// Targets `extension_entry_is_in_scope`, NOT `extension_is_in_scope` — the +/// latter is the function this task deliberately leaves untouched (Part 0.3), +/// so a guard aimed at it would be green before and after by construction and +/// pin nothing. +#[test] +fn a_package_below_a_default_import_package_is_still_out_of_scope() { + use crate::resolver::infer::extension_entry_is_in_scope; + use crate::types::{ExtensionEntry, Visibility}; + use tower_lsp::lsp_types::SymbolKind; + + let indexer = Indexer::new(); + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nclass Caller\n"); + let caller_file_data = indexer.files.get(caller_uri.as_str()); + + let entry = ExtensionEntry { + file_uri: "jar:file:///kotlin-stdlib.jar!/kotlin/text/regex/RegexKt.class".to_owned(), + name: "someExtension".to_owned(), + kind: SymbolKind::FUNCTION, + detail: "fun CharSequence.someExtension(): Boolean".to_owned(), + visibility: Visibility::Public, + package: Some("kotlin.text.regex".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }; + assert!( + !extension_entry_is_in_scope( + &entry, + &caller_uri, + caller_file_data.as_deref().map(|value| value.as_ref()), + ), + "kotlin.text.regex is not one of Kotlin's default-import packages" + ); +} + +/// The third call site, on the type-inference side: an unimporting Kotlin +/// caller must now be able to infer a `kotlin.collections` extension's +/// return type, which `find_extension_fn_return_type_scoped`'s own +/// in-scope check rejected before this fix. +#[test] +fn a_default_import_packaged_extension_return_type_is_inferable_without_an_import() { + use crate::types::{ExtensionEntry, FileData, SourceSet, Visibility}; + use std::sync::Arc; + use tower_lsp::lsp_types::SymbolKind; + + let indexer = Indexer::new(); + let jar_uri = "jar:file:///kotlin-stdlib.jar!/kotlin/collections/CollectionsKt.class".to_owned(); + indexer.jar_files.insert( + jar_uri.clone(), + Arc::new(FileData { + source_set: SourceSet::Library, + package: Some("kotlin.collections".to_owned()), + lines: Arc::new(vec![]), + ..Default::default() + }), + ); + indexer + .extension_by_receiver + .entry("List".to_owned()) + .or_default() + .push(ExtensionEntry { + file_uri: jar_uri, + name: "firstOrNull".to_owned(), + kind: SymbolKind::FUNCTION, + detail: "fun List.firstOrNull(): T?".to_owned(), + visibility: Visibility::Public, + package: Some("kotlin.collections".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }); + + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nclass Caller\n"); + + assert_eq!( + crate::resolver::infer::find_extension_fn_return_type( + &indexer, + "List", + "firstOrNull", + Some(&caller_uri), + ), + Some("T?".to_owned()), + "an unimporting Kotlin caller must be able to infer a \ + kotlin.collections extension's return type" + ); +} +``` + +- [ ] **Step 2: Verify they fail for the right reason** + +Test 3 names `extension_entry_is_in_scope`, which does not exist yet, so the crate does not compile +until Step 3 adds it. **Write Step 3's function signature first (an empty body returning +`extension_is_in_scope(...)`'s result, no default-import rule yet), then run:** + +``` +cargo test --bin kmp-lsp a_default_import_packaged_extension_is_in_scope # FAIL: 0 locations +cargo test --bin kmp-lsp a_default_import_packaged_extension_stays_out_of_scope # PASS (guard) +cargo test --bin kmp-lsp a_package_below_a_default_import_package # PASS (guard) +cargo test --bin kmp-lsp a_default_import_packaged_extension_return_type # FAIL: None +``` + +Two of the four are green-before-fix by design — they guard against over-reach (a Java caller; a +package merely *below* a default-import package), so there is no bug for them to catch first. That is +deliberate; do not "fix" them into reds. The other two must be genuinely red, and the empty-body +step is what makes that observable rather than a compile error. + +- [ ] **Step 3: Implement** + +1. `src/resolver/imports.rs:46` — `fn is_default_import_package` → `pub(super) fn is_default_import_package`. +2. `src/resolver/infer.rs` — add `extension_entry_is_in_scope` exactly as written in Fix 2 above, + directly below `extension_is_in_scope`. Do **not** modify `extension_is_in_scope` itself. +3. Swap the three call sites (`extension.rs:91-98`, `extension.rs:178-185`, `infer.rs:1936-1945`) to + the wrapper. Each becomes a single `if !extension_entry_is_in_scope(entry, from_uri, caller_file_data_ref) { continue; }`. +4. Leave `src/features/nullable_call_diagnostics.rs:275`, `src/resolver/infer.rs:1626` and + `src/indexer.rs:1099` alone. Add a one-line note to `extension_is_in_scope`'s doc comment pointing + at the new wrapper, so the next reader knows which one an extension-registry consumer should use. + +- [ ] **Step 4: Verify** + +``` +cargo test --bin kmp-lsp # expect 1956 + 4 new = 1960 passed, 0 failed, 3 ignored +cargo clippy --all-targets -- -D warnings +cargo fmt -- --check +``` + +If any pre-existing test goes red, read it before changing it: a test asserting "out of scope" for a +package that is **not** in `KOTLIN_DEFAULT_IMPORT_PACKAGES` indicates a real bug in the fix (a prefix +match instead of exact membership, most likely). A test asserting out-of-scope for a package that +**is** in the set is asserting the bug and should be discussed with the controller, not silently +rewritten. (Checked in advance: `top_level_extension_function_in_unimported_package_still_out_of_scope` +at `tests.rs:7219` uses `com.example.lib` and is unaffected.) + +- [ ] **Step 5: Measure on the real corpus** + +``` +cargo build --release +time ./target/release/kmp-lsp resolution-accuracy /home/ocel/Work/Moneta/android +``` + +Success criteria: +1. **PREDICTION to verify in this run, not an established fact:** `isNotEmpty` and `forEach` leave + the member-ref Gap top-20, and the 0-arg `firstOrNull` shape starts resolving. What the probe + *did* establish is narrower — that all five probed `kotlin.text`/`kotlin.collections` entries are + `in_scope=false` today. It did **not** establish that those names are currently in the Gap top-20 + (the 7458-entry member Gap was not enumerated by name in this plan's probe), nor that scope is + their *only* blocker — a name can clear the scope check and still Gap on receiver-type inference, + arity, or a missing anchor. **Step 1 of this measurement is therefore to record the before-run's + Gap top-20 and check whether these names are even in it.** If they are not, this criterion is + unfalsifiable as written and the real criterion is #2 alone; say so in the PR rather than claiming + a win the run cannot support. +2. Member recall up. This is the cluster's biggest lever — 3641 previously-unreachable registry + entries become reachable — so a movement outside the noise band is expected here, unlike Task 1. +3. **Watch `FilteredCandidate` (baseline 6234).** Handing the shape filter more candidates is the + point, but a large rise means in-scope candidates are being surfaced and then rejected on arity, + which would point at Task 3/Task 4 territory. Record the number either way; a rise is not by itself + a reason to hold the PR. + +- [ ] **Step 6: Commit and open a PR** + +The PR description must state: why the fix is a new wrapper rather than an edit to +`extension_is_in_scope` (four of six callers are not about extensions — Part 0.3), why the language +gate is there (Decision D2, with the `.java`-files-are-scanned evidence), and that +`nullable_call_diagnostics` was deliberately left out. + +```bash +git commit -m "fix(resolve): count Kotlin's default-import packages as extension scope + +Co-Authored-By: Claude Sonnet 5 " +``` + +--- + +### Task 3: `implicit_receiver_extension_match` selects its own entry's overload + +**Depends on Task 2** (same file). No corpus measurement required. + +**Files:** +- Modify: `src/resolver/extension.rs` — split `select_extension_symbol_range` (`:28-54`), rewrite the + symbol lookup in `implicit_receiver_extension_match` (`:192-209`) +- Modify: `src/features/rename.rs` — Decision D5's `//` line comment above the + `definitions.len() != 1` refusal (`:106-110`). It is inside `rename_impl`'s body, so a `///` doc + comment would not compile there. Comment only, no behaviour change. +- Modify: `src/resolver/tests.rs` — 1 new test + +**Interfaces:** new `pub(super) fn select_extension_symbol(&FileData, &str, &str, Option<&String>, +&str) -> Option<&SymbolEntry>`. `select_extension_symbol_range` keeps its exact signature and both +existing callers. + +- [ ] **Step 1: Write the failing test** + +```rust +/// PR #321's overload-collapse family, on the implicit-receiver entry point +/// (`resolve_implicit_receiver_callee` -> `implicit_receiver_extension_match`). +/// The registry correctly holds one entry per overload, but for EVERY entry +/// the loop re-ran the same `extension_declaration_matches` `.find(...)` over +/// the declaring file -- and that predicate compares only +/// (name, receiver, container), identical across overloads -- so every +/// iteration shape-checked the FIRST-declared overload and a differently +/// shaped call was rejected on all of them. The 1-arg overload was +/// unreachable through this entry point entirely. +#[test] +fn an_implicit_receiver_call_reaches_the_arity_matching_extension_overload() { + use crate::indexer::CallShape; + + let indexer = Indexer::new(); + let extensions_uri = uri("/app/Extensions.kt"); + indexer.index_content( + &extensions_uri, + concat!( + "package app\n", + "fun Foo.describe(): String = \"\"\n", + "fun Foo.describe(prefix: String): String = prefix\n", + ), + ); + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content( + &caller_uri, + concat!( + "package app\n", + "class Foo\n", + "fun Foo.use() {\n", + " describe(\"x\")\n", + "}\n", + ), + ); + + let shape = CallShape { + arg_count: 1, + trailing_lambda: false, + }; + let locations = + crate::resolver::resolve_implicit_receiver_callee(&indexer, "Foo", "describe", &caller_uri, shape); + assert_eq!( + locations.len(), + 1, + "expected the 1-arg overload, got {locations:?}" + ); + assert_eq!( + locations[0].range.start.line, 2, + "expected the SECOND declaration (the 1-arg overload, line index 2), \ + got {:?}", + locations[0].range + ); +} +``` + +- [ ] **Step 2: Verify it fails for the right reason** + +``` +cargo test --bin kmp-lsp an_implicit_receiver_call_reaches_the_arity_matching_extension_overload +``` + +Must fail with **0 locations** (both entries selected the 0-arg declaration, whose arity the 1-arg +shape rejects). If it fails with 1 location at line 1, the loop is picking the wrong overload for a +different reason — investigate before implementing. If it passes, the fixture is leaking through +`implicit_receiver_member_match` or a bare-name fallback; add a decoy so it does not. + +- [ ] **Step 3: Implement** + +Split `select_extension_symbol_range` into `select_extension_symbol` + a two-line +`select_extension_symbol_range` wrapper, exactly as written in Fix 3 above. Move +`select_extension_symbol_range`'s existing doc comment onto `select_extension_symbol` (it describes +the selection, not the range extraction) and leave a one-line comment on the wrapper. + +In `implicit_receiver_extension_match`, replace `:196-208`'s +`.and_then(|fd| fd.symbols.iter().find(|s| …).cloned())` with + +```rust +let symbol = indexer + .files + .get(&entry.file_uri) + .or_else(|| indexer.jar_files.get(&entry.file_uri)) + .and_then(|file_data| { + select_extension_symbol( + &file_data, + name, + receiver_base, + entry.container.as_ref(), + &entry.detail, + ) + .cloned() + }); +``` + +Note the closure parameter rename from `fd` to `file_data` and the inner `|s|` disappearing. Do both, +but for the right reason: AGENTS.md's no-abbreviated-names rule, not the pre-push hook. The hook's +abbreviation check is a fixed token list (`\b([sc])\b` plus a specific suffix list) that does **not** +catch `fd` or `pkg` — so this rename is house style, not gate-enforced, and nobody should be told +otherwise and then discover the gate was never watching. + +Then add Decision D5's note to `src/features/rename.rs`, above the `definitions.len() != 1` refusal: + +```rust +// A same-arity extension collision (two unrelated `Modifier.weight` +// declarations, say) reaches this refusal BY DESIGN since PR #321: +// `resolve_identity` correctly returns the whole overload set rather than +// one arbitrary member of it, and silently renaming an arbitrarily chosen +// one of two genuinely distinct declarations is the unsound behaviour that +// PR replaced. Listing both candidates in the message would not help -- +// an LSP error is surfaced as a toast with no navigation affordance -- so a +// real disambiguation UX needs a client-side picker this server has no +// protocol hook for. Reviewed and kept as-is, 2026-09-16. +``` + +- [ ] **Step 4: Verify** + +``` +cargo test --bin kmp-lsp # expect Task 2's count + 1 new +cargo clippy --all-targets -- -D warnings +cargo fmt -- --check +``` + +- [ ] **Step 5: Spot-check on the real corpus (no full benchmark)** + +This entry point has no currently-measured Gap name pointing at it and the change is confined to one +loop, so a full `resolution-accuracy` run is not the right instrument. Instead: + +``` +cargo build --release +cd /home/ocel/Work/Moneta/android +# pick a call inside an extension function's own body that targets a +# same-receiver overload set -- `rg -n "^fun [A-Z]\w*\." --glob '*.kt'` finds +# candidate declaring files; the registry groups named in Fix 3 +# (HttpClientCall.receive, Writer.write) are the shape to look for. +kmp-lsp definition +``` + +Record one before/after pair in the PR description. If no such site can be found on this corpus, +say so explicitly and rest on the unit test — do not invent a site. + +- [ ] **Step 6: Commit and open a PR** + +```bash +git commit -m "fix(resolve): select the implicit-receiver extension overload the call shape wants + +Co-Authored-By: Claude Sonnet 5 " +``` + +--- + +### Task 4: `find_extension_fn_return_type_scoped` — per-entry selection, no loop abort + +**Depends on Task 3** (consumes `select_extension_symbol`). No corpus measurement. + +**Files:** +- Modify: `src/resolver/infer.rs` (`find_extension_fn_return_type_scoped`, `:1955-1974`) +- Modify: `src/resolver/infer_tests.rs` — 2 new tests + +**Interfaces:** none change. + +- [ ] **Step 1: Write the failing tests** + +> **Both tests below were written into the tree, run red, fixed, and run green before this plan was +> finalized** — see Appendix C. Their first draft was **false-green** and is preserved in Appendix C +> as a trap to avoid repeating. Two things make this fixture shape mandatory: +> +> 1. **`index_content` auto-registers extensions.** `ExtensionContribution::apply` +> (`src/indexer/apply.rs:504-509`) and the merge path (`:730-733`) populate +> `extension_by_receiver` from every indexed file. A fixture that indexes the declaring file *and* +> hand-pushes its own `ExtensionEntry`s ends up with both sets in the bucket, and the auto-registered +> ones — which are correct — shadow the hand-written ones so the buggy path never runs. +> **Let `index_content` build the entries; do not hand-push alongside it.** +> 2. **A hand-written `entry.detail` can never equal a parsed `SymbolEntry.detail`.** The fallback path +> this task fixes only runs when `extract_return_type_from_detail(&entry.detail)` returns `None`, +> i.e. when the detail is truncated past the return type — and the fix works by matching +> `entry.detail` against `symbol.detail`. Author a mangled `entry.detail` by hand and the two can +> never match, so `select_extension_symbol` always falls back to the first declaration and the test +> stays red *after* the fix too. **Force the truncation with a genuinely long signature in real +> source**, the way `extension_fn_return_type_scoped_resolves_member_extension_via_truncated_detail` +> (`infer_tests.rs:702`) already does with `"w".repeat(150)`: both details then come from the same +> `MAX_DETAIL_CHARS = 120` truncation (`src/parser.rs:1219`) and are equal by construction, and +> `lines` is populated so `collect_signature` can still recover the real return type. +> +> Also note `src/resolver/infer_tests.rs` has **no module-level `uri()` helper** — it is redeclared +> inside each test (`:109`, `:170`, `:214`, `:256`, `:1753`). Both tests below follow that convention +> and import `Indexer` themselves. + +Append to `src/resolver/infer_tests.rs`, next to +`extension_fn_return_type_scoped_resolves_member_extension_via_truncated_detail` (`:702`). + +```rust +/// The truncated-`detail` fallback picked the FIRST declaration in the +/// declaring file matching (name, receiver, container) -- a predicate +/// identical across every overload -- so a second overload's return type was +/// unreachable through it, no matter which registry entry the loop was on. +/// PR #321's `select_extension_symbol` already solves exactly this +/// disambiguation by preferring the declaration whose full signature text +/// equals the registry entry's own. +/// +/// Both overloads are made long enough that `detail` truncates past the +/// return type (`MAX_DETAIL_CHARS`, `src/parser.rs:1219`), forcing the +/// `collect_signature` fallback -- the only path carrying the bug -- while +/// still differing inside the first 120 characters (`alpha` vs `beta`) so the +/// two truncated details remain distinguishable. +#[test] +fn extension_fn_return_type_scoped_selects_the_overload_the_entry_names() { + use super::find_extension_fn_return_type; + use crate::indexer::Indexer; + use tower_lsp::lsp_types::Url; + + fn uri(path: &str) -> Url { + Url::parse(&format!("file://{path}")).unwrap() + } + + let indexer = Indexer::new(); + let declaring_uri = uri("/app/Extensions.kt"); + let long_parameter = "w".repeat(150); + indexer.index_content( + &declaring_uri, + &format!( + "package app\n\ + class Foo\n\ + fun Foo.describe(alpha: Int, {long_parameter}: Int): String = \"\"\n\ + fun Foo.describe(beta: Int, {long_parameter}: Int): Boolean = true\n" + ), + ); + + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nfun use() {}\n"); + + // `find_extension_fn_return_type` returns on the first entry that yields + // a return type, so the first entry's (correct) answer would mask the + // bug. Drop it and the SECOND entry has to stand on its own -- which is + // exactly what it cannot do today. + indexer + .extension_by_receiver + .entry("Foo".to_owned()) + .or_default() + .remove(0); + + assert_eq!( + find_extension_fn_return_type(&indexer, "Foo", "describe", Some(&caller_uri)), + Some("Boolean".to_owned()), + "the remaining entry names the SECOND overload, so its return type \ + must be Boolean, not the first declaration's String" + ); +} + +/// An entry whose declaring file is not loaded must skip to the next entry, +/// not abort the whole search: both `indexer.files.get(...)?` +/// (`infer.rs:1958`) and `.find(...)?` (`:1969`) returned `None` from the +/// FUNCTION rather than continuing the loop, so one unusable entry hid every +/// usable one behind it. +/// +/// This entry is hand-built on purpose -- it must point at a file that is +/// deliberately never indexed, which `index_content` cannot produce -- and it +/// is `insert`ed ahead of the auto-registered real one so it is reached first. +/// Its `detail` carries no return type, so the loop falls past the +/// `extract_return_type_from_detail` shortcut to the file lookup that aborts. +#[test] +fn extension_fn_return_type_scoped_skips_an_entry_whose_file_is_missing() { + use super::find_extension_fn_return_type; + use crate::indexer::Indexer; + use crate::types::{ExtensionEntry, Visibility}; + use tower_lsp::lsp_types::{SymbolKind, Url}; + + fn uri(path: &str) -> Url { + Url::parse(&format!("file://{path}")).unwrap() + } + + let indexer = Indexer::new(); + let declaring_uri = uri("/app/Extensions.kt"); + indexer.index_content( + &declaring_uri, + "package app\nclass Foo\nfun Foo.describe(): String = \"\"\n", + ); + + indexer + .extension_by_receiver + .entry("Foo".to_owned()) + .or_default() + .insert( + 0, + ExtensionEntry { + file_uri: "file:///app/NeverIndexed.kt".to_owned(), + name: "describe".to_owned(), + kind: SymbolKind::FUNCTION, + detail: "fun Foo.describe(".to_owned(), + visibility: Visibility::Public, + package: Some("app".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }, + ); + + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nfun use() {}\n"); + + assert_eq!( + find_extension_fn_return_type(&indexer, "Foo", "describe", Some(&caller_uri)), + Some("String".to_owned()), + "an entry pointing at an unindexed file must be skipped, not abort \ + the search over the remaining entries" + ); +} +``` + +- [ ] **Step 2: Verify they fail for the right reason** + +Observed, not predicted — this is the exact output from running them against unmodified `878cc4ea`: + +``` +extension_fn_return_type_scoped_selects_the_overload_the_entry_names ... FAILED + left: Some("String") right: Some("Boolean") <- first-match bug +extension_fn_return_type_scoped_skips_an_entry_whose_file_is_missing ... FAILED + left: None right: Some("String") <- early-abort bug +``` + +If either shows something else — an empty bucket, a panic, or `None` on test 1 — the fixture is not +reaching the `collect_signature` fallback. Do **not** weaken the assertion; re-read the fixture note +above, since one of its two traps is almost certainly back. + +- [ ] **Step 3: Implement** + +In `src/resolver/infer.rs:1955-1974`: + +```rust +let Some(file_data) = indexer + .files + .get(&entry.file_uri) + .or_else(|| indexer.jar_files.get(&entry.file_uri)) +else { + // Not `?`: one entry whose declaring file isn't loaded must not abort + // the search over the remaining entries. + continue; +}; +let Some(declaring_symbol) = crate::resolver::extension::select_extension_symbol( + &file_data, + method_name, + receiver_base, + entry.container.as_ref(), + &entry.detail, +) else { + continue; +}; +let start_line = declaring_symbol.selection_start() as usize; +let full_signature = file_data.lines.collect_signature(start_line); +if let Some(return_type) = extract_return_type_from_detail(&full_signature) { + return Some(return_type); +} +``` + +`select_extension_symbol` is `pub(super)` in `resolver::extension`, so `resolver::infer` reaches it +as a sibling submodule — no visibility change needed. + +Add a short comment above `find_extension_fn_return_type_global` recording Part 0.4: it has no +`ExtensionEntry` and no `detail`, so the two-step selection does not apply, and +`Indexer::find_method_return_type_for_type` always passes `Some(uri)` so it is not on a production +path. Also record Decision D4 there in one sentence — arity-aware selection needs a `CallShape` +through `Resolver::method_return_type`, 482 registry groups on the Moneta corpus have +differing-return overloads, and that is its own plan. + +- [ ] **Step 4: Verify** + +``` +cargo test --bin kmp-lsp # expect Task 3's count + 2 new +cargo clippy --all-targets -- -D warnings +cargo fmt -- --check +``` + +- [ ] **Step 5: Commit and open a PR** + +The PR description must state what was fixed (per-entry selection + the loop abort), what was +deliberately **not** fixed (arity-aware overload choice), and the 482-group measurement that +motivates the follow-up plan. + +```bash +git commit -m "fix(infer): select the named overload in scoped extension return-type lookup + +Co-Authored-By: Claude Sonnet 5 " +``` + +--- + +### Item 5 — closed by Decision D5, no task + +Ship the current refusal as-is. The only artefact is the doc comment added in Task 3 Step 3. See +Decision D5 for the reasoning; if the controller disagrees, the alternative is a client-side +disambiguation picker, which is a protocol-level feature and belongs in its own plan, not a message +tweak. + +--- + +## Appendix A — how Part 0 was verified + +One throwaway instrument, since deleted. A `throwaway_probe(&index, root)` function appended to +`src/cli/resolution_accuracy_poc.rs`, called from `run_resolution_accuracy` behind +`if std::env::var("KMP_PROBE").is_ok()` placed immediately **after** the benchmark's own JAR indexing +and **before** its file loop, so it saw the benchmark's exact index state. `mod platform_types;` in +`src/resolver/mod.rs` was temporarily widened to `pub(crate)`. Run as: + +``` +cd /home/ocel/Work/Moneta/android +KMP_PROBE=1 target/release/kmp-lsp resolution-accuracy /home/ocel/Work/Moneta/android +``` + +It printed four blocks: + +1. **Built-in names.** For each of the 22 entries in `KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`: + `indexer.lookup_definitions(name).len()`, the first four candidate URIs, + `resolve_kotlin_builtin_type_platform_equivalent(index, name)`, and + `resolve_symbol_index_only(index, name, None, &caller)` where `caller` was the + lexicographically-first indexed workspace `.kt` file. → Part 0.2's table. +2. **Extension scope.** For `(CharSequence, isNotEmpty)`, `(String, isNotEmpty)`, + `(Iterable, firstOrNull)`, `(Iterable, forEach)`, `(List, firstOrNull)`: every registry entry's + `package`, `container`, `detail`, and `extension_is_in_scope(…)` against that same caller. All + `false`. → Fix 2's evidence table. +3. **Registry census.** 2360 buckets / 24897 entries / 3641 in a `KOTLIN_DEFAULT_IMPORT_PACKAGES` + package / 3638 of those top-level (`container.is_none()`). +4. **Overload groups.** Grouping every bucket's entries by `name`: 4211 groups with ≥2 entries, 482 + with differing crudely-extracted return types, 2589 with ≥2 entries declared in the same file, + plus 12 examples of each. → Fix 3's and Decision D4's evidence. + +Both edits reverted; `git status` clean; `cargo test --bin kmp-lsp` = 1956 passed / 0 failed / +3 ignored at the state this document was written. + +The call-graph claims (Part 0.3, Decision D2, Part 0.4's caller chain, Part 0.5's +`QualifierRoot::TypePath` gate) were established by reading the actual call sites, not by probe — +each is cited with its file and line above. + +## Appendix B — what changed from the context brief + +| # | Brief's claim | This plan | +|---|---|---| +| 0.1 | Item 1 is a false-**unique**-win: `lookup_definitions("List")` returns exactly one candidate. | **False.** 32 candidates; `default_kotlin_import_tie_break` narrows them to the `kotlin.collections` decoy. Different mechanism, and a "unique-match" fix would have missed `List` entirely. | +| 0.2 | The reorder "changes resolution for every name in that table (10+ names)"; `String` "not yet independently re-confirmed". | Enumerated all 22 on the corpus: **15 unchanged, 7 fixed**. `String` re-confirmed correct today and unaffected. The 7 split into two distinct failure shapes (tie-break loss vs. genuine unique decoy). | +| 0.3 | `extension_is_in_scope` has "only two callers"; fix it in place. | **Six callers**, four of them not about extensions — including `Indexer::jar_candidate_is_reachable`, which gates bare-name JAR candidate preference corpus-wide. Design changed to a new entry-shaped wrapper; the shared predicate is untouched. | +| 0.4 | Item 4's `_scoped` and `_global` are the same bug; fix both with `select_extension_symbol_range`'s pattern. | **Different shapes.** `_global` has no `ExtensionEntry` and no `detail`, so the pattern cannot apply, and no production caller reaches it. `_scoped` gains the pattern **plus** a newly-found early-abort bug (two `?`s that return from the function instead of the loop). | +| 0.4 / D4 | Item 4 should get "arity awareness". | **Scoped out with evidence.** No `CallShape` exists anywhere in the chain; adding one is a `Resolver`-trait change. Measured 482 differing-return registry groups as motivation for a separate plan, and flagged that 482 counts *groups*, not call sites. | +| Open question | "Does `extension_is_in_scope` ever run for a non-Kotlin origin file?" | **Resolved: yes.** `resolve_qualified` is language-agnostic and the `resolution-accuracy` benchmark itself scans `.java` files. The language gate is required, and is pinned by its own test (Decision D2). | +| Item 2's "compounding factor" | `jar_extension_for_type_root` is scope-blind, so the `CharSequence` case "worked" by same-leaf coincidence. | **Sharpened:** that fallback is reachable only for a `QualifierRoot::TypePath`. A real `text.isNotEmpty()` call has a *value* root and never reaches it — which is why the scope rejection is fatal in practice and why a naive end-to-end test of item 2 is green before the fix. | +| Item 5 | "Decide (a) ship as-is or (b) small UX improvement." | **Decision D5: (a), closed.** With a `//` comment recording why, folded into Task 3. | +| New | — | **Decision D1:** the reorder makes the un-memoized `detect_android_sdk_source_paths` run on hot names. A path-keyed memo ships in Task 1 itself. | +| New | — | **Fix 1 + Fix 2 are jointly necessary** for the trailing-lambda `firstOrNull` shape: even with a correct `List` anchor, the `Iterable`-keyed overloads are out of scope until Fix 2 lands. Stated so Task 1's measurement is not read as a failure. | + +## Appendix C — Task 4's tests were run, red then green, before this plan shipped + +Added after an independent review found Task 4's first-draft tests were **false-green**: both passed +against unmodified code, so Step 2's stated failures were predictions that had never been observed. +The rebuilt tests in Task 4 Step 1 were pasted into `src/resolver/infer_tests.rs` in this worktree +and run for real. + +**Red, against unmodified `878cc4ea`:** + +``` +running 4 tests +test …extension_fn_return_type_scoped_promotes_a_cache_backed_tier1_only_symbol ... ok +test …extension_fn_return_type_scoped_resolves_member_extension_via_truncated_detail ... ok +test …extension_fn_return_type_scoped_skips_an_entry_whose_file_is_missing ... FAILED +test …extension_fn_return_type_scoped_selects_the_overload_the_entry_names ... FAILED + +selects_the_overload_the_entry_names: left: Some("String") right: Some("Boolean") +skips_an_entry_whose_file_is_missing: left: None right: Some("String") +``` + +A temporary probe inside test 1 also dumped the auto-registered entries, confirming the two details +truncate to distinguishable strings — the property the fix depends on: + +``` +"fun Foo.describe(alpha: Int, wwwwwww…" +"fun Foo.describe(beta: Int, wwwwwwww…" +``` + +**Green, with Task 3's `select_extension_symbol` split and Task 4's implementation applied exactly as +written in those tasks:** + +``` +test …extension_fn_return_type_scoped_skips_an_entry_whose_file_is_missing ... ok +test …extension_fn_return_type_scoped_selects_the_overload_the_entry_names ... ok +``` + +Full suite at that state: **1958 passed, 0 failed, 3 ignored** (= the 1956 baseline + these 2), and +`cargo clippy --all-targets -- -D warnings` clean. That also pre-validates Task 3's helper split and +Task 4's implementation compile and break nothing — the two production diffs were written and +compiled, not just designed. All of it was then reverted; only this plan file is staged. + +**What the false-green first draft got wrong, recorded so it is not repeated:** it hand-pushed +`ExtensionEntry`s *alongside* an `index_content` call that already auto-registers them +(`src/indexer/apply.rs:504-509`), so the correct auto-entries shadowed the fixture and the buggy path +never ran; and it hand-authored a mangled `entry.detail` to force the truncated-detail fallback, +which by construction can never equal a parsed `SymbolEntry.detail` — so `exact_signature_match` +could never fire and the test would have stayed red even *after* a correct fix. Both traps are now +called out in Task 4 Step 1's fixture note. + +## Appendix D — what changed in the independent-review round + +An independent reviewer re-derived every claim from scratch against the repo and corpus, and ran +several of the plan's own tests. Verdict: specific fixes, not a rewrite. Everything the reviewer +**confirmed** — the 32-candidate claim, the 7-of-22 table (re-run against three different caller +files, identical result), six-callers-not-two, the `_global`/`_scoped` split, the two `?` early-abort +bugs at `infer.rs:1958`/`:1969`, Task 3's test being a genuine red, the Task 3 → Task 4 helper +signature match, the ordering/conflict analysis, and the commit trailers — is retained unchanged. + +| # | Finding | Resolution | +|---|---|---| +| **1 (blocker)** | Task 4's two tests were **false-green** — both passed against unmodified code, proven by running them. Two causes: `index_content` auto-registers extensions (`apply.rs:504-509`), shadowing the hand-pushed fixture; and a hand-authored `entry.detail` can never equal a parsed `SymbolEntry.detail`, so the fix could never fire. | **Fixed.** Both tests rebuilt on the real-source truncation pattern from `infer_tests.rs:702`, then **actually run** — real red, then real green with the fix applied, full suite 1958/0/3, clippy clean. Evidence in **Appendix C**, including both traps written into Task 4 Step 1's fixture note. | +| **2** | Task 4's tests would not compile: `infer_tests.rs` has no module-level `uri()` helper and they never imported `Indexer`. | **Fixed.** Both now declare a local `uri` and `use crate::indexer::Indexer;`, matching the file's per-test convention. Verified by the run. | +| **3** | `let _ = (SourceSet::Library, Arc::new(FileData::default()));` was a junk line silencing unused imports — the "trim if unused" note shipped as a placeholder. | **Fixed.** Line and both unused imports gone. | +| **4** | Task 2's third test guarded `extension_is_in_scope` — the function Task 2 explicitly does *not* modify — so it was green before and after by construction. | **Fixed.** Rewritten to call `extension_entry_is_in_scope` with a `kotlin.text.regex`-packaged `ExtensionEntry`, pinning the new wrapper's exact-membership rule. Step 2 now also says to stub the signature first so the other two reds are observable rather than a compile error. | +| **5** | D1's generation-keyed-memo-deferred-behind-a-measurement was over-built; and the plan missed that `workspace_json.rs:681` `log::info!`s on every detection, which is its own reason not to ship unmemoized even briefly. | **Adopted, not argued.** D1 rewritten: a path-keyed `Mutex)>>` memo, ~15 lines, **shipping in Task 1** rather than deferred. Placed in `platform_types.rs` (the only hot caller) rather than `workspace_json.rs`. Wall clock demoted from gate to record; a log-line-count check added as the direct memo verification. | +| **6** | "3638 of the registry's 24897 top-level entries" conflated two different denominators; "all four `tests.rs` edits" was a count error (only three tasks touch `tests.rs`). | **Fixed,** both. | +| **7** | The pre-push hook's abbreviation check is a fixed token list that does not catch `fd`/`pkg`, so citing it as the authority for Task 3's `fd` → `file_data` rename was an overclaim. | **Fixed.** Rename kept (AGENTS.md is the authority); the Global Constraints block now says explicitly that the hook is a partial backstop, not the definition of the rules. | +| **8** | D5's "LSP clients surface this as a toast" claim — the load-bearing reason for choosing (a) — was never verified against a real client. Files table called a function-body comment a "doc-comment note". | **Fixed.** The claim is now labelled **ASSUMPTION** with what would follow if it is wrong (reasons 1/2/4 carry the decision regardless). Both Files-table entries corrected to `//` line comment. | +| **9** | Task 2 Step 5 stated "`isNotEmpty`/`forEach` leave the Gap top-20" as fact; the probe only established they are out of scope today, not that they are in the top-20 or that scope is their only blocker. | **Fixed.** Reworded as a prediction, with an explicit first step to record the before-run's Gap top-20 and check whether the names are even in it — and instructions to say so in the PR if the criterion turns out unfalsifiable. | +| **10** | `InitUseCase.kt:73` is a `data class Double` **nested** in `sealed class IntRates`, not a top-level declaration. | **Fixed** in both places (design prose and the guard test's doc comment). Confirmed independently by reading the file. The guard test uses its own top-level fixture and was never affected. | From b12c87eb779b1d9b801c6f183cc8eadaebfcfe39 Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Wed, 16 Sep 2026 10:11:03 +0200 Subject: [PATCH 2/7] fix(resolve): prefer a built-in type's platform declaration over a same-named decoy resolve_chain's four IO-policy tails (Full, NoRg, IndexOnly, HierarchyAmbiguitySafe) each fell back to resolve_kotlin_builtin_type_platform_equivalent only after the global-definitions lookup came up empty. For 7 of the 22 names in KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS that lookup does not come up empty -- it returns a same-named decoy from inside a JAR, either via default_kotlin_import_tie_break narrowing List's 32 candidates to a body-less kotlin.collections decoy, or via a unique-match shortcut for the five Mutable* names, each of which has exactly one same-named candidate (a kotlin-gradle-plugin decoy). Anchoring on a decoy with no compiled body leaves the type with no walkable supertype chain, so no member or extension lookup downstream can ever reach it. Reorder all four tails to try the platform equivalent first, below steps 1-4.5 (local declaration, imports, same package, star imports, class hierarchy) so a real workspace declaration still wins. The reorder puts an unmemoized detect_android_sdk_source_paths (local.properties read + read_dir + a log::info! per success) on the hot path of every built-in-name resolution. Add a path-keyed memo (android_sdk_source_roots) in the same change, keyed on the workspace root since WorkspaceRoot::set can change it mid-session. Co-Authored-By: Claude Sonnet 5 --- src/resolver/platform_types.rs | 57 +++++++++-- src/resolver/resolve.rs | 93 +++++++++-------- src/resolver/tests.rs | 182 ++++++++++++++++++++++++++++++++- 3 files changed, 280 insertions(+), 52 deletions(-) diff --git a/src/resolver/platform_types.rs b/src/resolver/platform_types.rs index fbca906e..e8158f84 100644 --- a/src/resolver/platform_types.rs +++ b/src/resolver/platform_types.rs @@ -1,6 +1,9 @@ //! Kotlin/Swift stdlib package detection and the built-in-type → JVM-platform-type //! fallback (see `docs/superpowers/specs/2026-08-27-kotlin-builtin-type-platform-mapping-design.md`). +use std::path::{Path, PathBuf}; +use std::sync::Mutex; + use tower_lsp::lsp_types::{Location, Url}; use crate::indexer::Indexer; @@ -85,12 +88,21 @@ const KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS: &[(&str, &str)] = &[ ("Char", "java.lang.Character"), ]; -/// Last-resort fallback for a Kotlin compiler-intrinsic built-in type name -/// (see [`KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`]): every normal -/// resolution step already failed by the time any tail fallback calls this, -/// since a built-in type is never locally declared, explicitly imported, or -/// present in the workspace's own source tree — so this can only ever turn -/// an existing decline into a correct resolve, never introduce a wrong one. +/// Fallback for a Kotlin compiler-intrinsic built-in type name (see +/// [`KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`]), tried in `resolve_chain`'s +/// tail *ahead of* the global-definitions lookup, not behind it: for the ~22 +/// names in that table, the real platform declaration outranks any +/// same-named index/JAR candidate, because those names have no compiled +/// `.class` anywhere and every same-named candidate is therefore a decoy — +/// verified on the Moneta corpus, e.g. bare `List` has 32 same-named +/// candidates and the tie-break machinery picks a body-less +/// `kotlin.collections` decoy from among them, and `MutableSet` has exactly +/// one same-named candidate (a `kotlin-gradle-plugin` decoy) that wins +/// outright as a unique match. Steps 1–4.5 (local declaration, explicit +/// imports, same package, star imports, class hierarchy) still run first, so +/// a workspace file that genuinely declares its own `class Double` is +/// unaffected — this fallback only ever fires once those have already +/// declined. /// /// Re-derives the Android SDK sources root via the already-existing /// [`crate::workspace_json::detect_android_sdk_source_paths`] (no new @@ -129,7 +141,7 @@ pub(crate) fn resolve_kotlin_builtin_type_platform_equivalent( return vec![]; }; let relative_path = platform_fqn.replace('.', "/") + ".java"; - for sdk_source_root in crate::workspace_json::detect_android_sdk_source_paths(&workspace_root) { + for sdk_source_root in android_sdk_source_roots(&workspace_root) { let file_path = sdk_source_root.join(&relative_path); let Ok(file_uri) = Url::from_file_path(&file_path) else { continue; @@ -148,3 +160,34 @@ pub(crate) fn resolve_kotlin_builtin_type_platform_equivalent( } vec![] } + +/// Memoized [`crate::workspace_json::detect_android_sdk_source_paths`], keyed +/// on the workspace root it was computed for. +/// +/// That function does real filesystem work on every call — a +/// `local.properties` read, an `is_dir`, and a `read_dir` of `sdk/sources/` — +/// plus a `log::info!` on every success. Harmless while the built-in-type +/// fallback only ran after the whole resolution chain had already declined; +/// not harmless now that it runs ahead of the tail for every name in +/// [`KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`], which is the hottest name +/// class in a Kotlin corpus and sits on the hover/inlay path. +/// +/// Keyed on the root path rather than cached once, because +/// `WorkspaceRoot::set` can change the root mid-session. An SDK installed +/// *after* the first probe is not picked up until the root changes — the same +/// staleness the workspace scan already has, since it reads these paths once +/// at startup (`src/workspace/mod.rs:133`, `src/cli/run.rs:284`). +fn android_sdk_source_roots(workspace_root: &Path) -> Vec { + static DETECTED_ROOTS: Mutex)>> = Mutex::new(None); + let Ok(mut detected) = DETECTED_ROOTS.lock() else { + return crate::workspace_json::detect_android_sdk_source_paths(workspace_root); + }; + if let Some((cached_root, cached_paths)) = detected.as_ref() { + if cached_root == workspace_root { + return cached_paths.clone(); + } + } + let paths = crate::workspace_json::detect_android_sdk_source_paths(workspace_root); + *detected = Some((workspace_root.to_path_buf(), paths.clone())); + paths +} diff --git a/src/resolver/resolve.rs b/src/resolver/resolve.rs index f40246ef..94b9f331 100644 --- a/src/resolver/resolve.rs +++ b/src/resolver/resolve.rs @@ -386,7 +386,21 @@ fn resolve_chain( if !rg_result.is_empty() { return rg_result; } - // 5.4 ── global definitions index (includes JAR symbols) ─────────────── + // 5.4 ── Kotlin built-in-type platform equivalent ────────────────────── + // Ahead of the global definitions tail, not behind it: for the ~22 + // compiler-intrinsic mapped types in + // KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS there is no compiled `.class` + // anywhere, so every same-named index/JAR candidate is a decoy -- and a + // decoy wins today, either outright (MutableSet: one candidate) or via + // default_kotlin_import_tie_break, which prefers the `kotlin.collections` + // decoy out of `List`'s 32 candidates. Anchoring `List` on a body-less + // decoy leaves it with no walkable supertype chain, so no member or + // extension lookup downstream can ever reach `Iterable`. + let platform_equivalent = resolve_kotlin_builtin_type_platform_equivalent(indexer, name); + if !platform_equivalent.is_empty() { + return platform_equivalent; + } + // 5.5 ── global definitions index (includes JAR symbols) ─────────────── // `rg`/`fd` only search the *workspace's own* source tree, so a type // used purely through inference and never explicitly imported (Kotlin // doesn't require an import for that) — e.g. `scope.async { }.await()`, @@ -408,20 +422,12 @@ fn resolve_chain( .collect(), None => jar_tail_candidates, }; - let jar_tail = ambiguity_safe_tail_with_denylist(indexer, from_uri, jar_tail_candidates); - if !jar_tail.is_empty() { - return jar_tail; - } - // 5.5 ── Kotlin built-in-type platform equivalent (last resort) ──────── - // `rg`/`fd` search the *workspace's own* source tree and can never find - // `String`/`CharSequence`: those are compiler intrinsics with no - // compiled `.class` file in kotlin-stdlib's JAR at all, and no - // in-workspace source either -- see - // docs/superpowers/specs/2026-08-27-kotlin-builtin-type-platform-mapping-design.md. - return resolve_kotlin_builtin_type_platform_equivalent(indexer, name); + return ambiguity_safe_tail_with_denylist(indexer, from_uri, jar_tail_candidates); } - // Tail fallback — global definitions index (includes JAR symbols). + // Tail fallback — Kotlin built-in-type platform equivalent first (see the + // 5.4 comment above the `Full`/rg branch), then the global definitions + // index (includes JAR symbols). // - NoRg: first match. // - ScopedOnly: no tail at all (empty) -- see the variant's doc comment. // - IndexOnly / HierarchyAmbiguitySafe: unique match wins outright, else @@ -433,52 +439,55 @@ fn resolve_chain( // on the Moneta corpus (13 candidates for bare `String`, including a // `com.android.internal.*`-packaged one) when it used the older, // plain unique-match-only rule. - // - Full: never reached — it has its own equivalent tail (5.4 above, - // same `ambiguity_safe_tail_with_denylist` call) inside the rg branch, - // since Full always returns from within that `if full_io` block. + // - Full: never reached — it has its own equivalent tail (5.4/5.5 above, + // same platform-equivalent-first order, same + // `ambiguity_safe_tail_with_denylist` call) inside the rg branch, since + // Full always returns from within that `if full_io` block. // - // Each non-`ScopedOnly` arm falls through to - // `resolve_kotlin_builtin_type_platform_equivalent` when its own lookup - // comes up empty -- see the 5.5 comment above the `Full`/rg branch. - // `ScopedOnly` is deliberately excluded: "no tail at all" is its own - // documented contract (its callers already have their own downstream - // fallback), so it must not gain one here. + // Each non-`ScopedOnly` arm tries + // `resolve_kotlin_builtin_type_platform_equivalent` FIRST, ahead of its own + // global-definitions lookup -- see the 5.4 comment above the `Full`/rg + // branch for why: for the built-in-type table, every same-named + // index/JAR candidate is a decoy, so it must not get a chance to win + // before the platform equivalent does. `ScopedOnly` is deliberately + // excluded: "no tail at all" is its own documented contract (its callers + // already have their own downstream fallback), so it must not gain one + // here. match io { ResolveIo::Full => vec![], ResolveIo::ScopedOnly => vec![], ResolveIo::NoRg => { - let found = indexer + let platform_equivalent = + resolve_kotlin_builtin_type_platform_equivalent(indexer, name); + if !platform_equivalent.is_empty() { + return platform_equivalent; + } + indexer .lookup_definitions(name) .into_iter() .next() .map(|loc| vec![loc]) - .unwrap_or_default(); - if !found.is_empty() { - return found; - } - resolve_kotlin_builtin_type_platform_equivalent(indexer, name) + .unwrap_or_default() } ResolveIo::IndexOnly => { - let found = ambiguity_safe_tail_with_denylist( - indexer, - from_uri, - indexer.lookup_definitions(name), - ); - if !found.is_empty() { - return found; + let platform_equivalent = + resolve_kotlin_builtin_type_platform_equivalent(indexer, name); + if !platform_equivalent.is_empty() { + return platform_equivalent; } - resolve_kotlin_builtin_type_platform_equivalent(indexer, name) + ambiguity_safe_tail_with_denylist(indexer, from_uri, indexer.lookup_definitions(name)) } ResolveIo::HierarchyAmbiguitySafe => { - let found = ambiguity_safe_tail_with_denylist( + let platform_equivalent = + resolve_kotlin_builtin_type_platform_equivalent(indexer, name); + if !platform_equivalent.is_empty() { + return platform_equivalent; + } + ambiguity_safe_tail_with_denylist( indexer, hierarchy_walk_origin_uri.unwrap_or(from_uri), indexer.lookup_definitions(name), - ); - if !found.is_empty() { - return found; - } - resolve_kotlin_builtin_type_platform_equivalent(indexer, name) + ) } } } diff --git a/src/resolver/tests.rs b/src/resolver/tests.rs index efbfaa7b..86b96630 100644 --- a/src/resolver/tests.rs +++ b/src/resolver/tests.rs @@ -9275,9 +9275,12 @@ fn the_initializer_search_survives_a_pathologically_deep_file() { // compiled `.class` file anywhere in kotlin-stdlib's JAR (verified via // `unzip -l kotlin-stdlib-*.jar | grep String.class` -> no output — see // docs/superpowers/specs/2026-08-27-kotlin-builtin-type-platform-mapping-design.md). -// `resolve_kotlin_builtin_type_platform_equivalent` is the last-resort -// fallback that indexes the real platform declaration from the Android SDK -// sources bundle on demand. +// `resolve_kotlin_builtin_type_platform_equivalent` indexes the real platform +// declaration from the Android SDK sources bundle on demand, and `resolve_chain` +// tries it in its tail *ahead of* the global-definitions lookup, not behind it: +// for every name in `KOTLIN_BUILTIN_TYPE_PLATFORM_EQUIVALENTS`, a same-named +// index/JAR candidate is always a decoy, so it must not get a chance to win +// first. /// Builds a fake Android SDK layout (`local.properties` + `sdk/sources/ /// android-/`) under `root`, matching the exact @@ -9590,6 +9593,179 @@ fn resolve_kotlin_builtin_type_platform_equivalent_surfaces_iterable_extensions_ ); } +/// Part 0.1's real corpus mechanism, in miniature: bare `List` has MANY +/// same-named index candidates, `ambiguity_safe_tail_with_denylist` survives +/// the denylist and module-scope stages, and then +/// `default_kotlin_import_tie_break` narrows to the single +/// `kotlin.collections`-packaged one -- a kotlin-stdlib decoy with no +/// compiled body and no supertypes. The real `java.util.List` must win +/// instead, or `type_path_anchors` anchors every `List` receiver on a +/// declaration with no walkable supertype chain. +#[test] +fn a_builtin_type_name_prefers_the_platform_declaration_over_a_default_imported_decoy() { + use crate::types::FileData; + use std::sync::Arc; + + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + write_fake_android_sdk_source( + root, + "java/util/List.java", + "package java.util;\npublic interface List extends Collection {\n}\n", + ); + + let indexer = Indexer::new(); + indexer.workspace_root.set(root.to_path_buf()); + + let stdlib_decoy_uri = "jar:file:///kotlin-stdlib.jar!/kotlin/collections/List.class"; + let unrelated_decoy_uri = "jar:file:///unrelated.jar!/com/example/List.class"; + for decoy_uri in [stdlib_decoy_uri, unrelated_decoy_uri] { + indexer + .jar_definitions + .entry("List".to_owned()) + .or_default() + .push(tower_lsp::lsp_types::Location { + uri: Url::parse(decoy_uri).unwrap(), + range: Default::default(), + }); + } + indexer.jar_files.insert( + stdlib_decoy_uri.to_owned(), + Arc::new(FileData { + package: Some("kotlin.collections".to_owned()), + ..Default::default() + }), + ); + indexer.jar_files.insert( + unrelated_decoy_uri.to_owned(), + Arc::new(FileData { + package: Some("com.example".to_owned()), + ..Default::default() + }), + ); + + let caller_source = "package app\nfun use(items: List) { }\n"; + let caller_path = root.join("Caller.kt"); + std::fs::write(&caller_path, caller_source).unwrap(); + let caller_uri = Url::from_file_path(&caller_path).unwrap(); + indexer.index_content(&caller_uri, caller_source); + + let locations = resolve_symbol_index_only(&indexer, "List", None, &caller_uri); + assert_eq!( + locations.len(), + 1, + "expected exactly the platform declaration, got {locations:?}" + ); + assert!( + locations[0].uri.path().ends_with("java/util/List.java"), + "expected the real java.util.List, got {:?}", + locations[0].uri + ); +} + +/// The second, different failure shape from Part 0.2: `MutableSet` and its +/// four `Mutable*` siblings have exactly ONE same-named index candidate (a +/// kotlin-gradle-plugin decoy), so they never reach a tie-break at all -- +/// the unique-match arm returns the decoy outright. The platform equivalent +/// must still win, and note the target's simple name differs from the +/// Kotlin one (`MutableSet` -> `java.util.Set`). +#[test] +fn a_mutable_builtin_type_name_prefers_the_platform_declaration_over_a_unique_decoy() { + use crate::types::FileData; + use std::sync::Arc; + + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + write_fake_android_sdk_source( + root, + "java/util/Set.java", + "package java.util;\npublic interface Set extends Collection {\n}\n", + ); + + let indexer = Indexer::new(); + indexer.workspace_root.set(root.to_path_buf()); + + let decoy_uri = "jar:file:///kotlin-gradle-plugin.jar!/kotlin/collections/MutableSet.class"; + indexer.jar_definitions.insert( + "MutableSet".to_owned(), + vec![tower_lsp::lsp_types::Location { + uri: Url::parse(decoy_uri).unwrap(), + range: Default::default(), + }], + ); + indexer.jar_files.insert( + decoy_uri.to_owned(), + Arc::new(FileData { + package: Some("kotlin.collections".to_owned()), + ..Default::default() + }), + ); + + let caller_source = "package app\nfun use(items: MutableSet) { }\n"; + let caller_path = root.join("Caller.kt"); + std::fs::write(&caller_path, caller_source).unwrap(); + let caller_uri = Url::from_file_path(&caller_path).unwrap(); + indexer.index_content(&caller_uri, caller_source); + + let locations = resolve_symbol_index_only(&indexer, "MutableSet", None, &caller_uri); + assert_eq!( + locations.len(), + 1, + "expected exactly the platform declaration, got {locations:?}" + ); + assert!( + locations[0].uri.path().ends_with("java/util/Set.java"), + "expected the real java.util.Set, got {:?}", + locations[0].uri + ); +} + +/// Decoy guard, expected GREEN both before and after: the reorder lives in +/// `resolve_chain`'s TAIL, below steps 1-4.5, so a workspace file that really +/// declares its own `Double` must still win for a caller in its own package. +/// (Not purely hypothetical: the Moneta corpus declares a `Double` at +/// feature/credit_card/clip/.../InitUseCase.kt:73, though as a NESTED data +/// class rather than a top-level one. This fixture uses a top-level +/// declaration, which is the shape the tail ordering actually has to respect.) +#[test] +fn a_workspace_declaration_still_outranks_the_builtin_platform_equivalent() { + let directory = tempfile::tempdir().unwrap(); + let root = directory.path(); + write_fake_android_sdk_source( + root, + "java/lang/Double.java", + "package java.lang;\npublic final class Double extends Number {\n}\n", + ); + + let indexer = Indexer::new(); + indexer.workspace_root.set(root.to_path_buf()); + + let declaration_source = "package app\nclass Double(val value: String)\n"; + let declaration_path = root.join("Double.kt"); + std::fs::write(&declaration_path, declaration_source).unwrap(); + let declaration_uri = Url::from_file_path(&declaration_path).unwrap(); + indexer.index_content(&declaration_uri, declaration_source); + + let caller_source = "package app\nfun use(value: Double) { }\n"; + let caller_path = root.join("Caller.kt"); + std::fs::write(&caller_path, caller_source).unwrap(); + let caller_uri = Url::from_file_path(&caller_path).unwrap(); + indexer.index_content(&caller_uri, caller_source); + + let locations = resolve_symbol_index_only(&indexer, "Double", None, &caller_uri); + assert_eq!( + locations, + vec![tower_lsp::lsp_types::Location { + uri: declaration_uri, + range: locations + .first() + .map(|location| location.range) + .unwrap_or_default(), + }], + "the same-package workspace declaration must still win, got {locations:?}" + ); +} + // ─── extension-registry overload collapse (2026-09-14 plan, Task 1) ────────── // // `resolve_extension_in_scope`, `jar_extension_for_type_root`, and From 5e4deddea16ef90d3d8bd6be87f560410cb4efd4 Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Wed, 16 Sep 2026 10:42:17 +0200 Subject: [PATCH 3/7] fix(resolve): count Kotlin's default-import packages as extension scope extension_is_in_scope only knows same-package and explicit-import visibility, so a correctly-registered extension declared in one of Kotlin's own default-import packages (kotlin.text, kotlin.collections, ...) was rejected as out-of-scope at every real call site, since no Kotlin file ever writes e.g. `import kotlin.text.isNotEmpty`. Adds extension_entry_is_in_scope, a wrapper used only at the two extension-registry call sites plus the extension return-type scoped lookup, rather than editing extension_is_in_scope directly -- four of its six callers aren't about extension scoping at all, and widening the shared predicate would change bare-name JAR candidate preference corpus-wide. Gated on the calling file's language: resolve_qualified and the resolution-accuracy benchmark both run over indexed .java files too, and a Java file never implicitly imports kotlin.*. Real-corpus measurement (Moneta android, before/after release binaries): member Gap 7578->7188 (-390), FilteredCandidate 6553->6868 (+315, expected -- more in-scope candidates now reach the shape filter, pointing at follow-up work there), member recall 91.4%->91.5%. isNotEmpty and the 0-arg firstOrNull shape both left the Gap top-20 as predicted. Co-Authored-By: Claude Sonnet 5 --- src/resolver/extension.rs | 24 ++--- src/resolver/imports.rs | 2 +- src/resolver/infer.rs | 62 +++++++++-- src/resolver/tests.rs | 221 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 284 insertions(+), 25 deletions(-) diff --git a/src/resolver/extension.rs b/src/resolver/extension.rs index bde13190..6f9cc431 100644 --- a/src/resolver/extension.rs +++ b/src/resolver/extension.rs @@ -88,15 +88,11 @@ pub(super) fn resolve_extension_in_scope( if entry.name != name { continue; } - let in_scope = crate::resolver::infer::extension_is_in_scope( - entry.package.as_ref(), - &entry.name, - entry.container.as_ref(), - entry.visibility, - entry.file_uri == from_uri.as_str(), + if !crate::resolver::infer::extension_entry_is_in_scope( + entry, + from_uri, caller_file_data_ref, - ); - if !in_scope { + ) { continue; } let Ok(uri) = Url::parse(&entry.file_uri) else { @@ -175,15 +171,11 @@ fn implicit_receiver_extension_match( if entry.name != name { continue; } - let in_scope = crate::resolver::infer::extension_is_in_scope( - entry.package.as_ref(), - &entry.name, - entry.container.as_ref(), - entry.visibility, - entry.file_uri == from_uri.as_str(), + if !crate::resolver::infer::extension_entry_is_in_scope( + entry, + from_uri, caller_file_data_ref, - ); - if !in_scope { + ) { continue; } let Ok(uri) = Url::parse(&entry.file_uri) else { diff --git a/src/resolver/imports.rs b/src/resolver/imports.rs index 6a09be1d..ea540426 100644 --- a/src/resolver/imports.rs +++ b/src/resolver/imports.rs @@ -43,7 +43,7 @@ pub(super) const KOTLIN_DEFAULT_IMPORT_PACKAGES: &[&str] = &[ /// Kotlin's implicit default-import packages (JVM target): names declared directly /// in these are in scope in every file without an `import`. Narrower than /// [`is_stdlib`] — `android`/`androidx`/most `java.*` are *not* auto-imported. -fn is_default_import_package(pkg: &str) -> bool { +pub(super) fn is_default_import_package(pkg: &str) -> bool { KOTLIN_DEFAULT_IMPORT_PACKAGES.contains(&pkg) } diff --git a/src/resolver/infer.rs b/src/resolver/infer.rs index f4c48bfa..86b30bed 100644 --- a/src/resolver/infer.rs +++ b/src/resolver/infer.rs @@ -1788,6 +1788,11 @@ pub(crate) fn find_method_return_type( /// `Some`, `entry_visibility` and `is_same_file` gate `private`/`protected` /// member extensions, which — unlike the package/import checks below — are /// otherwise skipped entirely for a member extension (see the branch itself). +/// +/// An extension-registry consumer should generally call +/// [`extension_entry_is_in_scope`] instead — it wraps this function with +/// Kotlin's default-import package rule, which this function alone does not +/// know about. pub(crate) fn extension_is_in_scope( entry_package: Option<&String>, entry_name: &str, @@ -1849,6 +1854,54 @@ pub(crate) fn extension_is_in_scope( }) } +/// Whether extension registry `entry` is callable from the file at `from_uri`. +/// +/// [`extension_is_in_scope`]'s package/import rules, plus the one rule they +/// cannot express: a Kotlin file implicitly imports every name declared +/// directly in Kotlin's own default-import packages (see +/// [`crate::resolver::imports::KOTLIN_DEFAULT_IMPORT_PACKAGES`]), so +/// `kotlin.text`'s `isNotEmpty` or `kotlin.collections`'s `firstOrNull` is in +/// scope at every call site with no `import` line anywhere — and no real file +/// ever writes one. +/// +/// Gated on the CALLING file's language, for the same reason +/// [`crate::resolver::tie_break`]'s `default_kotlin_import_tie_break` gates +/// its own use of the same set: Kotlin's default imports are a fact about +/// Kotlin source files, and `resolve_qualified` runs over indexed `.java` and +/// `.swift` files too. +/// +/// Measured on the Moneta corpus: the registry holds 24897 entries across 2360 +/// receiver buckets; 3641 of those entries live in a default-import package, +/// 3638 of them top-level, and every one was rejected here for every caller. +/// +/// Deliberately NOT folded into [`extension_is_in_scope`] itself: four of that +/// function's six callers are not about extensions at all +/// (`candidate_declaration_is_reachable`, `Indexer::jar_candidate_is_reachable` +/// and `nullable_call_diagnostics`' stricter own rule), and widening the shared +/// predicate would change bare-name JAR candidate preference corpus-wide. +pub(crate) fn extension_entry_is_in_scope( + entry: &crate::types::ExtensionEntry, + from_uri: &Url, + caller_file_data: Option<&FileData>, +) -> bool { + if extension_is_in_scope( + entry.package.as_ref(), + &entry.name, + entry.container.as_ref(), + entry.visibility, + entry.file_uri == from_uri.as_str(), + caller_file_data, + ) { + return true; + } + let caller_is_kotlin = crate::Language::from_path(from_uri.as_str()) == crate::Language::Kotlin; + let entry_is_default_imported = entry + .package + .as_ref() + .is_some_and(|package| crate::resolver::imports::is_default_import_package(package)); + caller_is_kotlin && entry_is_default_imported +} + /// Whether `SymbolEntry` `symbol` is the actual declaration a matched extension /// candidate (`name` on `receiver_base`, with `entry_container` from its /// `ExtensionEntry`) refers to. Its own `container` must equal @@ -1933,14 +1986,7 @@ fn find_extension_fn_return_type_scoped( if !matches!(entry.kind, SymbolKind::FUNCTION | SymbolKind::METHOD) { continue; } - if !extension_is_in_scope( - entry.package.as_ref(), - &entry.name, - entry.container.as_ref(), - entry.visibility, - entry.file_uri == from_uri.as_str(), - caller_file_data_ref, - ) { + if !extension_entry_is_in_scope(entry, from_uri, caller_file_data_ref) { continue; } // Try detail first; fall back to source lines when detail is truncated. diff --git a/src/resolver/tests.rs b/src/resolver/tests.rs index 86b96630..d9785ef5 100644 --- a/src/resolver/tests.rs +++ b/src/resolver/tests.rs @@ -11250,3 +11250,224 @@ fn kotlin_default_import_tie_break_does_not_apply_from_a_java_origin_file() { resolve this ambiguity, got {locs:?}" ); } + +/// Builds an indexer whose registry holds one `kotlin.text`-packaged +/// extension on `CharSequence`, plus a caller file in an unrelated package +/// with no imports at all -- the shape of every real Kotlin call site, since +/// nobody writes `import kotlin.text.isNotEmpty`. +fn indexer_with_a_default_imported_extension(caller_path: &str) -> (Indexer, Url) { + use crate::types::{ExtensionEntry, FileData, SourceSet, SymbolEntry, Visibility}; + use std::sync::Arc; + use tower_lsp::lsp_types::{Position, Range, SymbolKind}; + + let indexer = Indexer::new(); + let jar_uri = "jar:file:///kotlin-stdlib.jar!/kotlin/text/StringsKt.class".to_owned(); + let declaration_range = Range { + start: Position { + line: 7, + character: 0, + }, + end: Position { + line: 7, + character: 10, + }, + }; + let detail = "fun CharSequence.isNotEmpty(): Boolean".to_owned(); + + indexer.jar_files.insert( + jar_uri.clone(), + Arc::new(FileData { + symbols: vec![SymbolEntry { + name: "isNotEmpty".to_owned(), + kind: SymbolKind::FUNCTION, + visibility: Visibility::Public, + range: declaration_range, + selection_range: declaration_range, + detail: detail.clone(), + container: None, + params: String::new(), + param_counts: (0, 0), + cold: crate::types::pack_cold_fields( + vec![], + "CharSequence".to_owned(), + String::new(), + String::new(), + ), + trailing_lambda: false, + deprecated: false, + }], + source_set: SourceSet::Library, + package: Some("kotlin.text".to_owned()), + lines: Arc::new(vec![]), + ..Default::default() + }), + ); + indexer + .extension_by_receiver + .entry("CharSequence".to_owned()) + .or_default() + .push(ExtensionEntry { + file_uri: jar_uri, + name: "isNotEmpty".to_owned(), + kind: SymbolKind::FUNCTION, + detail, + visibility: Visibility::Public, + package: Some("kotlin.text".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }); + + let caller_uri = uri(caller_path); + indexer.index_content(&caller_uri, "package app\nclass Caller\n"); + (indexer, caller_uri) +} + +/// Kotlin's default-import packages (`kotlin`, `kotlin.text`, +/// `kotlin.collections`, `java.lang`, ...) are in scope in every Kotlin file +/// with no `import` line -- and no real file writes one. Before this fix +/// `extension_is_in_scope` knew only about same-package and explicit +/// imports, so it rejected all 3638 top-level default-import-packaged +/// registry entries for every caller on the Moneta corpus. +#[test] +fn a_default_import_packaged_extension_is_in_scope_without_an_explicit_import() { + let (indexer, caller_uri) = indexer_with_a_default_imported_extension("/app/Caller.kt"); + + let locations = super::extension::resolve_extension_in_scope( + &indexer, + "CharSequence", + "isNotEmpty", + &caller_uri, + ); + assert_eq!( + locations.len(), + 1, + "kotlin.text.isNotEmpty must be in scope for an unimporting Kotlin \ + caller, got {locations:?}" + ); + assert_eq!( + locations[0].range.start.line, 7, + "expected the real declaration range, got {:?}", + locations[0].range + ); +} + +/// Decision D2's guard: `resolve_qualified` runs over indexed `.java` files +/// too (the resolution-accuracy benchmark scans both `.kt` and `.java`), and +/// a Java file never implicitly imports `kotlin.*`. Same reasoning, and the +/// same Copilot review finding, that already gates +/// `default_kotlin_import_tie_break` on the origin file's language. +#[test] +fn a_default_import_packaged_extension_stays_out_of_scope_for_a_java_caller() { + let (indexer, caller_uri) = indexer_with_a_default_imported_extension("/app/Caller.java"); + + let locations = super::extension::resolve_extension_in_scope( + &indexer, + "CharSequence", + "isNotEmpty", + &caller_uri, + ); + assert!( + locations.is_empty(), + "a Java origin must not pick up Kotlin's default imports, got {locations:?}" + ); +} + +/// The new rule must NOT leak to a package that merely starts with a +/// default-import prefix: `kotlin.text.regex` is not `kotlin.text`, and +/// `is_default_import_package` is an exact-membership check, not a prefix +/// match. Guard against a future "fix" that swaps it for `starts_with`. +/// +/// Targets `extension_entry_is_in_scope`, NOT `extension_is_in_scope` — the +/// latter is the function this task deliberately leaves untouched (Part 0.3), +/// so a guard aimed at it would be green before and after by construction and +/// pin nothing. +#[test] +fn a_package_below_a_default_import_package_is_still_out_of_scope() { + use crate::resolver::infer::extension_entry_is_in_scope; + use crate::types::{ExtensionEntry, Visibility}; + use tower_lsp::lsp_types::SymbolKind; + + let indexer = Indexer::new(); + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nclass Caller\n"); + let caller_file_data = indexer.files.get(caller_uri.as_str()); + + let entry = ExtensionEntry { + file_uri: "jar:file:///kotlin-stdlib.jar!/kotlin/text/regex/RegexKt.class".to_owned(), + name: "someExtension".to_owned(), + kind: SymbolKind::FUNCTION, + detail: "fun CharSequence.someExtension(): Boolean".to_owned(), + visibility: Visibility::Public, + package: Some("kotlin.text.regex".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }; + assert!( + !extension_entry_is_in_scope( + &entry, + &caller_uri, + caller_file_data.as_deref().map(|value| value.as_ref()), + ), + "kotlin.text.regex is not one of Kotlin's default-import packages" + ); +} + +/// The third call site, on the type-inference side: an unimporting Kotlin +/// caller must now be able to infer a `kotlin.collections` extension's +/// return type, which `find_extension_fn_return_type_scoped`'s own +/// in-scope check rejected before this fix. +#[test] +fn a_default_import_packaged_extension_return_type_is_inferable_without_an_import() { + use crate::types::{ExtensionEntry, FileData, SourceSet, Visibility}; + use std::sync::Arc; + use tower_lsp::lsp_types::SymbolKind; + + let indexer = Indexer::new(); + let jar_uri = + "jar:file:///kotlin-stdlib.jar!/kotlin/collections/CollectionsKt.class".to_owned(); + indexer.jar_files.insert( + jar_uri.clone(), + Arc::new(FileData { + source_set: SourceSet::Library, + package: Some("kotlin.collections".to_owned()), + lines: Arc::new(vec![]), + ..Default::default() + }), + ); + indexer + .extension_by_receiver + .entry("List".to_owned()) + .or_default() + .push(ExtensionEntry { + file_uri: jar_uri, + name: "firstOrNull".to_owned(), + kind: SymbolKind::FUNCTION, + detail: "fun List.firstOrNull(): T?".to_owned(), + visibility: Visibility::Public, + package: Some("kotlin.collections".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }); + + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nclass Caller\n"); + + assert_eq!( + crate::resolver::infer::find_extension_fn_return_type( + &indexer, + "List", + "firstOrNull", + Some(&caller_uri), + ), + // Not `Some("T?")`: `extract_return_type_from_detail` strips a + // trailing `?` intentionally, same as `return_type_nullable_stripped` + // pins for the non-extension path -- unrelated to this task's scope + // fix, which is what actually turned this from `None` into `Some`. + Some("T".to_owned()), + "an unimporting Kotlin caller must be able to infer a \ + kotlin.collections extension's return type" + ); +} From f3b101027a406b590ea1095e2e81acf0fb3e0b1f Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Wed, 16 Sep 2026 14:03:48 +0200 Subject: [PATCH 4/7] fix(resolve): select the implicit-receiver extension overload the call shape wants implicit_receiver_extension_match had the same first-match bug PR #321 fixed elsewhere in extension.rs: it looked up the declaring symbol for each registry entry via .find(extension_declaration_matches), a predicate that only checks (name, receiver, container) -- identical across every overload of the same name -- so every entry in the loop resolved to the same first-declared overload regardless of which entry was actually being iterated, making every overload but the first unreachable through this entry point. Split select_extension_symbol_range's existing disambiguation (filter by extension_declaration_matches, then pick the exact detail match, falling back to the first) out into select_extension_symbol, returning the whole SymbolEntry instead of just its Range. select_extension_symbol_range is now a two-line wrapper over it, and implicit_receiver_extension_match uses the new helper so its per-entry arity check runs against the right overload. Also documents Decision D5 in rename.rs: PR #321 means a same-arity extension collision now correctly refuses to rename rather than silently picking one candidate -- reviewed and kept as-is, not a bug. Co-Authored-By: Claude Sonnet 5 --- src/features/rename.rs | 9 ++++++ src/resolver/extension.rs | 59 +++++++++++++++++++++++---------------- src/resolver/tests.rs | 59 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 24 deletions(-) diff --git a/src/features/rename.rs b/src/features/rename.rs index 0156590e..8aac28f5 100644 --- a/src/features/rename.rs +++ b/src/features/rename.rs @@ -103,6 +103,15 @@ pub(crate) async fn rename_impl( "identity is ambiguous — could not resolve a single definition", )); }; + // A same-arity extension collision (two unrelated `Modifier.weight` + // declarations, say) reaches this refusal BY DESIGN since PR #321: + // `resolve_identity` correctly returns the whole overload set rather than + // one arbitrary member of it, and silently renaming an arbitrarily chosen + // one of two genuinely distinct declarations is the unsound behaviour that + // PR replaced. Listing both candidates in the message would not help -- + // an LSP error is surfaced as a toast with no navigation affordance -- so a + // real disambiguation UX needs a client-side picker this server has no + // protocol hook for. Reviewed and kept as-is, 2026-09-16. if definitions.len() != 1 { return Err(refusal( "identity is ambiguous — matches more than one definition", diff --git a/src/resolver/extension.rs b/src/resolver/extension.rs index 6f9cc431..3eb27497 100644 --- a/src/resolver/extension.rs +++ b/src/resolver/extension.rs @@ -4,18 +4,19 @@ use tower_lsp::lsp_types::{Location, Range, Url}; use crate::indexer::{CallShape, Indexer}; -use crate::types::FileData; +use crate::types::{FileData, SymbolEntry}; use super::resolve::resolve_symbol; // ─── step implementations ──────────────────────────────────────────────────── -/// Select the `Range` of the declaring symbol for one extension registry -/// `entry` inside its declaring file's already-parsed symbol table. +/// Select the declaring symbol for one extension registry `entry` inside its +/// declaring file's already-parsed symbol table. /// -/// Shared by [`resolve_extension_in_scope`] and -/// [`super::qualified::jar_extension_for_type_root`]: both now return every -/// same-named registry entry rather than the first, so both need this exact +/// Shared by [`resolve_extension_in_scope`], +/// [`super::qualified::jar_extension_for_type_root`], and +/// `implicit_receiver_extension_match`: all now consider every same-named +/// registry entry rather than just the first, so all three need this exact /// two-step selection — a file may declare several overloads of the same /// extension (same name, same receiver, same container), and /// `extension_declaration_matches` alone can't tell them apart. Prefer the @@ -23,15 +24,15 @@ use super::resolve::resolve_symbol; /// entry's own `detail`; when nothing matches exactly (e.g. the registry /// entry's detail was computed slightly differently than the symbol table's, /// or the file has since drifted), fall back to the first declaration with a -/// matching name/receiver/container shape rather than returning no range at +/// matching name/receiver/container shape rather than returning nothing at /// all. -pub(super) fn select_extension_symbol_range( - file_data: &FileData, +pub(super) fn select_extension_symbol<'file_data>( + file_data: &'file_data FileData, name: &str, receiver_base: &str, container: Option<&String>, detail: &str, -) -> Range { +) -> Option<&'file_data SymbolEntry> { let declaring_symbols: Vec<_> = file_data .symbols .iter() @@ -47,8 +48,21 @@ pub(super) fn select_extension_symbol_range( let exact_signature_match = declaring_symbols .iter() .find(|symbol| symbol.detail == detail); - let selected = exact_signature_match.or_else(|| declaring_symbols.first()); - selected + exact_signature_match + .or_else(|| declaring_symbols.first()) + .copied() +} + +/// The `Range` half of [`select_extension_symbol`], for callers that only +/// need the location and not the rest of the symbol (params/arity). +pub(super) fn select_extension_symbol_range( + file_data: &FileData, + name: &str, + receiver_base: &str, + container: Option<&String>, + detail: &str, +) -> Range { + select_extension_symbol(file_data, name, receiver_base, container, detail) .map(|symbol| symbol.selection_range) .unwrap_or_default() } @@ -185,18 +199,15 @@ fn implicit_receiver_extension_match( .files .get(&entry.file_uri) .or_else(|| indexer.jar_files.get(&entry.file_uri)) - .and_then(|fd| { - fd.symbols - .iter() - .find(|s| { - crate::resolver::infer::extension_declaration_matches( - s, - name, - receiver_base, - entry.container.as_ref(), - ) - }) - .cloned() + .and_then(|file_data| { + select_extension_symbol( + &file_data, + name, + receiver_base, + entry.container.as_ref(), + &entry.detail, + ) + .cloned() }); let Some(symbol) = symbol else { continue }; let is_vararg = symbol.params.contains("vararg ") || symbol.params.contains("vararg\t"); diff --git a/src/resolver/tests.rs b/src/resolver/tests.rs index d9785ef5..72d2e481 100644 --- a/src/resolver/tests.rs +++ b/src/resolver/tests.rs @@ -11471,3 +11471,62 @@ fn a_default_import_packaged_extension_return_type_is_inferable_without_an_impor kotlin.collections extension's return type" ); } + +/// PR #321's overload-collapse family, on the implicit-receiver entry point +/// (`resolve_implicit_receiver_callee` -> `implicit_receiver_extension_match`). +/// The registry correctly holds one entry per overload, but for EVERY entry +/// the loop re-ran the same `extension_declaration_matches` `.find(...)` over +/// the declaring file -- and that predicate compares only +/// (name, receiver, container), identical across overloads -- so every +/// iteration shape-checked the FIRST-declared overload and a differently +/// shaped call was rejected on all of them. The 1-arg overload was +/// unreachable through this entry point entirely. +#[test] +fn an_implicit_receiver_call_reaches_the_arity_matching_extension_overload() { + use crate::indexer::CallShape; + + let indexer = Indexer::new(); + let extensions_uri = uri("/app/Extensions.kt"); + indexer.index_content( + &extensions_uri, + concat!( + "package app\n", + "fun Foo.describe(): String = \"\"\n", + "fun Foo.describe(prefix: String): String = prefix\n", + ), + ); + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content( + &caller_uri, + concat!( + "package app\n", + "class Foo\n", + "fun Foo.use() {\n", + " describe(\"x\")\n", + "}\n", + ), + ); + + let shape = CallShape { + arg_count: 1, + trailing_lambda: false, + }; + let locations = crate::resolver::resolve_implicit_receiver_callee( + &indexer, + "Foo", + "describe", + &caller_uri, + shape, + ); + assert_eq!( + locations.len(), + 1, + "expected the 1-arg overload, got {locations:?}" + ); + assert_eq!( + locations[0].range.start.line, 2, + "expected the SECOND declaration (the 1-arg overload, line index 2), \ + got {:?}", + locations[0].range + ); +} From b0ad1756965ec78c7956a48a99742f92d4453bca Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Wed, 16 Sep 2026 14:08:35 +0200 Subject: [PATCH 5/7] fix(infer): select the named overload in scoped extension return-type lookup find_extension_fn_return_type_scoped's truncated-detail fallback picked the FIRST declaration matching (name, receiver, container) in the declaring file -- a predicate identical across every overload of the same extension -- so a second overload's return type was unreachable no matter which registry entry was being processed. Route it through select_extension_symbol (PR #321), which disambiguates by matching the declaration's full signature against the registry entry's own detail. Separately, both `?` operators in this loop (file lookup, declaration lookup) returned None from the whole function instead of continuing to the next entry, so a single registry entry whose declaring file wasn't loaded silently hid every entry after it in iteration order. Both are now `continue`. Not fixed: arity-aware overload selection needs a CallShape threaded through Resolver::method_return_type -- 482 registry groups on the Moneta corpus have differing-return overloads, and that's its own plan. find_extension_fn_return_type_global has no ExtensionEntry/detail to disambiguate with and is not on a production path (its only caller always passes Some(uri)), so it's left as-is with a comment recording why. Co-Authored-By: Claude Sonnet 5 --- src/resolver/infer.rs | 49 ++++++++++----- src/resolver/infer_tests.rs | 116 ++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 14 deletions(-) diff --git a/src/resolver/infer.rs b/src/resolver/infer.rs index 86b30bed..4514dc09 100644 --- a/src/resolver/infer.rs +++ b/src/resolver/infer.rs @@ -1998,22 +1998,34 @@ fn find_extension_fn_return_type_scoped( // `extension_by_receiver` already had it, which — per the comment // above `entries` — only happens once Tier-2 materialization has // already populated `jar_files` for this same jar. - let file_data = indexer + // + // Neither lookup below may abort the whole search with `?`: an entry + // whose declaring file isn't loaded, or whose declaration can't be + // matched, must be skipped so later entries in `entries` still get a + // chance — one unusable entry must not hide every usable one behind + // it in iteration order. + let Some(file_data) = indexer .files .get(&entry.file_uri) - .or_else(|| indexer.jar_files.get(&entry.file_uri))?; - let start_line = file_data - .symbols - .iter() - .find(|s| { - extension_declaration_matches( - s, - method_name, - receiver_base, - entry.container.as_ref(), - ) - })? - .selection_start() as usize; + .or_else(|| indexer.jar_files.get(&entry.file_uri)) + else { + continue; + }; + // PR #321's `select_extension_symbol` (see its doc comment in + // `resolver::extension`) prefers the declaration whose full `detail` + // exactly matches this registry entry's own, so a second overload's + // return type is reachable through this fallback too — not just the + // first declaration matching (name, receiver, container). + let Some(declaring_symbol) = crate::resolver::extension::select_extension_symbol( + &file_data, + method_name, + receiver_base, + entry.container.as_ref(), + &entry.detail, + ) else { + continue; + }; + let start_line = declaring_symbol.selection_start() as usize; let full_sig = file_data.lines.collect_signature(start_line); if let Some(ret) = extract_return_type_from_detail(&full_sig) { return Some(ret); @@ -2022,6 +2034,15 @@ fn find_extension_fn_return_type_scoped( None } +// Part 0.4: this function has no `ExtensionEntry` and no `detail` to match +// against (it walks `file_data.symbols` directly), so the two-step +// `select_extension_symbol` disambiguation above does not apply here — and it +// doesn't need to, since `Indexer::find_method_return_type_for_type` always +// passes `Some(uri)`, so this fallback is not reachable on a production path. +// Decision D4: giving either function arity-aware overload selection needs a +// `CallShape` threaded through `Resolver::method_return_type`; 482 registry +// groups on the Moneta corpus have differing-return overloads, so that is +// real but is its own plan, not this one. fn find_extension_fn_return_type_global( indexer: &Indexer, receiver_base: &str, diff --git a/src/resolver/infer_tests.rs b/src/resolver/infer_tests.rs index bb6849ad..c0422c45 100644 --- a/src/resolver/infer_tests.rs +++ b/src/resolver/infer_tests.rs @@ -730,6 +730,122 @@ fn extension_fn_return_type_scoped_resolves_member_extension_via_truncated_detai ); } +/// The truncated-`detail` fallback picked the FIRST declaration in the +/// declaring file matching (name, receiver, container) -- a predicate +/// identical across every overload -- so a second overload's return type was +/// unreachable through it, no matter which registry entry the loop was on. +/// PR #321's `select_extension_symbol` already solves exactly this +/// disambiguation by preferring the declaration whose full signature text +/// equals the registry entry's own. +/// +/// Both overloads are made long enough that `detail` truncates past the +/// return type (`MAX_DETAIL_CHARS`, `src/parser.rs:1219`), forcing the +/// `collect_signature` fallback -- the only path carrying the bug -- while +/// still differing inside the first 120 characters (`alpha` vs `beta`) so the +/// two truncated details remain distinguishable. +#[test] +fn extension_fn_return_type_scoped_selects_the_overload_the_entry_names() { + use super::find_extension_fn_return_type; + use crate::indexer::Indexer; + use tower_lsp::lsp_types::Url; + + fn uri(path: &str) -> Url { + Url::parse(&format!("file://{path}")).unwrap() + } + + let indexer = Indexer::new(); + let declaring_uri = uri("/app/Extensions.kt"); + let long_parameter = "w".repeat(150); + indexer.index_content( + &declaring_uri, + &format!( + "package app\n\ + class Foo\n\ + fun Foo.describe(alpha: Int, {long_parameter}: Int): String = \"\"\n\ + fun Foo.describe(beta: Int, {long_parameter}: Int): Boolean = true\n" + ), + ); + + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nfun use() {}\n"); + + // `find_extension_fn_return_type` returns on the first entry that yields + // a return type, so the first entry's (correct) answer would mask the + // bug. Drop it and the SECOND entry has to stand on its own -- which is + // exactly what it cannot do today. + indexer + .extension_by_receiver + .entry("Foo".to_owned()) + .or_default() + .remove(0); + + assert_eq!( + find_extension_fn_return_type(&indexer, "Foo", "describe", Some(&caller_uri)), + Some("Boolean".to_owned()), + "the remaining entry names the SECOND overload, so its return type \ + must be Boolean, not the first declaration's String" + ); +} + +/// An entry whose declaring file is not loaded must skip to the next entry, +/// not abort the whole search: both `indexer.files.get(...)?` +/// (`infer.rs:1958`) and `.find(...)?` (`:1969`) returned `None` from the +/// FUNCTION rather than continuing the loop, so one unusable entry hid every +/// usable one behind it. +/// +/// This entry is hand-built on purpose -- it must point at a file that is +/// deliberately never indexed, which `index_content` cannot produce -- and it +/// is `insert`ed ahead of the auto-registered real one so it is reached first. +/// Its `detail` carries no return type, so the loop falls past the +/// `extract_return_type_from_detail` shortcut to the file lookup that aborts. +#[test] +fn extension_fn_return_type_scoped_skips_an_entry_whose_file_is_missing() { + use super::find_extension_fn_return_type; + use crate::indexer::Indexer; + use crate::types::{ExtensionEntry, Visibility}; + use tower_lsp::lsp_types::{SymbolKind, Url}; + + fn uri(path: &str) -> Url { + Url::parse(&format!("file://{path}")).unwrap() + } + + let indexer = Indexer::new(); + let declaring_uri = uri("/app/Extensions.kt"); + indexer.index_content( + &declaring_uri, + "package app\nclass Foo\nfun Foo.describe(): String = \"\"\n", + ); + + indexer + .extension_by_receiver + .entry("Foo".to_owned()) + .or_default() + .insert( + 0, + ExtensionEntry { + file_uri: "file:///app/NeverIndexed.kt".to_owned(), + name: "describe".to_owned(), + kind: SymbolKind::FUNCTION, + detail: "fun Foo.describe(".to_owned(), + visibility: Visibility::Public, + package: Some("app".to_owned()), + trailing_lambda: false, + deprecated: false, + container: None, + }, + ); + + let caller_uri = uri("/app/Caller.kt"); + indexer.index_content(&caller_uri, "package app\nfun use() {}\n"); + + assert_eq!( + find_extension_fn_return_type(&indexer, "Foo", "describe", Some(&caller_uri)), + Some("String".to_owned()), + "an entry pointing at an unindexed file must be skipped, not abort \ + the search over the remaining entries" + ); +} + /// `Resolver::function_return_type` is the import-aware catalog entry: it binds /// through the scope chain first, then falls back to a workspace-wide by-name /// lookup, returning a self-documenting [`ReturnType`]. This asserts the From d53160a6827bd12726236cd5b08aa3089770d4e6 Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Wed, 16 Sep 2026 14:33:43 +0200 Subject: [PATCH 6/7] docs: update rename.rs's D5 comment with the cluster's measured frequency impact The final whole-branch review of this cluster ran the cumulative real-corpus measurement no single task had run (878cc4ea..b0ad1756 in one pass) and found FilteredCandidate rose 449, not the -167 the individual tasks' pairwise deltas would suggest composing to. Traced mechanism: Task 2's extension-scope widening feeds more real candidates into resolve_extension_in_scope, which is exactly the function rename's ambiguity check depends on. The refusal's own reasoning (PR #321 correctness) is unchanged; only its frequency is. Co-Authored-By: Claude Sonnet 5 --- src/features/rename.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/features/rename.rs b/src/features/rename.rs index 8aac28f5..3de8daa6 100644 --- a/src/features/rename.rs +++ b/src/features/rename.rs @@ -112,6 +112,15 @@ pub(crate) async fn rename_impl( // an LSP error is surfaced as a toast with no navigation affordance -- so a // real disambiguation UX needs a client-side picker this server has no // protocol hook for. Reviewed and kept as-is, 2026-09-16. + // + // The 2026-09-16 extension-registry-follow-ups cluster raised how OFTEN + // this fires, not whether it should: its Task 2 widened extension-scope + // recognition (default-import packages now count), so more real + // candidates reach `resolve_extension_in_scope` per receiver, and a + // real-corpus measurement across that whole cluster showed FilteredCandidate + // rising by 449 (see that plan's final review) -- some fraction of which + // is this refusal firing on receivers it previously never reached at all. + // The reasoning above still holds; only its frequency changed. if definitions.len() != 1 { return Err(refusal( "identity is ambiguous — matches more than one definition", From 40308c09a1368d42ba2cb860d2e508eff453d776 Mon Sep 17 00:00:00 2001 From: Juraj Oceliak Date: Fri, 18 Sep 2026 10:05:22 +0200 Subject: [PATCH 7/7] fix: address Copilot review comments on PR #322 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - src/resolver/extension.rs: select_extension_symbol no longer collects every shape-matching declaration into a Vec before checking for an exact detail match. Two non-allocating iterator passes instead — real overhead since this is now called once per registry entry from implicit_receiver_extension_match's own loop. - src/resolver/infer.rs, docs/.../2026-09-16-extension-follow-ups-plan.md: extension_entry_is_in_scope's doc comment claimed "four of six callers are unrelated to extensions" naming only three, one of which (nullable_call_diagnostics's extension_in_scope_here) actually IS extension-registry-shaped (takes a real ExtensionEntry), just with its own stricter member-extension rule layered on top. Corrected to name the two genuinely unrelated callers (receiver-less by-name fallbacks with no ExtensionEntry at all) and describe the third accurately, tightened to stay concise per the same doc-comment discipline AGENTS.md already asks for. - Same file: a stale test-expectation snippet in the plan doc (Some("T?")) corrected to match the actual helper contract (Some("T") -- extract_return_type_from_detail strips nullable), matching what the real test in tests.rs already asserts. D1's memo-staleness concern (same root path, different SDK config) was already explicitly argued in platform_types.rs's own doc comment with a real precedent (the workspace scan has the same one-time-read staleness) -- left as-is, not re-litigated. Co-Authored-By: Claude Sonnet 5 --- .../2026-09-16-extension-follow-ups-plan.md | 18 ++++++---- src/resolver/extension.rs | 34 ++++++++++--------- src/resolver/infer.rs | 13 ++++--- 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md b/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md index 6359b73d..ada5e21a 100644 --- a/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md +++ b/docs/superpowers/plans/2026-09-16-extension-follow-ups-plan.md @@ -360,11 +360,14 @@ the record. /// receiver buckets; 3641 of those entries live in a default-import package, /// 3638 of them top-level, and every one was rejected here for every caller. /// -/// Deliberately NOT folded into [`extension_is_in_scope`] itself: four of that -/// function's six callers are not about extensions at all -/// (`candidate_declaration_is_reachable`, `Indexer::jar_candidate_is_reachable` -/// and `nullable_call_diagnostics`' stricter own rule), and widening the shared -/// predicate would change bare-name JAR candidate preference corpus-wide. +/// Deliberately NOT folded into [`extension_is_in_scope`] itself. Of its other +/// three direct callers: `candidate_declaration_is_reachable` and +/// `Indexer::jar_candidate_is_reachable` are receiver-less by-name fallbacks +/// with no `ExtensionEntry` at all, so widening the shared predicate would +/// change bare-name JAR candidate preference corpus-wide for callers this +/// rule was never about. `nullable_call_diagnostics`'s `extension_in_scope_here` +/// IS extension-registry-shaped but layers its own stricter member-extension +/// rule on top — a different contract, kept on the original predicate. pub(crate) fn extension_entry_is_in_scope( entry: &crate::types::ExtensionEntry, from_uri: &Url, @@ -1184,7 +1187,10 @@ fn a_default_import_packaged_extension_return_type_is_inferable_without_an_impor "firstOrNull", Some(&caller_uri), ), - Some("T?".to_owned()), + // `extract_return_type_from_detail` intentionally strips the trailing + // `?` (see `return_type_nullable_stripped` in this same file) — `"T"`, + // not `"T?"`, is this helper's real contract. + Some("T".to_owned()), "an unimporting Kotlin caller must be able to infer a \ kotlin.collections extension's return type" ); diff --git a/src/resolver/extension.rs b/src/resolver/extension.rs index 3eb27497..aa92395e 100644 --- a/src/resolver/extension.rs +++ b/src/resolver/extension.rs @@ -33,24 +33,26 @@ pub(super) fn select_extension_symbol<'file_data>( container: Option<&String>, detail: &str, ) -> Option<&'file_data SymbolEntry> { - let declaring_symbols: Vec<_> = file_data + let is_declaring_symbol = |symbol: &&SymbolEntry| { + crate::resolver::infer::extension_declaration_matches( + symbol, + name, + receiver_base, + container, + ) + }; + // Two non-allocating passes instead of collecting every shape-matching + // declaration into a `Vec` first: this is called once per registry entry + // by `implicit_receiver_extension_match`'s own per-entry loop, so an + // allocation and a full re-scan of every overload on every entry (the + // `Vec` version's cost even when the very first declaration is an exact + // match) is real, measurable overhead the old single-entry-point + // `.find(extension_declaration_matches)` never paid. + file_data .symbols .iter() - .filter(|symbol| { - crate::resolver::infer::extension_declaration_matches( - symbol, - name, - receiver_base, - container, - ) - }) - .collect(); - let exact_signature_match = declaring_symbols - .iter() - .find(|symbol| symbol.detail == detail); - exact_signature_match - .or_else(|| declaring_symbols.first()) - .copied() + .find(|symbol| is_declaring_symbol(symbol) && symbol.detail == detail) + .or_else(|| file_data.symbols.iter().find(is_declaring_symbol)) } /// The `Range` half of [`select_extension_symbol`], for callers that only diff --git a/src/resolver/infer.rs b/src/resolver/infer.rs index 4514dc09..eee0ca80 100644 --- a/src/resolver/infer.rs +++ b/src/resolver/infer.rs @@ -1874,11 +1874,14 @@ pub(crate) fn extension_is_in_scope( /// receiver buckets; 3641 of those entries live in a default-import package, /// 3638 of them top-level, and every one was rejected here for every caller. /// -/// Deliberately NOT folded into [`extension_is_in_scope`] itself: four of that -/// function's six callers are not about extensions at all -/// (`candidate_declaration_is_reachable`, `Indexer::jar_candidate_is_reachable` -/// and `nullable_call_diagnostics`' stricter own rule), and widening the shared -/// predicate would change bare-name JAR candidate preference corpus-wide. +/// Deliberately NOT folded into [`extension_is_in_scope`] itself. Of its other +/// three direct callers: `candidate_declaration_is_reachable` and +/// `Indexer::jar_candidate_is_reachable` are receiver-less by-name fallbacks +/// with no `ExtensionEntry` at all, so widening the shared predicate would +/// change bare-name JAR candidate preference corpus-wide for callers this +/// rule was never about. `nullable_call_diagnostics`'s `extension_in_scope_here` +/// IS extension-registry-shaped but layers its own stricter member-extension +/// rule on top — a different contract, kept on the original predicate. pub(crate) fn extension_entry_is_in_scope( entry: &crate::types::ExtensionEntry, from_uri: &Url,