feat: LSP references, deep symbols, smart completions, user hover#7
Conversation
…tions, and user-defined hover - Module completions now filter to the specific module typed before the dot - Hover shows signatures for user-defined functions, variables, structs, types, and interfaces - Go-to-definition finds params, locals, and nested symbols inside function bodies - Add textDocument/references support with word-boundary matching - Return null hover for unknown symbols instead of generic fallback - 8 new tests (14 total LSP tests)
![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: 55b8d95f7c
ℹ️ 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".
| } | ||
| // Recurse into body | ||
| for inner in body { | ||
| collect_symbols_from_stmt(inner, line, symbols); |
There was a problem hiding this comment.
This recursion reuses the parent line for every nested statement, so locals/loop vars/catch vars are recorded as if they were declared on the enclosing function line. get_definition then builds ranges from that incorrect line, which can jump to the function header (or column 0 if the name is not on that line) instead of the actual declaration whenever the local is declared on a later line.
Useful? React with 👍 / 👎.
| return serde_json::Value::Null; | ||
| // Try deep symbol search first (includes params, locals inside functions) | ||
| let deep = collect_all_symbols(&text); | ||
| if let Some(symbol) = deep.iter().find(|s| s.name == word) { |
There was a problem hiding this comment.
Definition lookup now picks the first symbol with a matching name in the flattened deep-symbol list, without considering lexical scope or cursor location. In files with repeated local names (for example i in multiple functions), go-to-definition on a later occurrence will incorrectly jump to an unrelated earlier declaration, making navigation unreliable in common code patterns.
Useful? React with 👍 / 👎.
humancto
left a comment
humancto
left a comment
There was a problem hiding this comment.
Review: Phase 1 LSP Improvements
Nice batch of DX improvements — context-aware completions, user-defined symbol hover, deep go-to-definition, and references are all solid additions. The test coverage is good (8 new tests) and CI is green on Linux/macOS/Clippy/Format/Integration. The hover output format (showing full signatures with types) is particularly well done.
Should fix before merge
-
ImplBlockmissing from recursive traversal — Bothcollect_symbols_from_stmtandhover_from_stmtdon't handleStmt::ImplBlock, so methods defined inimpl/giveblocks are invisible to deep symbols, go-to-definition, and hover. This is the only Stmt variant with a body that's not covered. -
Word boundary checks need explicit parentheses — The
before_ok/after_okexpressions inget_referencesrely on implicit&&>||precedence. While technically correct, this is a maintenance hazard. Add parens or extract a helper. -
Missing CHANGELOG.md entry — Required by project guidelines for user-facing changes.
Worth noting (non-blocking)
- Inner symbol line numbers are approximate — Locals inside function bodies inherit the parent function's line number (AST limitation:
body: Vec<Stmt>isn'tSpanned). Go-to-definition for inner locals will jump to the function line, not the declaration. Fine for now, but worth a doc comment. - References is single-file — LSP spec expects workspace-wide search. Acceptable for v1.
context.includeDeclarationignored — Declaration is always included. Minor spec gap.- Security Audit CI failure appears pre-existing (not caused by this PR's changes).
Generated by Claude Code
| let before_ok = abs_col == 0 | ||
| || !line_content.as_bytes()[abs_col - 1].is_ascii_alphanumeric() | ||
| && line_content.as_bytes()[abs_col - 1] != b'_'; | ||
| let after_pos = abs_col + word.len(); | ||
| let after_ok = after_pos >= line_content.len() | ||
| || !line_content.as_bytes()[after_pos].is_ascii_alphanumeric() | ||
| && line_content.as_bytes()[after_pos] != b'_'; |
There was a problem hiding this comment.
Bug (operator precedence): These boundary checks rely on && binding tighter than || in Rust to be correct. While the logic does evaluate correctly today, it's a readability trap — most readers will misread it as (abs_col == 0 || !is_alpha) && != '_', which would be wrong.\n\nPlease add explicit parentheses to make the intent unambiguous:\n\nrust\nlet before_ok = abs_col == 0\n || (!line_content.as_bytes()[abs_col - 1].is_ascii_alphanumeric()\n && line_content.as_bytes()[abs_col - 1] != b'_');\nlet after_ok = after_pos >= line_content.len()\n || (!line_content.as_bytes()[after_pos].is_ascii_alphanumeric()\n && line_content.as_bytes()[after_pos] != b'_');\n\n\nOr better yet, extract a small helper like fn is_word_char(b: u8) -> bool { b.is_ascii_alphanumeric() || b == b'_' } and simplify to:\nrust\nlet before_ok = abs_col == 0 || !is_word_char(line_content.as_bytes()[abs_col - 1]);\nlet after_ok = after_pos >= line_content.len() || !is_word_char(line_content.as_bytes()[after_pos]);\n
Generated by Claude Code
| fn collect_symbols_from_stmt(stmt: &Stmt, line: usize, symbols: &mut Vec<DocumentSymbolInfo>) { | ||
| match stmt { | ||
| Stmt::FnDef { | ||
| name, params, body, .. | ||
| } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 12, | ||
| line, | ||
| }); | ||
| // Add parameters as variable symbols | ||
| for param in params { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: param.name.clone(), | ||
| kind: 13, | ||
| line, | ||
| }); | ||
| } | ||
| // Recurse into body | ||
| for inner in body { | ||
| collect_symbols_from_stmt(inner, line, symbols); | ||
| } | ||
| } | ||
| Stmt::Let { name, .. } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 13, | ||
| line, | ||
| }); | ||
| } | ||
| Stmt::StructDef { name, .. } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 23, | ||
| line, | ||
| }); | ||
| } | ||
| Stmt::TypeDef { name, .. } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 10, | ||
| line, | ||
| }); | ||
| } | ||
| Stmt::InterfaceDef { name, .. } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 11, | ||
| line, | ||
| }); | ||
| } | ||
| Stmt::PromptDef { name, .. } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 12, | ||
| line, | ||
| }); | ||
| } | ||
| Stmt::AgentDef { name, .. } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 5, | ||
| line, | ||
| }); | ||
| } | ||
| Stmt::For { | ||
| var, var2, body, .. | ||
| } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: var.clone(), | ||
| kind: 13, | ||
| line, | ||
| }); | ||
| if let Some(v2) = var2 { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: v2.clone(), | ||
| kind: 13, | ||
| line, | ||
| }); | ||
| } | ||
| for s in body { | ||
| collect_symbols_from_stmt(s, line, symbols); | ||
| } | ||
| } | ||
| Stmt::TryCatch { | ||
| try_body, | ||
| catch_var, | ||
| catch_body, | ||
| } => { | ||
| for s in try_body { | ||
| collect_symbols_from_stmt(s, line, symbols); | ||
| } | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: catch_var.clone(), | ||
| kind: 13, | ||
| line, | ||
| }); | ||
| for s in catch_body { | ||
| collect_symbols_from_stmt(s, line, symbols); | ||
| } | ||
| } | ||
| Stmt::If { | ||
| then_body, | ||
| else_body, | ||
| .. | ||
| } => { | ||
| for s in then_body { | ||
| collect_symbols_from_stmt(s, line, symbols); | ||
| } | ||
| if let Some(eb) = else_body { | ||
| for s in eb { | ||
| collect_symbols_from_stmt(s, line, symbols); | ||
| } | ||
| } | ||
| } | ||
| Stmt::While { body, .. } | ||
| | Stmt::Loop { body, .. } | ||
| | Stmt::Spawn { body } | ||
| | Stmt::SafeBlock { body } | ||
| | Stmt::TimeoutBlock { body, .. } | ||
| | Stmt::RetryBlock { body, .. } | ||
| | Stmt::ScheduleBlock { body, .. } | ||
| | Stmt::WatchBlock { body, .. } => { | ||
| for s in body { | ||
| collect_symbols_from_stmt(s, line, symbols); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| fn get_hover(uri: &str, line: usize, character: usize) -> serde_json::Value { |
There was a problem hiding this comment.
Missing Stmt::ImplBlock traversal: collect_symbols_from_stmt doesn't recurse into Stmt::ImplBlock { methods, .. }. This means methods defined via impl/give blocks won't be found by deep go-to-definition or references. Since ImplBlock.methods is a Vec<Stmt> (containing FnDef entries), it should be straightforward to add:\n\nrust\nStmt::ImplBlock { methods, .. } => {\n for m in methods {\n collect_symbols_from_stmt(m, line, symbols);\n }\n}\n\n\nSame gap exists in hover_from_stmt below.
Generated by Claude Code
| } => { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: name.clone(), | ||
| kind: 12, | ||
| line, | ||
| }); | ||
| // Add parameters as variable symbols | ||
| for param in params { | ||
| symbols.push(DocumentSymbolInfo { | ||
| name: param.name.clone(), | ||
| kind: 13, | ||
| line, | ||
| }); | ||
| } | ||
| // Recurse into body | ||
| for inner in body { | ||
| collect_symbols_from_stmt(inner, line, symbols); | ||
| } |
There was a problem hiding this comment.
Inaccurate line numbers for inner symbols: When recursing into function bodies, the parent's line is propagated to all inner statements (params, locals, nested lets). Since body: Vec<Stmt> doesn't carry Spanned wrappers, there's no line info for inner stmts.\n\nThis means go-to-definition for a local variable inside a function will jump to the function definition line rather than the variable's actual declaration line. symbol_range() then does lines.get(line).find(name) on the function's line, which will fail for inner locals (returning column 0).\n\nI understand this is an AST limitation (body stmts aren't Spanned), but it's worth calling out. Consider at minimum adding a doc comment noting this known limitation, so future contributors don't spend time debugging it.
Generated by Claude Code
| } | ||
|
|
||
| /// Find all references to a symbol in the document (text-based search with word boundaries). | ||
| fn get_references(uri: &str, line: usize, character: usize) -> Vec<serde_json::Value> { |
There was a problem hiding this comment.
Nit (spec compliance): Two minor LSP spec deviations to be aware of:\n\n1. context.includeDeclaration (from ReferenceParams) is not checked — the declaration is always included. Most clients default to true so this is fine in practice, but it's a gap.\n2. This is single-file only. The LSP spec expects textDocument/references to search across the workspace. Totally acceptable for a v1, but worth noting in a comment or the PR description so it doesn't get forgotten.
Generated by Claude Code
There was a problem hiding this comment.
Missing CHANGELOG entry: Per the project's CLAUDE.md guidelines: "Every PR that ships user-facing changes must have an entry under [Unreleased]." This PR adds 5 user-facing LSP features and should have a corresponding CHANGELOG.md update under ### Added.
Generated by Claude Code
…ity, CHANGELOG - Add ImplBlock handling to collect_symbols_from_stmt and hover_from_stmt so methods in impl/give blocks appear in go-to-def and hover - Extract is_ident_byte helper in get_references to clarify operator precedence in word boundary checks - Add [Unreleased] CHANGELOG entries for LSP improvements
humancto
left a comment
humancto
left a comment
There was a problem hiding this comment.
Re-review: All feedback addressed ✓
All three items from my previous review are resolved in 8216c28:
- Word boundary clarity — Extracted
is_ident_byteclosure, boundary checks are now clean one-liners. Much easier to read. ImplBlocktraversal — Added to bothcollect_symbols_from_stmtandhover_from_stmt. Methods inimpl/giveblocks are now reachable.- CHANGELOG entry — Added under
[Unreleased] > ### Addedwith all four user-facing features listed.
CI status
- Linux, macOS, Clippy, Format, Integration Tests, Backend Parity: all green
- Windows: failing, but this PR only touches
src/lsp/mod.rs(unit tests use in-memory doc store, no platform-specific paths) andCHANGELOG.md. This was already in-progress/unresolved during my first review — likely pre-existing or flaky. Worth investigating separately but should not block this PR. - Security Audit: failing (pre-existing, same as first review)
LGTM. Ship it.
Generated by Claude Code
1 participant
Summary
Phase 1 (Developer Experience) — first batch of LSP improvements.
math.now only showsmathmembers instead of all 200+ members from every modulefn greet(name: String, age: Int) -> String), variables show mutability/type, structs show fields, types show variants, interfaces show methodstextDocument/references: find all references to any identifier with proper word-boundary checkingnullinstead of generic fallback (correct LSP behavior)Test plan
forge run examples/hello.fgpassesforge run examples/functional.fgpasses.fgfile in VS Code with Forge LSP, verify hover/completions/references work