lsp: make goto/hover follow the checker resolution instead of name-scans - #271
Merged
Merged
Conversation
…'t show
Completions could insert code the type checker rejects, or pop up where
no code belongs. Four fixes, unified under one invariant: accepting any
offered completion must never insert code that fails to compile.
- Plain `use lib;` binds only the namespace, so `push_imported` now offers
its items in qualified `lib::item` form (the label the LSP layer inserts
verbatim) plus the bare namespace name — never bare `exported`, which the
checker rejects as an undefined function. A braced `use lib::arith::{add};`
binds only the braced names, so exactly those are offered bare (an item
naming no public def in the target is dropped), not every public def.
- New `<module>::` completion context: after a plain-import namespace
qualifier, that module's public defs are offered by their bare name — the
one position where bare is what compiles. Resolution trusts only plain
(namespace-binding) imports, so an item import — which binds names bare and
no namespace — never anchors a `::` qualifier, and a `::` position never
falls back to the keyword/local list.
- Member completions after `.` on a struct defined in another module drop
private methods (the checker forbids `receiver.private_method()` across
modules); a same-file receiver keeps its private methods, callable there.
- Completions are suppressed inside comments and string literals, decided by
the lexer's token spans (via inference-parser's `tokenize`) so the quote
boundaries are exact, rather than by ad-hoc text scanning.
Adds a comprehensive unit-test matrix (plain/braced imports, `::` context
incl. nested modules and item-import exclusion, private-vs-public members
local and cross-module, comment/string suppression at quote boundaries) and
one wire-level e2e test for the `::` trigger. The empirically-verified
compile semantics behind each case are encoded in the tests.
Fixes #246
Positions an editor routinely queries returned nothing. Five gaps, each now resolved, with a comprehensive unit/integration/e2e test matrix. - Caret at an identifier's exclusive end (where a double-click or a just-finished keystroke leaves it) missed: hit_test covers start <= offset < end, so the end position lands on the enclosing call or statement, and goto/hover passed the raw offset with no fallback. Extract the identifier-biased one-byte-back fallback that completions already had into ide-db as the shared `enclosing_hit`, and drive goto, hover, and the completion locals through it. The shared version prefers an identifier when the direct hit is not one, which the old completions copy did not need but goto/hover do; it still refuses to pull a caret past a `}` back into the closing definition. - `use` directives were not hit-testable: the walk started only from a file's defs and never its directives. Walk the path segments and braced item imports (arena-backed idents with real locations) as ancestor-less identifier hits, and resolve them: a segment to the module file it names (`lib`, then `lib::geom`), a braced item to its public definition in the target module. A `from`-clause external module reference names no source file and is intentionally not resolved. - `Def::Function::type_params` were dropped from `def_children`, so a declared type parameter (`T'`) fell to the whole function. Descend into them and resolve a type-parameter name to itself under goto/hover. - Enum variant *declarations* were unreachable: goto_in_def and hover's ident_in_def covered function args and struct fields but not variants. A variant declaration name now resolves to itself like every other declaration. - Function-local `const` references were invisible. Rather than index them globally (which would ignore scope), extend the local-resolution path: `in_scope_locals` and `resolve_local` now see `Stmt::ConstDef`, gated on the same statement-order name-end bound as a `let`. This matches the type checker, which registers a local const in statement order, so a const used before its declaration or from another function does not resolve. Fixes #244
Goto-definition and hover resolved several identifiers by a syntactic by-name scan that could contradict what the type checker already resolved. Six cases produced wrong or missing navigation on programs that compile cleanly: - A free-function call over a same-named struct method landed on the method (and hovered its `fn get(self)` signature). The checker records `receiver_struct=None` for a free call, so the by-name search now skips struct methods via a new `find_free_def_by_name`, applied in both `goto_call` and `callee_signature`. - A bare imported value resolved to the first module that happened to export the name, ignoring `UseDirective::braced`/`imported_types`. It now resolves only through a braced import that names it; a bare name under a plain `use m;` (a type error) resolves to nothing. - A constant imported through a `pub use` re-export chain returned None. `goto_value_def` now follows re-export directives to the defining file, with a visited-set guard against re-export cycles. - Hover on the leaf of a `::`-qualified type ignored the qualifier and showed a local same-named type. It now resolves the qualified path the way goto does. - Function types rendered as the checker-internal `Function<2, i32>` carrier. The checker now builds a source-like `fn(...) -> i32` carrier when constructing the type's `TypeInfo` (params spelled with their lowercase source names). The parser drops `fn(...)` parameter types by a pinned AST-parity quirk, so only the return type survives to real parsed source; the params are threaded whenever they are present. - Goto on a local-binding use reported `full_range == focus_range` (the ident), while the declaration site reported the whole statement. `resolve_local` now returns the declaration's full range, so both agree. Fixes #245
Only offer the bare namespace name of a plain `use` after the module resolves, so `use nonexistent;` no longer suggests an unresolvable name that the checker would reject. Drop the redundant `start < offset` match guards in `offset_in_comment_or_string`; the early break on `start >= offset` already establishes that invariant for every arm.
…it-test-coverage-gaps
…to-hover-checker-resolution
The stacked parents (#262 completions, #267 hit-test gaps) landed on main as squash commits, so the shared content arrived under different SHAs and had to be reconciled by hand. Conflicts resolved: - CHANGELOG.md: union of both Unreleased sections; the #245 entry keeps its place after the #244 goto/hover group, and [#245] joins the link refs. - ide/ide/src/{goto_definition,hover}.rs: imports take the branch supersets (Visibility, TypeId/TypeNode, find_free_def_by_name, resolve_qualified_module); the #245 test blocks append after the #244 ones already on main. - goto_definition.rs `resolve_local`: git textually kept both sides' ConstDef match arm. The branch widened the return to (Location, IdentId), so main's arm returning a bare IdentId is stale and was dropped. Verified: the merged tree's delta against main is exactly the change set of 64676e9 (the branch's only unique commit); cargo test 4411 passed / 0 failed, clippy clean.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
0xGeorgii
added a commit
that referenced
this pull request
Jul 19, 2026
main carries this branch's first two commits as the #258 squash (3e5fccb = 3babb3f + 5f28be0), so the shared panic-boundary work arrived under different SHAs; main has also moved on considerably since (#262, #266, #267, #271). This branch's remaining unique work is #268 (protocol polish) and #273 (named-constant array size). Conflicts resolved (26 across four files): - apps/lsp/src/server.rs (13): taken wholesale from this branch. main's copy is byte-identical to this branch at 5f28be0, so no main-side change exists to preserve and this branch's tip is a strict descendant. - apps/lsp/tests/e2e.rs (10): taken from this branch. Every conflict was the same pair — main still triggers the panic-boundary tests with the real named-constant-array-size `todo!` (PANIC_SOURCE), which #273 turns into an ordinary diagnostic. This branch's versions drive the deliberate debug-only seam (panic_fixture/PANIC_DOC_SOURCE/PANIC_ENV) instead. Keeping main's would have left the tests waiting on a panic that no longer happens. main's own additions to this file (#262, #267) auto-merged and were verified present by name. - ide/base-db/src/line_index.rs (1): a genuine union — main's #266 BOM test and this branch's two #268 offset_clamped tests were appended at the same spot. Both kept. - CHANGELOG.md (2): union of both Unreleased sections. Both sides carry the identical #241 entry; it is kept once, after main's list, followed by this branch's #249 and #240 entries. [#240]/[#249] join the link refs, which already had [#241]. Verified: the merged tree against main adds exactly the lines of this branch's unique work (5f28be0..3898335) across the same thirteen files, and removes nothing from main beyond what that work legitimately rewrites — checked in both directions at line level. cargo test 4506 passed / 0 failed, including both sides' tests (main's leading_bom, this branch's offset_clamped, the reseated panic-boundary tests, and #273's over-the-wire diagnostic test). clippy clean; `cargo build --release --tests -p inference-lsp` warning-free, which is what the debug_assertions gating on the seam constants exists to keep.
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.
Fixes #245. Stacked on #267 (
244-bug-fix-hit-test-coverage-gaps) — merge that first.Goto/hover no longer contradict the type checker. All six items; every positive test was empirically verified to FAIL on the base branch pre-fix (via a detached base worktree running the same public-API tests).
Changes
receiver_struct=None(explicitly not a method), goto and hover'scallee_signaturerestrict the by-name search to non-method defs (newfind_free_def_by_name: top-level + spec-nested, skipping struct methods).goto_value_defconsults only braced imports whoseimported_typesname the bare value — a plainuse m;no longer resolves bare names (matching the checker, which rejects them).pub usere-export chains: new recursiveresolve_exported_valuefollows re-exports to the defining file, with a visited-set cycle guard.lib::Thover resolves through the qualifier the way goto does (newtype_ident_signature/qualified_member_signature).TypeInfoKind::Functioncarrier is now a source-likefn(...) -> retinstead of the internalFunction<2, i32>. Caveat, documented in the code: the parser deliberately drops writtenfn(...)parameter types when lowering (pinned AST-parity quirk), so hovers on real parsed source showfn() -> i32; actual params are threaded whenever present. Fixing the parser to preserve them would break the AST-parity contract — out of scope here.full_rangeconsistency: goto on a local-binding use now returns the whole declaration statement asfull_rangewith the ident as focus, matching the declaration-site path.Testing
10 new ide-level probe tests mirroring the issue repros + 2 updated checker unit tests pinning the new carrier (
fn(i32, bool) -> string,fn() -> unit).inference-ide141/141,inference-tests2299/2299, full default-workspacecargo test4411 passed / 0 failed; clippy clean.Confidence Score: 4/5
Safe to merge with one focused fix:
resolve_exported_valueusesfind_def_by_name(which walks struct methods) instead offind_free_def_by_name, leaving a narrow goto mis-navigation for modules that export both a struct method and a top-level value with the same name.resolve_exported_value— the new function that followspub usere-export chains — callsfind_def_by_name, which includes struct methods in the pre-order flatten. If an exported module has a public struct method and a public top-level function sharing a name (exactly the scenario the rest of this PR corrects), goto can still land on the method instead of the function when the value was imported via a re-export chain. The rest of the PR is clean and well-tested.ide/ide/src/goto_definition.rs— specificallyresolve_exported_valueat line 262, wherefind_def_by_nameshould befind_free_def_by_name.Important Files Changed
find_free_def_by_namefor calls, selective-import filtering, andresolve_exported_valuefor re-export chains;resolve_exported_valuestill callsfind_def_by_name(which includes struct methods) instead offind_free_def_by_name, leaving the same-named method bug class it was meant to fix.find_free_def_by_namefor free-call signatures and addstype_ident_signature/qualified_member_signaturefor qualified-type hover; logic mirrors goto's qualifier resolution cleanly.find_free_def_by_nameand its helpernon_method_defs/collect_non_method_def; implementation is correct — pushes top-level and spec-nested defs while skipping struct methods.Function<N, Ret>tofn(params) -> retusing a newsource_like_spellinghelper; correctly handles nested function types and builtin/custom type spellings.fn() -> unit/fn(i32, bool) -> stringcarrier format.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[goto_definition / hover at offset] --> B{node kind?} B -->|Expr: Identifier call| C[goto_call / callee_signature] B -->|Expr: Identifier value| D[resolve_local?] B -->|Type node| E[type_ident_signature] B -->|Use directive| F[goto_in_directive] C --> C1{receiver_struct?} C1 -->|Some struct| C2[find_def_by_name to find_method] C1 -->|None free call| C3[find_free_def_by_name OK] D -->|found local| D1[nav_at_ident with full stmt range] D -->|not local| G[goto_value_def] G --> G1[find_def_by_name in entry file] G1 -->|not found| G2{braced import names it?} G2 -->|yes| G3[resolve_exported_value] G2 -->|no plain use| G4[None matches checker] G3 --> G5[find_def_by_name WARN should be find_free_def_by_name] G5 -->|not found or pub use| G6[follow pub use re-export chain with cycle guard] E --> E1{qualified lib T?} E1 -->|yes| E2[qualified_member_signature via resolve_qualified_module] E1 -->|bare type| E3[type_name_signature] F --> F1[find_def_by_name direct WARN no re-export follow]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[goto_definition / hover at offset] --> B{node kind?} B -->|Expr: Identifier call| C[goto_call / callee_signature] B -->|Expr: Identifier value| D[resolve_local?] B -->|Type node| E[type_ident_signature] B -->|Use directive| F[goto_in_directive] C --> C1{receiver_struct?} C1 -->|Some struct| C2[find_def_by_name to find_method] C1 -->|None free call| C3[find_free_def_by_name OK] D -->|found local| D1[nav_at_ident with full stmt range] D -->|not local| G[goto_value_def] G --> G1[find_def_by_name in entry file] G1 -->|not found| G2{braced import names it?} G2 -->|yes| G3[resolve_exported_value] G2 -->|no plain use| G4[None matches checker] G3 --> G5[find_def_by_name WARN should be find_free_def_by_name] G5 -->|not found or pub use| G6[follow pub use re-export chain with cycle guard] E --> E1{qualified lib T?} E1 -->|yes| E2[qualified_member_signature via resolve_qualified_module] E1 -->|bare type| E3[type_name_signature] F --> F1[find_def_by_name direct WARN no re-export follow]Comments Outside Diff (3)
ide/ide/src/goto_definition.rs, line 472-496 (link)goto_in_directivecallsfind_def_by_name(arena, sfid, name)directly on the immediate target module. If that module only re-exports the name (e.g.midhaspub use lib::{MAX}but no ownMAXdefinition),find_def_by_namereturnsNoneand the directive-level navigation silently fails. The newresolve_exported_valueused ingoto_value_defwould handle this correctly, but it isn't wired in here, creating an asymmetry: clickingMAXinuse mid::{MAX}at the use-site now resolves all the way tolib, while clickingMAXin the sameusedirective text does not.ide/ide/src/goto_definition.rs, line 225-228 (link)resolve_exported_valueusesfind_def_by_name, which includes struct methodsfind_def_by_namecallsfile_defsunder the hood, which is a pre-order flatten that covers struct methods. If an exported module has a struct method namedNAMEthat appears before a public top-level constant/function namedNAMEin source order,find_def_by_namereturns the method, and—sincedef_is_publiccan be true for a method—goto navigates to the wrong definition. This is exactly the same bug class that motivated introducingfind_free_def_by_nameingoto_call.resolve_exported_valueis a new function added in this PR and should usefind_free_def_by_namefor consistency: it is only ever called to resolve bare exported values (constants, functions, types), never struct methods.ide/ide/src/goto_definition.rs, line 262-266 (link)find_def_by_nameincludes struct methods viafile_defs. When a module has both a public struct method namedNAMEand a public top-level constant/function namedNAME, and the method appears earlier in source order,resolve_exported_valuereturns the method instead of the top-level def—navigating to the wrong place.find_free_def_by_namewas introduced in this PR for exactly this reason and should be used here too.Reviews (3): Last reviewed commit: "Merge branch 'main' into 245-bug-fix-got..." | Re-trigger Greptile