Skip to content

[ls] Implement initial local completions#160

Merged
purefunctor merged 2 commits into
mainfrom
justin/local-completions
Jun 11, 2026
Merged

[ls] Implement initial local completions#160
purefunctor merged 2 commits into
mainfrom
justin/local-completions

Conversation

@purefunctor

Copy link
Copy Markdown
Owner

Adds completions for local bindings using the scope graph API

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

Confidence 4/5 - Solid foundation for local completions with thorough baseline testing

This 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

  • Complete handler coverage: All 8 CompletionResolveData enum variants have corresponding resolve implementations, ensuring every completion type can be resolved to display details
  • Clean scope resolution: The scope_node() method gracefully handles token position mapping with three cases (none, single, between tokens), propagating errors appropriately through Result<Option<...>>
  • Deterministic results: Completion items from binders and let-bindings are sorted by name, with deduplication via FxHashSet to prevent duplicate suggestions
  • Consistent architecture: The new ScopeTerms and ScopeTypes completion sources follow existing patterns and integrate seamlessly with the filtering and collection pipeline

Test Coverage

The snapshot test fixture comprehensively validates six distinct scenarios:

  • Local variable bindings from multiple scopes (let-bindings, function parameters, record puns, top-level definitions)
  • Local data constructors with proper type information
  • Local type variables (both explicit forall bindings and implicit type variables)
  • Correct scoping hierarchy (closer bindings shadow outer ones)

Potential Considerations

  • Implicit binding cursor logic: The implicit_binding_at_cursor helper depends on syntax pointer stabilisation; if stabilisation fails for a type ID, the binding silently filters out rather than surfacing an error—acceptable but worth monitoring for edge cases
  • Limited edge case testing: The snapshot covers the main paths well but doesn't explicitly test scenarios like empty scopes, deeply nested bindings, or malformed syntax trees
  • Minimal documentation: The code lacks inline comments explaining the scope graph traversal or cursor-to-scope resolution strategy, which could aid future maintenance

API Design

The new public methods are minimal and straightforward: simple field accessors (lookup_forall_binding, lookup_implicit_binding) on CheckedNodes, and iterator methods on scope types. No unsafe code or complex abstractions introduced.

The PR is ready for merge pending any feedback on the implicit binding cursor filtering strategy and acceptance of the test coverage breadth.

Walkthrough

This 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.

Changes

Scope-based completion with stored type-variable bindings

Layer / File(s) Summary
Type-variable binding storage in checker
compiler-core/checking/src/lib.rs, compiler-core/checking/src/source/types.rs, compiler-core/checking/src/source/type_items.rs, compiler-core/checking/src/core/zonk.rs
CheckedNodes gains forall_bindings and implicit_bindings hash maps storing type IDs keyed by binding and node IDs. Type checking in infer_kind_core and check_type_variable_binding now records computed kinds into these maps, and zonking applies type-ID folding to both new maps.
Lowering infrastructure for bindings and node lookup
compiler-core/lowering/src/scope.rs, compiler-core/stabilizing/src/id.rs
ImplicitBindings::iter() exposes implicit binding streams; LoweringGraphNodes gains getter methods (binder_node, expression_node, type_node, let_node) to retrieve graph node IDs for AST entities. AstId::into_raw method enables serialisation of AST node identifiers.
Completion context scope resolution
compiler-lsp/analyzer/src/completion/prelude.rs
CompletionContext adds offset: TextSize field and new scope_node() method that resolves cursor position to containing lowered graph construct by lowering the file, locating the syntax token, traversing ancestors, and resolving let-binding pointers.
Completion resolution for local entities
compiler-lsp/analyzer/src/completion/resolve.rs
CompletionResolveData gains variants for binder, let, record pun, forall type variable, and implicit type variable. New resolver helpers render local signatures via checked-module lookups. Serde adapter module ast_id serialises/deserialises AstId with NonZeroU32 validation.
Scope-based completion sources
compiler-lsp/analyzer/src/completion/sources.rs
New ScopeTerms source traverses lowered scope graph from cursor node to emit completion items for visible binders and let-bindings. New ScopeTypes source suggests forall type variables and cursor-filtered implicit type variables, deduplicating by name and sorting for stable results.
Completion pipeline integration and wiring
compiler-lsp/analyzer/src/completion.rs
CompletionContext construction now passes offset argument. CursorText::None and CursorText::Name code paths now call ScopeTerms.collect_into() and ScopeTypes.collect_into() alongside existing local and imported completion collections.
Test fixture for scope completion
tests-integration/fixtures/lsp/1751394000_completion_local/Main.purs
check function updated to accept Int and record { vPun :: Int } instead of two Int parameters. New Check5 type alias, CheckClass typeclass, and checkInstance instance added to exercise scope-completion suggestions.

Possibly related PRs

  • purefunctor/purescript-alexandrite#140: Both PRs modify the LSP analyser's completion pipeline—especially compiler-lsp/analyzer/src/completion/prelude.rs and compiler-lsp/analyzer/src/completion.rs—to change cursor/position handling; the retrieved PR negotiates PositionEncoding for encoding-aware range/offset conversions alongside this PR's scope-based offset threading.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset, which adds completions for local bindings using the scope graph API across multiple modules.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.62295% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.29%. Comparing base (ab0d4a4) to head (da9e02a).

Files with missing lines Patch % Lines
compiler-lsp/analyzer/src/completion/prelude.rs 59.25% 17 Missing and 5 partials ⚠️
compiler-lsp/analyzer/src/completion/sources.rs 89.28% 5 Missing and 7 partials ⚠️
compiler-lsp/analyzer/src/completion/resolve.rs 91.08% 0 Missing and 9 partials ⚠️
compiler-core/lowering/src/scope.rs 60.00% 6 Missing ⚠️
compiler-lsp/analyzer/src/completion.rs 20.00% 0 Missing and 4 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 for implicit_binding_at_cursor(..). The later bindings.get(name) is necessary because ImplicitBindings::get() returns the distinct ImplicitBindingId (Idx<SmolStr>), which is what CompletionResolveData::ImplicitTypeVariable(.., binding_id) requires.
Optional: if this extra IndexMap lookup per entry is a concern, consider exposing an iterator that also yields ImplicitBindingId alongside 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

📥 Commits

Reviewing files that changed from the base of the PR and between ab0d4a4 and da9e02a.

⛔ Files ignored due to path filters (1)
  • tests-integration/fixtures/lsp/1751394000_completion_local/Main.snap is excluded by !**/*.snap
📒 Files selected for processing (11)
  • compiler-core/checking/src/core/zonk.rs
  • compiler-core/checking/src/lib.rs
  • compiler-core/checking/src/source/type_items.rs
  • compiler-core/checking/src/source/types.rs
  • compiler-core/lowering/src/scope.rs
  • compiler-core/stabilizing/src/id.rs
  • compiler-lsp/analyzer/src/completion.rs
  • compiler-lsp/analyzer/src/completion/prelude.rs
  • compiler-lsp/analyzer/src/completion/resolve.rs
  • compiler-lsp/analyzer/src/completion/sources.rs
  • tests-integration/fixtures/lsp/1751394000_completion_local/Main.purs

@purefunctor purefunctor merged commit 1a819ca into main Jun 11, 2026
12 of 13 checks passed
@purefunctor purefunctor deleted the justin/local-completions branch June 11, 2026 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant