Skip to content

Improved reference <> definition support#1165

Merged
sbillig merged 33 commits intoargotorg:masterfrom
micahscopes:symbol-tracking
Dec 8, 2025
Merged

Improved reference <> definition support#1165
sbillig merged 33 commits intoargotorg:masterfrom
micahscopes:symbol-tracking

Conversation

@micahscopes
Copy link
Collaborator

@micahscopes micahscopes commented Nov 25, 2025

Builds on #1162 (traversal API) to add a reference view API at the HIR semantic layer, then uses it to power a bunch of LSP functionality.

Changes

HIR semantic layer (hir::semantic::reference):

  • ReferenceView abstraction for tracking symbolic references (paths, field accesses, method calls, use paths)
  • Target enum that unifies module-level scopes and local bindings
  • TargetResolution for handling ambiguous references gracefully (returns all candidates)
  • HasReferences trait + collector for finding all references to a target across a module or ingot
  • Proper resolution for use paths, including decomposed use foo::{bar, baz} statements and child file modules

LSP handlers (language-server):

  • goto-definition - expanded to handle path segments, where clauses, locals, ambiguous refs
  • find-all-references - ingot-wide search for both scopes and locals
  • document highlight
  • rename symbol (non-module)
  • go to type definition
  • hover - now works on local bindings, shows multiple candidates for ambiguous refs
  • semantic tokens (basic)

Fixes along the way:

  • Local bindings now properly shadow module-level items
  • Rename replaces just the identifier, not the whole path (Foo::Bar → Foo::Baz not just Baz)
  • Reference spans return the token not the whole expression
  • File module goto works (was using relative path instead of file:// URL)
  • Use path resolution starts from the correct scope

What's left for followup

  • Module renaming (needs to handle file renames)
  • Cross-ingot references (@g-r-a-n-t can we take a look at this together?)
  • Semantic tokens could use more granularity
  • Integration tests for the LSP handlers beyond snapshot tests

@micahscopes micahscopes force-pushed the symbol-tracking branch 2 times, most recently from f5f8ff9 to a5b3c2f Compare December 4, 2025 08:05
@micahscopes micahscopes changed the title Symbol tracking Improved reference <> definition support Dec 5, 2025
@micahscopes micahscopes marked this pull request as ready for review December 5, 2025 21:30
Comment on lines 225 to 240
fn local_target(&self, db: &'db dyn HirAnalysisDb) -> Option<Target<'db>> {
let body = self.scope.body()?;
let func = body.containing_func(db)?;
let expr_id = body.find_expr_for_path(db, self.path)?;
let (_, typed_body) = check_func_body(db, func);

let span = typed_body.expr_binding_def_span(func, expr_id)?;
let ty = typed_body.expr_ty(db, expr_id);

Some(Target::Local {
span,
ty,
func,
expr: expr_id,
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

PathView as it's defined, and this function (and find_expr_for_path) don't suffice for variable names in function bodies, where scoping rules rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm working on this, still not satisfied though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be addressed now, not in the most elegant way, but effectively

@micahscopes micahscopes force-pushed the symbol-tracking branch 2 times, most recently from 5c0409f to ab1bf80 Compare December 7, 2025 16:56
@micahscopes
Copy link
Collaborator Author

micahscopes commented Dec 7, 2025

@sbillig I think this is much better now, though not the most elegant

Local variables and parameters should shadow module-level items with
the same name. Modified PathView::target() and target_at() to check
local bindings first before falling back to module-level resolution.

Updated test snapshots to reflect correct behavior where parameters
and local bindings now resolve as locals instead of incorrectly
resolving to module paths or value references.

Added locals.fe test case to verify local shadowing works correctly,
including parameter shadowing by let bindings.
When renaming Bar in Foo::Bar, the entire path was being replaced
with just the new name instead of producing Foo::Baz.

Added PathView::last_segment_span() to get only the identifier span
of the last segment. Added ReferenceView::rename_span() that delegates
to last_segment_span for paths while returning normal spans for other
reference types. Updated rename handler to use rename_span() instead
of span() for text edits.

This preserves goto-definition behavior (which needs full path spans
to detect intermediate segment clicks) while fixing rename to only
replace the actual referenced item.
Added goto_path_segments.fe to verify all intermediate path segments
are resolvable for goto-definition (e.g. clicking on 'stuff' in
'stuff::calculations::ambiguous' resolves correctly).

Added goto_where_clause.fe to verify trait bounds in where clauses
are resolvable (e.g. clicking on 'MyTrait' in 'where T: MyTrait'
resolves to the trait definition).

Both tests cover inline module resolution. Cross-ingot resolution
is blocked pending PR argotorg#1146 which adds ingot initialization when
opening .fe files.
When use statements with braces like `use foo::{ bar, baz }` are
decomposed into separate Use HIR items, they share identical item spans
pointing to the original source. This caused find_enclosing_item to
return only the first matching use, breaking goto for subsequent items.

Changed find_enclosing_item to find_enclosing_items (returning all
items with the smallest span) and reference_at now tries each candidate
until finding one whose references match the cursor position.

This handles decomposed uses naturally without special-casing.
Scope graphs don't include edges to child file modules, only to items
declared in the file itself. This caused use statements like `use stuff::`
to fail when `stuff` is a child file module (stuff.fe).

Added fallback in UsePathView::target to check the module tree for child
file modules when normal name resolution fails. This allows resolving the
first segment of use paths to child file modules.
Extended the semantic traversal API to handle ambiguous references where
a path can resolve to multiple items (e.g., both a module and function
with the same name).

API design:
- target() / target_at() - returns first/best match (existing behavior)
- target_candidates() / target_candidates_at() - returns all candidates

This provides a coherent API where:
- Simple cases use the singular methods
- Ambiguous resolution uses the plural methods
- LSP handlers can return arrays for multiple goto targets
- All ambiguity handling is encapsulated in the HIR semantic layer

The goto handler now returns GotoDefinitionResponse::Array when multiple
targets exist, allowing editors to present all options to the user.
When navigating to TopLevelMod (file modules), we were incorrectly using
file.path(db) which returns a relative path, then formatting it as a file://
URL. This created invalid URLs like 'file://src/stuff.fe' instead of valid
absolute file:// URLs.

Fixed by using file.url(db) which returns the proper absolute file:// URL.

This fixes goto-definition not working when clicking on root path segments
that resolve to file modules (e.g., clicking 'stuff' in 'use stuff::calculations').

crates/language-server/src/util.rs:66
- Fix UsePathView to start resolution from the use item's scope instead of
  its parent scope. This fixes resolution of file module references in use paths.

- Add UsePathView::target_at for cursor-aware segment resolution in use paths

- Update ReferenceView::target_at to delegate to UsePathView::target_at for use paths

- Improve hover display for ambiguous references:
  - Change header from 'Ambiguous reference' to 'Multiple definitions'
  - Remove candidate numbering for cleaner display
  - Add horizontal rules between candidates
  - Include docstrings for all candidates

- Remove duplicate goto_path_segments test (covered by goto_ambiguous)

crates/hir/src/core/semantic/reference/mod.rs:372
crates/hir/src/core/semantic/reference/mod.rs:420
crates/hir/src/core/semantic/reference/mod.rs:498
crates/language-server/src/functionality/hover.rs:39
Copy link
Collaborator

@sbillig sbillig left a comment

Choose a reason for hiding this comment

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

👍

@sbillig sbillig merged commit c25eae2 into argotorg:master Dec 8, 2025
5 checks passed
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.

2 participants