feat(lsp): hover, document symbols, go-to-definition and smarter completion 🔍 - #164
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45b0434c06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
timfennis
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06022f09af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…completion 🔍 Rework the document model to store the analysed AST and its AnalysisResult side tables (both Send + Sync) as the source of truth, replacing the pre-flattened offset-keyed type maps. This powers the new position-based features and keeps the flat completion-support maps only as a documented resilience cache for dot-completion mid-edit. Features: - Hover: inferred type at the cursor; signature + docs for built-in functions. - Document symbols: outline of top-level/nested functions and variables. - Go-to-definition: lexical scope resolution over the AST (local slot numbers are stack-relative and reset per function, so they can't key a global map). - Completion: in-scope locals and language keywords in general completion. Performance: - Snapshot native-function metadata once at startup; completion and hover no longer rebuild an interpreter per request. - LineIndex gives O(log n) offset<->position instead of rescanning from 0; completion holds the read lock instead of cloning document state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses PR review (Codex + Tim). All issues shared one cause: the LSP re-derived name resolution by name only and ignored scope/binding info. - New scope_resolve.rs: one scope-aware declaration walk with correct visibility (enclosing scope + declared-before-use; functions hoisted). Shared by go-to-definition and completion. - go-to-def: a use before a later inner shadow now resolves to the outer binding (was: inner scope won by size before the visibility check). - completion: in-scope locals only — a local from one function is no longer suggested inside another. - hover: only show a built-in's signature when the identifier resolved to a global; a local shadowing a built-in (e.g. `let len = 1; len`) shows its type. - Use AHashMap throughout ndc_lsp, per project convention. Known gap: go-to-def is still name-only and does not disambiguate function overloads. The follow-up that exposes the analyser's resolution closes this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Position.character is UTF-16 by default and the server advertises no other encoding, but LineIndex counted Unicode scalar values — so positions after an astral character (e.g. an emoji) mapped to the wrong byte offset for hover, go-to-definition, and completion, and ranges had the symmetric error. Count UTF-16 units in both directions; a column splitting a surrogate pair clamps to the character's start. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
06022f0 to
e5a491a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5a491ad66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When an edit leaves the buffer unparsable, update_source advanced source and line_index but the cached AST/analysis stayed from the last good parse, so hover, go-to-definition, document symbols, and inlay hints mapped old spans onto the edited text (wrong ranges / symbols from deleted code). Track whether the AST matches the current source (analysis_matches_source); these features now no-op until the buffer parses again. Completion still works mid-edit via the name/offset caches, as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4bccb69cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
local_completions walks the cached AST's declaration spans against the current source, so mid-edit (last parse failed) it could offer locals from stale scopes. Skip the AST walk when analysis_matches_source is false; keywords and the resilient dot-completion caches are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d9317005
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex flagged a race where a slow `validate` could overwrite newer source. Rather than patch that one spot, harden the whole staleness/concurrency class: - Tag each document with the client's monotonic `version`. `validate` runs analysis off the lock and commits/publishes only if the stored version still matches — a stale run no longer rolls the buffer back. - Commit analysis in place (`set_analysis`); never rewrite source/line_index that validate didn't produce. - Publish diagnostics with the document version so clients discard superseded ones. - `did_change`: with full-document sync, process only the final change. - Add `did_close`: drop cached state (no unbounded growth) and clear diagnostics. - Document the staleness model: AST-backed features gate on `analysis_matches_source`; the completion caches are intentionally resilient. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afbcdc5789
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lish 🔒 Two review findings: - Lexical visibility: a `let`/for-loop binding was visible inside its own initializer, so `let x = x` (and `for x in x`) could resolve the RHS/sequence to the binding being declared. Bindings now become visible from the END of their initializer (RHS / sequence), matching the analyser, which resolves the initializer before creating the binding. Later iterations, guards, and the body still see the binding. - Diagnostics race: a `validate` publish could slip past a concurrent `did_close` and leave stale diagnostics on a closed file. `validate` now holds the document lock across both the commit and the publish, and `did_close` does the same, so the two fully serialize. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89cf627c4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A reassignment target (`x` in `x = 2`) is an Lvalue, not an expression node, so node_at_offset returned the enclosing assignment and go-to-def bailed. Resolve the identifier under the cursor whether it's an expression use or an lvalue identifier; reassignment targets now jump to the declaration. Declaration, parameter, and loop-variable lvalues are matched too but resolve to nothing, since they aren't visible to themselves. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
andy-cpp/ndc_lsp/src/features/completion.rs
Lines 46 to 48 in 9720244
Scope the fallback used for dot completion
When the user types a new ident. at a position that was not present in the last successful AST, expression_types has no entry and this falls back to variable_types by name only. In a file with a top-level x: String and a later inner x: Int, typing a new top-level x. after that inner declaration can use the inner/out-of-scope type because the flat map was overwritten, so completion suggests methods for the wrong receiver. The fallback should resolve the visible declaration at the cursor instead of using an unscoped name lookup.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let best = decls | ||
| .iter() | ||
| .filter(|d| d.name == name && is_visible(d, offset)) |
There was a problem hiding this comment.
When multiple visible functions share a name but have different arities, this name-only filter can return the wrong declaration: for example, with fn foo(a) { ... } followed by fn foo(a, b) { ... }, go-to-definition from foo(1) will reduce to the later two-argument declaration even though the analyser resolves the one-argument overload. This makes the new definition feature inaccurate for valid overloaded functions; it needs to use the resolved binding/call arity rather than only d.name == name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is the known limitation the function doc already calls out: the interim resolver is name-only and can't disambiguate overloads. It's not fixable here — the analyser selects the right overload but records it as a stack slot, which isn't a stable identity and doesn't map back to a declaration span. That's exactly what the approved follow-up (parser name_span + analyser DefId side-table exposing resolved_uses) fixes, making go-to-def and hover overload- and shadowing-correct by construction. Deferring this to that PR rather than re-implementing overload resolution in code the follow-up will delete. All the soundness findings from this review round are fixed; merging on that basis.
1 participant
Context
The
ndc_lspserver offered only inlay hints, dot-completion, and diagnostics. This adds the commonly-expected LSP features and, along the way, cleans up the document model and removes a per-request hot path. Prompted by a request to improve the LSP across features, architecture, and performance.Changes
Architecture
DocumentStatenow stores the analysed AST +AnalysisResultside tables (verifiedSend + Sync) as the source of truth for position-based features, instead of only pre-flattened offset-keyed maps. The flatvariable_types/expression_typesmaps are kept as a documented resilience cache so dot-completion keeps working while the buffer is mid-edit and doesn't parse.node_at_offsetresolver to the AST visitor.Performance
FunctionInfo); completion and hover no longer rebuild an interpreter per request.LineIndexgives O(log n) offset↔position conversion instead of rescanning from byte 0; completion holds the read lock instead of cloning document state.Features
Docs
Note for reviewers
The original plan was to add a
HashMap<ResolvedVar, Span>to the analyser for go-to-definition. While implementing I found thatResolvedVar::Localslots are stack-relative and reset to 0 per function (new_function_scopesetsbase_offset: 0), so such a map collides across functions and resolves to the wrong declaration. Go-to-definition is therefore implemented via lexical scope resolution over the AST — correct, and contained entirely withinndc_lspwith no analyser change.🤖