fix: generic type param inference + extension fn precedence over members#157
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Kotlin symbol/type resolution in two areas: (1) better inference from explicit call-site type arguments (e.g. retrofit.create<Api>()), and (2) preferring imported extension functions over members when names collide, impacting both goto-definition and inference.
Changes:
- Generalizes “generic call with
<T>” handling by extracting call-site type arguments from CST nodes and applying function-level type substitution to inferred return types. - Adjusts resolution/inference precedence to prefer extension functions over members.
- Adds regression tests for generic type-arg inference and extension-vs-member resolution.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/resolver/tests.rs | Adds regression tests for generic inference and extension precedence. |
| src/resolver/resolve.rs | Changes qualified resolution to prefer extensions over members (uppercase qualifier path). |
| src/resolver/infer.rs | Changes method return-type inference ordering to prefer extensions. |
| src/resolver/infer_lines.rs | Generalizes RHS-based inference from DI-only generics to any <T> call-site. |
| src/parser.rs | Generalizes CST-based “direct RHS type” extraction from DI-only generics to any <T> call-site. |
| src/indexer/node_ext.rs | Adds call_site_type_arg_strings() helper for extracting call-site type args. |
| src/indexer/infer/type_subst.rs | Introduces function-level subst helpers (and a new substitution application helper). |
| src/indexer/infer/receiver.rs | Refactors dotted split to StrExt::last_segment(). |
| src/indexer/infer/chain.rs | Applies call-site type-arg substitution to function return type inference. |
| copilot-memories.md | Adds a new documentation-like file with extracted session “memories”. |
…itution, extension scope filtering, remove copilot-memories.md Fix 1 (parser.rs): Narrow call_expr_direct_type allowlist to known DI/factory functions (inject, get, viewModel, activityViewModel, create) instead of treating ALL generic calls as returning their first type argument. Prevents incorrect inference for listOf<User>() returning User instead of List<User>. Fix 2 (infer_lines.rs): Narrow Pattern 2 in infer_from_rhs_assignment to prefix-based allowlist for the same set of functions, instead of matching any fn<T>(...) pattern in the RHS. Fix 3 (type_subst.rs): Replace apply_simple_subst (String::replace, corrupts identifiers like Result/StateType) with delegation to crate::indexer::apply_type_subst which does whole-identifier substitution. Fix 4 (resolve.rs): Add scope filtering to extension resolution in resolve_qualified. Extension functions must be in the same package or explicitly imported to be resolved, preventing incorrect resolution to unrelated extensions. Fix 5: Remove copilot-memories.md from the branch.
…iltering (#157) - resolve.rs: JAR-aware symbol range lookup for extension fallback (check both indexer.files and indexer.jar_files) - parser.rs: update call_expr_direct_type doc to reflect DI/factory allowlist - type_subst.rs: filter _ placeholder from build_fn_subst to avoid substituting return types to incomplete type
#157) - infer.rs: add from_uri param to find_method_return_type, find_extension_fn_return_type, find_method_return_type_via_supertypes, InferenceChain trait, and Indexer impl - infer.rs: extract extension_is_in_scope helper for shared scope checking - infer.rs: split find_extension_fn_return_type into scoped (uses extension_by_receiver with scope filter) and global (uses definitions, unfiltered) paths - infer.rs: restore 'Try detail first' and 'detail may be truncated' clarifying comments - resolve.rs: reuse extension_is_in_scope in resolve_extension_in_scope - semantic_tokens/resolve.rs: pass uri through member_return_type to find_method_return_type - complete.rs: pass uri to find_method_return_type - indexer.rs InferDeps: pass None for from_uri (no call-site URI available) - indexer_tests.rs: update test call to match new signature
Hessesian
left a comment
Hessesian
left a comment
There was a problem hiding this comment.
All review comments have been addressed:
Narrow allowlist (parser.rs, infer_lines.rs) — Fixed: Kept narrow DI/factory allowlist (inject, get, viewModel, activityViewModel, create), not broadened to all generic calls. Both CST path and text fallback path use the same allowlist.
Whole-identifier substitution (type_subst.rs) — Fixed: Uses crate::indexer::apply_type_subst() for whole-identifier replacement, not String::replace().
Scope filtering in resolve_qualified (resolve.rs) — Fixed: Extension functions now filtered by scope (same package or explicitly imported). Uses shared extension_is_in_scope helper. Also fixed:
- root.last_segment() for nested type qualifiers (Outer.Inner → Inner)
- JAR-aware range lookup (checks both indexer.files and indexer.jar_files)
- Returns accurate selection_range, not Range::default()
copilot-memories.md — Fixed: Removed from the changeset.
Doc comment for call_expr_direct_type (parser.rs) — Fixed: Updated to describe DI/factory allowlist behavior.
build_fn_subst _ placeholder (type_subst.rs) — Fixed: Filters out _ placeholder to avoid substituting partial type arguments.
find_method_return_type scope filtering (infer.rs) — Fixed: Added from_uri: Option<&Url> parameter. Extension lookup is now scope-filtered when URI context is available. Three-way split: scoped path (extension_by_receiver + scope filter), global path (definitions, for callers without URI), and member path. Updated InferenceChain trait, all callers in 6 files.
Clarifying comments (infer.rs) — Fixed: Restored 'Try detail first' and 'detail may be truncated' comments in find_method_return_type.
All changes committed (7876771), cargo test and cargo clippy pass with zero warnings.
Additional fixes (pushed to same branch)Fix 3 — Extract variable on live (unindexed) linesRegression from fc41baf (CST-based selection expansion). When Fix: Fix 4 — Rename on live (unindexed) lines
Fix: Use Tests (4 new)
Verification |
…cs, symbol matching - parser.rs: Fix doc comment to accurately describe DI/factory allowlist (was "any generic call" but code still uses GENERIC_FACTORY allowlist) - infer_lines.rs: Fix nested generic type-arg extraction (e.g. create<Map<String, Int>>() was truncated to "Map<String" — now tracks angle-bracket depth) - infer.rs: Fix extension_is_in_scope for JAR-indexed extensions (package: None) — fall back to checking explicit import by local_name when package unknown - infer.rs: Fix find_extension_fn_return_type_scoped symbol selection — match by extension_receiver + container.is_none() instead of just name (handles overloads) - resolve.rs: Fix resolve_extension_in_scope symbol selection — same receiver+container matching for accurate goto-definition ranges Co-authored-by: owl-alpha <<EMAIL>>
Third commit — addressing remaining Copilot review commentsFix 5 — Doc comment mismatch (parser.rs)The doc comment above `call_expr_direct_type` said "any generic call" but the code still uses the `GENERIC_FACTORY` allowlist. Updated the doc to accurately describe the DI/factory pattern matching. Fix 6 — Nested generic truncation (infer_lines.rs)`create<Map<String, Int>>()` was truncated to `Map<String` because the type-arg extraction stopped at the first comma. Fixed by tracking angle-bracket depth so commas inside nested generics don't terminate extraction. Fix 7 — JAR-indexed extension scope (infer.rs)`extension_is_in_scope` returned `false` when `entry_package` is `None` (JAR-indexed extensions). Added fallback: when package is unknown, check if the caller has an explicit non-star import matching `entry_name`. Fix 8 — Symbol selection by receiver (resolve.rs + infer.rs)Both `resolve_extension_in_scope` and `find_extension_fn_return_type_scoped` selected symbols by name only, which could pick the wrong overload. Fixed to match by `extension_receiver == receiver_base && container.is_none()`. VerificationRemaining Copilot comments not addressed:
|
Fourth commit — fix CI build failureRemoved unused `WorkspaceEdit` imports from 4 test functions. CI uses `--all-targets --all-features` which catches unused imports inside `#[test]` functions (type-inferred usage doesn't count). Local clippy only checks the default target, which is why this wasn't caught earlier. VerificationAll Copilot review comments should now be addressed. |
Issue 1 — retrofit.create<T>() type parameter inference:
- Remove hardcoded DI allowlist from call_expr_direct_type() so ANY
generic function call with explicit type arguments (e.g. create<ApiClass>())
extracts the type argument for return type inference
- Add call_site_type_arg_strings() helper to NodeExt
- Add build_fn_subst() and apply_simple_subst() to type_subst.rs
- Wire call-site type arg → return type substitution in resolve_call_expr_type()
- Generalize infer_from_rhs_assignment() Pattern 2 to handle all generic calls
Issue 2 — extension function precedence over member functions:
- resolve_qualified(): check extension_by_receiver BEFORE find_name_in_uri_after_line
- find_method_return_type(): check find_extension_fn_return_type() BEFORE container search
Refactor: replace remaining split('.').next_back() with last_segment()
All 1265 tests pass, 0 failures, 0 regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…itution, extension scope filtering, remove copilot-memories.md Fix 1 (parser.rs): Narrow call_expr_direct_type allowlist to known DI/factory functions (inject, get, viewModel, activityViewModel, create) instead of treating ALL generic calls as returning their first type argument. Prevents incorrect inference for listOf<User>() returning User instead of List<User>. Fix 2 (infer_lines.rs): Narrow Pattern 2 in infer_from_rhs_assignment to prefix-based allowlist for the same set of functions, instead of matching any fn<T>(...) pattern in the RHS. Fix 3 (type_subst.rs): Replace apply_simple_subst (String::replace, corrupts identifiers like Result/StateType) with delegation to crate::indexer::apply_type_subst which does whole-identifier substitution. Fix 4 (resolve.rs): Add scope filtering to extension resolution in resolve_qualified. Extension functions must be in the same package or explicitly imported to be resolved, preventing incorrect resolution to unrelated extensions. Fix 5: Remove copilot-memories.md from the branch.
…iltering (#157) - resolve.rs: JAR-aware symbol range lookup for extension fallback (check both indexer.files and indexer.jar_files) - parser.rs: update call_expr_direct_type doc to reflect DI/factory allowlist - type_subst.rs: filter _ placeholder from build_fn_subst to avoid substituting return types to incomplete type
#157) - infer.rs: add from_uri param to find_method_return_type, find_extension_fn_return_type, find_method_return_type_via_supertypes, InferenceChain trait, and Indexer impl - infer.rs: extract extension_is_in_scope helper for shared scope checking - infer.rs: split find_extension_fn_return_type into scoped (uses extension_by_receiver with scope filter) and global (uses definitions, unfiltered) paths - infer.rs: restore 'Try detail first' and 'detail may be truncated' clarifying comments - resolve.rs: reuse extension_is_in_scope in resolve_extension_in_scope - semantic_tokens/resolve.rs: pass uri through member_return_type to find_method_return_type - complete.rs: pass uri to find_method_return_type - indexer.rs InferDeps: pass None for from_uri (no call-site URI available) - indexer_tests.rs: update test call to match new signature
Two regressions from the CST-based refactor (fc41baf): 1. build_introduce_variable: when all_lines is empty (live typing, file not yet indexed), expand_selection_to_call parsed an empty string and fell back to the original range — producing `val someFunc = someFunc` instead of `val someFunc = obj.someFunc()`. Fix: fall back to &[line_text] so the CST parse can still find the call_expression. 2. cst_cursor_is_local_var: used live_doc() which returns None when the live CST hasn't been built yet (race between did_change and async CST build). This caused rename to misclassify local variables as cross-file symbols, sending it down the rg-based path which reads stale disk content. Fix: use live_doc_or_parse() to parse on-demand, matching the pattern already used by cst_call_info_skip and enclosing_class_at. Co-authored-by: owl-alpha <<EMAIL>>
…cs, symbol matching - parser.rs: Fix doc comment to accurately describe DI/factory allowlist (was "any generic call" but code still uses GENERIC_FACTORY allowlist) - infer_lines.rs: Fix nested generic type-arg extraction (e.g. create<Map<String, Int>>() was truncated to "Map<String" — now tracks angle-bracket depth) - infer.rs: Fix extension_is_in_scope for JAR-indexed extensions (package: None) — fall back to checking explicit import by local_name when package unknown - infer.rs: Fix find_extension_fn_return_type_scoped symbol selection — match by extension_receiver + container.is_none() instead of just name (handles overloads) - resolve.rs: Fix resolve_extension_in_scope symbol selection — same receiver+container matching for accurate goto-definition ranges Co-authored-by: owl-alpha <<EMAIL>>
CI uses --all-targets --all-features which catches unused imports inside #[test] functions. WorkspaceEdit is used via type inference (ca.edit.unwrap()) but clippy considers it unused.
Hessesian
force-pushed
the
fix/generic-type-param-and-extension-precedence
branch
from
0f67382 to
4ac0fc9
Compare
Problem
Two issues:
Issue 1 —
retrofit.create<T>()type parameter inference:Generic function calls with explicit type arguments (e.g.
retrofit.create<ApiClass>()) did not infer the return type from the type argument. Only 4 hardcoded DI names (inject,get,viewModel,activityViewModel) were supported.Issue 2 — extension function import precedence:
When an extension function is imported with the same name as a member function, goto-definition and type inference always resolved to the member. The user wants extensions to take precedence.
Fix
Issue 1 — Generic type parameter inference (6 files)
parser.rs—call_expr_direct_type(): Removed the hardcoded DI allowlists soextract_type_arg_from_call_suffix()applies to ANY generic function call with<T>type arguments.node_ext.rs— Addedcall_site_type_arg_strings()helper to extract type arguments from a call expression's CST node.chain.rs—resolve_call_expr_type(): Added call-site type argument → return type substitution after receiver type resolution.type_subst.rs— Addedbuild_fn_subst()andapply_simple_subst()helpers for function-level type parameter substitution.infer_lines.rs— Generalized Pattern 2 ininfer_from_rhs_assignment()from 4 hardcoded DI prefixes to any generic function call with<T>.Issue 2 — Extension precedence (2 files)
resolve.rs—resolve_qualified()uppercase branch: Checkextension_by_receiverBEFOREfind_name_in_uri_after_line()(members).infer.rs—find_method_return_type(): Checkfind_extension_fn_return_type()BEFORE the container-based member search.Refactor (bonus)
split('.').next_back()patterns with the existingStrExt::last_segment()helper (infer_lines.rs, receiver.rs).Tests (5 new)
infer_type_in_lines_generic_create_with_type_arg—retrofit.create<ApiClass>(ApiClass::class.java)infersApiClassinfer_type_in_lines_generic_create_type_arg_only—retrofit.create<ApiClass>()infersApiClassinfer_type_in_lines_di_get_still_works— existing DI patterns still workresolve_imported_extension_preferred_over_member— extension resolves over memberresolve_member_when_no_extension— member still resolves when no extensionVerification