fix(resolve): extension-registry overload-collapse family (4 fixes) - #321
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Address the unresolved hover, JAR mapping coverage, declaration disambiguation, and documentation consistency issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request fixes extension-function overload collapse, normalizes JAR receiver keys, and updates hover behavior with regression coverage.
Changes:
- Preserve all matching extension candidates and declaration ranges.
- Normalize nullable and nested JAR receiver keys; bump manifest cache version.
- Add resolver, JAR, hover, and documentation updates.
File summaries
| File | Summary |
|---|---|
src/resolver/tests.rs |
Adds resolver overload and precedence regressions. |
src/resolver/qualified.rs |
Preserves overloads across qualified-resolution tiers. |
src/resolver/extension.rs |
Collects extension candidates and selects declaration ranges. |
src/indexer/jar.rs |
Normalizes JAR extension receiver keys. |
src/indexer/jar_tests.rs |
Tests JAR key normalization and cache invalidation. |
src/indexer/jar_manifest_cache.rs |
Bumps the manifest cache schema version. |
src/features/hover.rs |
Handles same-arity candidate sets. |
src/features/hover_tests.rs |
Adds hover collision coverage. |
docs/superpowers/specs/2026-09-09-resolve-latency-precedence-split-design.md |
Documents future resolver design material. |
docs/superpowers/plans/2026-09-14-extension-registry-overload-collapse-plan.md |
Records diagnosis and implementation planning. |
docs/architecture/unified-resolution-strategy.md |
Documents the overload-collapse pattern. |
Review details
Suppressed comments (5)
docs/superpowers/plans/2026-09-14-extension-registry-overload-collapse-plan.md:331
- This newly added plan is stale at this point: the PR already implements
locations_share_call_signatureand adds the hover regression test, so the 'ship it, verify it' section and its 'Task 4' reversal are no longer deferred. Leaving this as the current decision contradicts the implementation and test plan; mark the completed work instead.
**Decision: ship it, verify it, and name the reversal in advance.**
- It is philosophically consistent with PR #304's own comment in that same function: showing one
arbitrary signature for a genuinely ambiguous name is worse than showing none.
- Task 1 Step 6 adds an explicit hover check on a colliding-extension shape, so the change is
*observed*, not assumed.
- The PR description must state the trade in one sentence.
- **Pre-designed reversal, if it proves unacceptable:** loosen `pick_unambiguous_location` to accept a
candidate set whose members all share the same name *and* the same arity, returning the first — a
~3-line change confined to `src/features/hover.rs` that restores hover **without** re-collapsing
resolution. This is deliberately *not* bundled into Task 1: it is an unmeasured hover-behaviour
change, and mixing it into a resolver PR would make the measurement ambiguous. It is Task 4.
docs/superpowers/plans/2026-09-14-extension-registry-overload-collapse-plan.md:743
- The added plan still says the String→CharSequence cause is unisolated and leaves the instrumentation tasks unchecked, while the PR description says this investigation was completed and identified the Kotlin default-import-package gap. Reconcile the plan/ledger with that result and leave only the follow-up; otherwise the repository documents contradictory diagnosis status.
**This task ships no production code.** The failure is verified real but its cause is not isolated,
and this project's own process lesson is that implementing against an un-isolated cause is how the
last two reverts happened. The critique independently agreed with deferring it — and noted it is
likely the **largest** of the three tasks, since it plausibly subsumes several Gap names this plan
excludes.
docs/superpowers/specs/2026-09-09-resolve-latency-precedence-split-design.md:435
- This added design spec is now stale relative to the resolver it describes:
resolve_extension_via_supertype_hierarchyreturns all nearest-level matches andQualifiedCandidatesstoresVec<Location>in this PR, not theOption/first-match contract asserted here. Keeping the old design in-repo will direct the planned refactor back to the overload-collapse bug; update or mark this section superseded.
/// The supertype-walk extension fallback
/// (`resolve_extension_via_supertype_hierarchy`). `Option`, not `Vec` —
/// `resolve_extension_via_supertype_hierarchy` already collapses to at
/// most one match before returning (resolve.rs:2372:
/// `matches.into_iter().next().into_iter().collect()`, with a comment
/// explaining a same-level sibling tie is deliberately resolved to
/// "take just the first" rather than surfaced as ambiguous) — a `Vec`
docs/superpowers/specs/2026-09-09-resolve-latency-precedence-split-design.md:5
- This new 841-line specification describes a future resolve latency/precedence/file split, not the extension-registry overload, JAR-key, or hover changes in this PR, and it is not mentioned in the PR description. Move it to a separate change or remove it so unrelated design work is not bundled into this review.
# `resolve.rs` Latency Signal, Member/Extension Precedence, and File Split — Design
Status: **proposed** (2026-09-09). Scope: `src/resolver/resolve.rs` (2673 lines). Written
against `fix/extension-supertype-variable-receiver`, after the supertype-extension fallback
(`with_supertype_extension_fallback` / `resolve_extension_via_supertype_hierarchy`) landed on
src/indexer/jar_tests.rs:3126
- This test constructs each manifest receiver by calling
extension_receiver_keyitself, so it starts after the changedbuild_jar_manifestmapping at lines 1515-1516. Reverting that production call to store the raw sidecar receiver would still make this test pass becausepopulate_tier1_from_manifesttrusts the value. Exercise the sidecar-to-manifest mapping (or feed raw values through that path) to cover the Tier-1 fix.
extension_receiver: Some(crate::indexer::jar::extension_receiver_key("String?")),
},
crate::indexer::jar_manifest_cache::JarManifestName {
name: "leaf".to_owned(),
kind: "fun".to_owned(),
- Files reviewed: 11/12 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+145
to
+146
| if locations.len() > 1 && locations_share_call_signature(indexer, &locations) { | ||
| return locations.into_iter().next(); |
Comment on lines
+1515
to
+1516
| extension_receiver: (!s.extension_receiver_type.is_empty()) | ||
| .then(|| extension_receiver_key(&s.extension_receiver_type)), |
Comment on lines
+47
to
+50
| let exact_signature_match = declaring_symbols | ||
| .iter() | ||
| .find(|symbol| symbol.detail == detail); | ||
| let selected = exact_signature_match.or_else(|| declaring_symbols.first()); |
Comment on lines
+168
to
+173
| .and_then(|fd| { | ||
| fd.symbols | ||
| .iter() | ||
| .find(|s| s.selection_range == location.range) | ||
| .map(|s| (s.name.clone(), s.arity_for_call_shape_check())) | ||
| }) |
Comment on lines
+7282
to
+7284
| /// `pick_unambiguous_location` consequently declines to render hover for | ||
| /// this exact shape, since more than one location comes back; see Task 4 of | ||
| /// the 2026-09-14 extension-registry-overload-collapse plan for whether that |
…addendum Adds the verified plan for the resolution-accuracy Gap-cluster investigation (root cause: extension-tier lookups collapse an overload set to one arbitrary candidate, plus a JAR extension-receiver key normalization gap), and an addendum to the unified-resolution-strategy doc documenting the same root cause's recurrence in a second, previously undocumented registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resolve_extension_in_scope, jar_extension_for_type_root, and QualifiedCandidates's own_type_extension/supertype_extension tiers used to collapse a real overload set down to one arbitrary candidate (.into_iter().next() / an early return inside the registry-entry loop) before the caller's own arity/shape filter ever got to pick the right one. All four now return the full candidate set; a shared select_extension_symbol_range helper (extension.rs) disambiguates each entry's declaration range by exact detail match, falling back to the first matching declaration. ambiguous_member_extension_name_collision_first_match_wins is rewritten to ambiguous_member_extension_name_collision_returns_the_whole_candidate_set: it pinned the collapse bug for colliding member extensions, so it now asserts the full 2-location candidate set (in declaration order) instead of one arbitrary match, applying the PR #304 precedent (qualified member lookups return every overload) to extensions. Hover trade: pick_unambiguous_location (src/features/hover.rs) declines to render hover when more than one location survives shape filtering, so colliding same-arity extension overloads (the B1 pattern) now lose hover where they previously showed one arbitrary match's docs; Task 4 of the 2026-09-14 extension-registry-overload-collapse plan is the candidate reversal if that loss is judged unacceptable. This could not be directly observed via the kmp-lsp CLI hover subcommand (it doesn't exercise compute_hover/pick_unambiguous_location at all -- only the real LSP server's textDocument/hover handler does), so the trade is supported by direct code reading plus this commit's own Vec<Location>-count test evidence, not a live hover request -- see task-1-report.md. Real-corpus measurement (resolution-accuracy on Moneta) did not confirm the plan's success criterion: firstOrNull did not leave the Gap top-20, and filtered_candidate_total rose 4.3%, which is the plan's own named trigger for a sibling-ancestor tie-break contingency in walk_hierarchy_breadth_first -- not attempted here per the plan's explicit "do not attempt under measurement pressure" instruction. Full numbers and an unconfirmed root-cause hypothesis are in task-1-report.md. 5 new tests + 1 rewritten; 1951 total passing; cargo clippy and cargo fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The JAR indexer derived its extension_by_receiver/jar_extension_receivers key differently from the parser: it stripped generics but never the nullable `?` marker, and never reduced a dotted nested-type receiver to its leaf. A JAR-compiled extension on a nullable or nested-type receiver (e.g. fun String?.orEmpty()) was indexed under a key no lookup site ever asks for, so it was never found. Add extension_receiver_key(), the one shared derivation both call sites (Tier-2's build_jar_file_data and Tier-1's build_jar_manifest) now use. Bump JAR_MANIFEST_CACHE_VERSION 3 -> 4: a v3-cached extension_receiver value is keyed by the old derivation and would silently mismatch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pick_unambiguous_location declined outright whenever more than one candidate survived, a side effect of the extension-registry overload-collapse fix now correctly surfacing colliding same-arity extension overloads (the Modifier.weight shape) as a candidate set instead of one arbitrary match. When every surviving candidate shares the same name and declared arity, render the first instead of declining — genuinely differing overloads still decline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- src/indexer/jar.rs: extract the sidecar-response-to-manifest-name mapping (including the extension_receiver_key normalization call) into its own sidecar_symbols_to_manifest_names function, so a unit test can exercise the real production mapping directly. - src/indexer/jar_tests.rs: jar_manifest_tier1_receiver_key_matches_tier2 now builds raw SidecarSymbols and feeds them through sidecar_symbols_to_manifest_names, instead of pre-normalizing the receiver key itself before calling populate_tier1_from_manifest — the old version would still pass if the production call site's own normalization were reverted. - docs/.../2026-09-14-extension-registry-overload-collapse-plan.md: checked off every executed step (Tasks 1-3), added an outcome note reconciling Decision S3 with Task 4 actually shipping in this PR, and an outcome note on Task 3 recording its real finding (a Kotlin-default-import scope gap in extension_is_in_scope, not either leading hypothesis). Also fixed a stale commit-message template (old attribution, wrong model name). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Hessesian
force-pushed
the
fix/extension-registry-overload-collapse
branch
from
September 16, 2026 06:49
8cc196d to
21108e1
Compare
5 tasks
Hessesian
added a commit
that referenced
this pull request
Sep 18, 2026
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 (878cc4e..b0ad175 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 <noreply@anthropic.com> * fix: address Copilot review comments on PR #322 - 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an "overload-collapse" bug family in the extension-function resolver, where a plural
registry (
extension_by_receiver: DashMap<String, Vec<ExtensionEntry>>) was narrowed to thefirst matching candidate at several consumer sites, even though each site's own signature
promises every candidate — the same bug class PR #304 already fixed for the member-lookup tier,
never swept to the structurally identical extension tier.
resolve_extension_in_scope,jar_extension_for_type_root, andQualifiedCandidates'sown_type_extension/supertype_extensiontiers now return every realcandidate instead of collapsing to one. New shared
select_extension_symbol_rangehelperdisambiguates each entry's declaration range.
extension_by_receiverkeys withthe same three normalizations the source-side parser already does (strip generics, strip
nullable, take dotted leaf) — fixes nullable-receiver (
orEmpty) and nested-type-receiverextensions being filed under the wrong key.
String→CharSequencesupertype-walk dead-end;real cause is a Kotlin-default-import-package gap in
extension_is_in_scope, not either of thetwo hypotheses considered going in. Recorded as a follow-up, not fixed here.
the resolver fix), narrowly, without re-collapsing the underlying resolution.
Full design, evidence, and a multi-round critique/revision cycle: see
docs/superpowers/plans/2026-09-14-extension-registry-overload-collapse-plan.md.Follow-ups found, deliberately not fixed here (see plan/ledger for detail)
implicit_receiver_extension_matchhas the identical first-match bug (named Task 5 in theplan, deferred on purpose — a separate entry point, no measured Gap name points at it yet).
find_extension_fn_return_type(type-inference side) has the same family of bug.List-receiver "false unique win" inresolve_chain's IndexOnly tail (a kotlin-stdlib JARdecoy outranks the real
java.util.Listbefore the builtin-type-platform fallback ever runs).renamenow correctly refuses on a same-arity collision where it previously renamed anarbitrary candidate (safety-direction behavior change, worth knowing about).
Test plan
cargo test— 1956 passed, 0 failed, 3 ignoredcargo clippy --all-targets -- -D warnings— cleancargo fmt -- --check— cleanwhole-branch review checking cross-task interaction — both clean, see ledger
orEmptyand nested-type JAR extensions confirmedresolving post-fix (nullable-suffixed key count 54→0);
firstOrNull's own real-corpusimprovement did not materialize — root cause is the separately-tracked
Listfalse-unique-winfollow-up above, not this PR's own fixes, which are independently verified correct via
unit tests
🤖 Generated with Claude Code