[ls] Implement initial local completions#160
Conversation
Confidence 4/5 - Solid foundation for local completions with thorough baseline testingThis PR introduces local completions through two key components: a public metadata API exposing locally-scoped type bindings, and new completion sources that leverage the scope graph for intelligent suggestions. Implementation Quality
Test CoverageThe snapshot test fixture comprehensively validates six distinct scenarios:
Potential Considerations
API DesignThe new public methods are minimal and straightforward: simple field accessors ( The PR is ready for merge pending any feedback on the implicit binding cursor filtering strategy and acceptance of the test coverage breadth. WalkthroughThis pull request extends the type checker to store forall and implicit type-variable bindings for completion purposes, then implements scope-based completion suggestions in the LSP analyzer. The changes thread stored type information from checking through lowering infrastructure to enable the completion engine to suggest locally visible terms and type variables at cursor position. ChangesScope-based completion with stored type-variable bindings
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #160 +/- ##
==========================================
+ Coverage 83.21% 83.29% +0.07%
==========================================
Files 133 133
Lines 25149 25454 +305
Branches 25149 25454 +305
==========================================
+ Hits 20928 21201 +273
- Misses 2266 2279 +13
- Partials 1955 1974 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
compiler-lsp/analyzer/src/completion/sources.rs (1)
196-224: ImplicitBindings double-lookup is required (not redundant).
ImplicitBindings::iter()yields(&str, &[TypeId]), which is the data needed forimplicit_binding_at_cursor(..). The laterbindings.get(name)is necessary becauseImplicitBindings::get()returns the distinctImplicitBindingId(Idx<SmolStr>), which is whatCompletionResolveData::ImplicitTypeVariable(.., binding_id)requires.
Optional: if this extraIndexMaplookup per entry is a concern, consider exposing an iterator that also yieldsImplicitBindingIdalongside the&[TypeId]and name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-lsp/analyzer/src/completion/sources.rs` around lines 196 - 224, The code currently calls bindings.iter() to get (&str, &[TypeId]) for implicit_binding_at_cursor(...) and then performs a second lookup with bindings.get(name) to obtain the ImplicitBindingId required by CompletionResolveData::ImplicitTypeVariable; this double-lookup is intentional and must be preserved. Keep the GraphNode::Implicit branch logic: use bindings.iter() for the cursor-check via implicit_binding_at_cursor(context, type_id), but still call bindings.get(name) and use the returned ImplicitBindingId (binding_id) when constructing CompletionResolveData::ImplicitTypeVariable(context.current_file, node_id, binding_id). If performance is a concern, consider adding an iterator on ImplicitBindings that yields (name, &[TypeId], ImplicitBindingId) and use that instead, but do not remove the bindings.get(name) lookup as it supplies the distinct binding_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@compiler-lsp/analyzer/src/completion/sources.rs`:
- Around line 196-224: The code currently calls bindings.iter() to get (&str,
&[TypeId]) for implicit_binding_at_cursor(...) and then performs a second lookup
with bindings.get(name) to obtain the ImplicitBindingId required by
CompletionResolveData::ImplicitTypeVariable; this double-lookup is intentional
and must be preserved. Keep the GraphNode::Implicit branch logic: use
bindings.iter() for the cursor-check via implicit_binding_at_cursor(context,
type_id), but still call bindings.get(name) and use the returned
ImplicitBindingId (binding_id) when constructing
CompletionResolveData::ImplicitTypeVariable(context.current_file, node_id,
binding_id). If performance is a concern, consider adding an iterator on
ImplicitBindings that yields (name, &[TypeId], ImplicitBindingId) and use that
instead, but do not remove the bindings.get(name) lookup as it supplies the
distinct binding_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf5af82c-9061-4757-b743-c8739073c7bc
⛔ Files ignored due to path filters (1)
tests-integration/fixtures/lsp/1751394000_completion_local/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
compiler-core/checking/src/core/zonk.rscompiler-core/checking/src/lib.rscompiler-core/checking/src/source/type_items.rscompiler-core/checking/src/source/types.rscompiler-core/lowering/src/scope.rscompiler-core/stabilizing/src/id.rscompiler-lsp/analyzer/src/completion.rscompiler-lsp/analyzer/src/completion/prelude.rscompiler-lsp/analyzer/src/completion/resolve.rscompiler-lsp/analyzer/src/completion/sources.rstests-integration/fixtures/lsp/1751394000_completion_local/Main.purs
Adds completions for local bindings using the scope graph API