Repository navigation
feat(server): go-to-definition on includes and module names - #481
Conversation
An open file is split by the PCH: the preamble's include directives are invisible to the worker AST. Their links now travel structurally over the binary protocol (worker::FileLink — protocol::DocumentLink carries optional/LSPAny fields the bincode codec cannot represent) and are cached in PCHState, replacing the pre-serialized JSON blob and the string-surgery merge in the documentLink handler. The DocumentLink query moved off the RawValue QueryKind pass-through onto a dedicated typed RPC. Definition resolution order: preamble include lines answer from the cached links; import/module names lex the cursor line and resolve through the master's module map (works in any region); everything else falls through to the index and the worker, whose GoToDefinition stub now serves main-region include directives from unit directives.
__has_include arguments navigate like includes; module tests cover the implementation-unit declaration, dotted names and the keyword-cursor negative; header method grouping and the ASCII-offset assumption note.
clang's raw lexer asserts a NUL-terminated buffer; lexing a StringRef view into the session text aborted every definition request in Debug builds. Also fix the include-definition unit fixture and align the worker GoToDefinition missing-document result with its query siblings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds include- and module-based go-to-definition support, changes document links to structured preamble link data, and updates the worker, compiler, server, tests, and navigation docs around those flows. ChangesInclude/module navigation and document-link pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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: ac649c4a5d
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/compiler/compiler.cpp (1)
763-768: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPersist
preamble_linkswith PCH cache entries.Workspace::save_cache()/load_cache()currently round-trip onlypath,key,bound, anddepsforPCHState, so a restart drops preamble include links on cache hits until the PCH is rebuilt.🤖 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 `@src/server/compiler/compiler.cpp` around lines 763 - 768, Persist the missing PCH cache field by updating the cache save/load path for PCHState so preamble_links is serialized and restored alongside path, key, bound, and deps. Locate the PCH cache handling in Workspace::save_cache(), Workspace::load_cache(), and the compiler code that assigns st.preamble_links, and make sure cache entries round-trip those links across restarts so cache hits keep the preamble include links without requiring a rebuild.
🧹 Nitpick comments (1)
src/feature/document_links.cpp (1)
25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate directive-scanning logic between
add_linkandtry_directive.Both lambdas independently repeat the same sequence: the offset must fall on the argument of an
#includeor __has_include, by decomposing the location, checking it belongs to the interested file within content bounds, and locating the directive argument range before doing something different with it (building a link vs. checking containment). Extracting a shared helper (e.g.resolve_directive_argument(unit, content, lang_opts, loc) -> optional<LocalSourceRange>) used by bothdocument_linksandinclude_definitionwould remove this duplication and keep future directive-handling tweaks (e.g. adding embeds to go-to-definition) in one place.♻️ Sketch of shared helper
+namespace { +std::optional<LocalSourceRange> resolve_directive_argument(CompilationUnitRef unit, + llvm::StringRef content, + const clang::LangOptions* lang_opts, + clang::SourceLocation loc) { + auto [fid, offset] = unit.decompose_location(loc); + if(fid != unit.interested_file() || offset >= content.size()) + return std::nullopt; + return find_directive_argument(content, offset, lang_opts); +} +} // namespace + auto document_links(CompilationUnitRef unit, PositionEncoding encoding) -> std::vector<protocol::DocumentLink> { ... auto add_link = [&](clang::SourceLocation loc, llvm::StringRef target) { - auto [fid, offset] = unit.decompose_location(loc); - if(fid != interested || offset >= content.size()) - return; - auto range = find_directive_argument(content, offset, lang_opts); + auto range = resolve_directive_argument(unit, content, lang_opts, loc); if(!range) return; ...Also applies to: 67-105
🤖 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 `@src/feature/document_links.cpp` around lines 25 - 38, The directive-argument lookup is duplicated in add_link and try_directive, so extract the shared “resolve directive argument” flow into a helper that takes unit, content, lang_opts, and loc and returns the directive argument range when the location is on an `#include` or __has_include argument. Update both document_links and include_definition to call this helper and keep only their specific follow-up logic (building protocol::DocumentLink vs. checking containment/definition handling), so future directive tweaks stay in one place.
🤖 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.
Inline comments:
In `@tests/unit/server/stateful_worker_tests.cpp`:
- Around line 200-206: The inline comment in the send_request test is stale and
contradicts the current assertion in stateful_worker_tests::peer->send_request
handling. Update the comment near the result.value().data check so it reflects
the actual expected "null" response from the with_ast AST-unusable default, and
remove any mention of an empty array or TODO stub.
---
Outside diff comments:
In `@src/server/compiler/compiler.cpp`:
- Around line 763-768: Persist the missing PCH cache field by updating the cache
save/load path for PCHState so preamble_links is serialized and restored
alongside path, key, bound, and deps. Locate the PCH cache handling in
Workspace::save_cache(), Workspace::load_cache(), and the compiler code that
assigns st.preamble_links, and make sure cache entries round-trip those links
across restarts so cache hits keep the preamble include links without requiring
a rebuild.
---
Nitpick comments:
In `@src/feature/document_links.cpp`:
- Around line 25-38: The directive-argument lookup is duplicated in add_link and
try_directive, so extract the shared “resolve directive argument” flow into a
helper that takes unit, content, lang_opts, and loc and returns the directive
argument range when the location is on an `#include` or __has_include argument.
Update both document_links and include_definition to call this helper and keep
only their specific follow-up logic (building protocol::DocumentLink vs.
checking containment/definition handling), so future directive tweaks stay in
one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa11c093-7ca6-40c2-bc2b-53810c0e3f69
📒 Files selected for processing (20)
docs/en/features/navigation.mdsrc/feature/diagnostics.cppsrc/feature/document_links.cppsrc/feature/feature.hsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/protocol/worker.hsrc/server/service/lsp_client.cppsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/worker/stateful_worker.cppsrc/server/worker/stateless_worker.cppsrc/server/worker/worker_common.hsrc/server/workspace/workspace.htests/data/index_features/nav.cpptests/data/index_features/nav_late.htests/integration/features/test_index.pytests/integration/modules/test_modules.pytests/unit/feature/document_link_tests.cpptests/unit/server/stateful_worker_tests.cpp
💤 Files with no reviewable changes (1)
- src/feature/diagnostics.cpp
Module names are indexed like macro symbols: import sites and module
declarations emit occurrences and Definition/Reference relations keyed
by a hash of the full module name, so go-to-definition and references
flow through the ordinary index pipeline — macro-expanded module names
and partition imports (clang resolves :part to its enclosing module)
work for free, and the master's ad-hoc line-lexing scan is deleted.
Fixes the dotted-import name join in the directive collector and bumps
the cache format version so old shards reindex.
document_links now produces the plain feature::DocumentLink {range,
target} end to end — worker RPC, PCH cache and master merge use it
directly, and the LSP DocumentLink exists only at the reply edge.
worker::FileLink and both conversion helpers are deleted.
Preprocessor::getNamedModuleName asserts on non-module units in Debug; index_modules now checks is_named_module() first. The integration cache helper follows the bumped on-disk version.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/compile/compilation_unit.h (1)
210-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment now misattributed to the wrong method.
Line 210's
/// Get the C++20 named module name if any.describesmodule_name(), but the newis_named_module()declaration was inserted between it andmodule_name(), so it now reads as documentation foris_named_module().✏️ Proposed fix
- /// Get the C++20 named module name if any. /// Whether this unit is a named module (interface or implementation). /// Must be checked before module_name(): the preprocessor asserts on /// name access for non-module units. bool is_named_module(); + /// Get the C++20 named module name if any. auto module_name() -> llvm::StringRef;🤖 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 `@src/compile/compilation_unit.h` around lines 210 - 216, The doc comment above is now attached to is_named_module() instead of module_name(), so move or rewrite it to document the correct method. Keep the “named module name” text with module_name(), and add a separate brief comment for is_named_module() explaining that it reports whether the compilation unit is a named module; use the existing symbols is_named_module() and module_name() to place the comments correctly.src/compile/directive.h (1)
124-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment contradicts actual
full_namefallback behavior.The comment states
full_nameis "empty when clang could not resolve the module", but the assignment indirective.cpp'smoduleImportsets it toimport.name(not empty) whenMis null: "import.full_name = M ? M->getFullModuleName() : import.name;" So for an unresolved import,full_namemirrorsnamerather than being empty. This also makes the downstreamimport.full_name.empty() ? import.name : import.full_namefallback intu_index.cppeffectively dead for the "unresolved" case it appears to guard against.📝 Suggested comment fix
- /// Resolved full module name (includes the enclosing module for - /// partition imports); empty when clang could not resolve the module. + /// Resolved full module name (includes the enclosing module for + /// partition imports); falls back to `name` when clang could not + /// resolve the module (never actually empty if `name` is non-empty). std::string full_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 `@src/compile/directive.h` around lines 124 - 127, Update the documentation for `Directive::full_name` to match the actual behavior in `moduleImport`: for unresolved imports it is populated from `import.name`, not left empty. Also review the `tu_index.cpp` fallback that checks `import.full_name.empty()` and adjust either the comment or the logic so the documented behavior and downstream handling are consistent.
🤖 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.
Inline comments:
In `@src/index/tu_index.cpp`:
- Around line 226-232: The identifier parsing in tu_index.cpp is dereferencing
the result of advance_if(is_identifier) without checking it first. Update the
logic around the lexer.next(), lexer.advance_if(is_identifier), and the
name_begin/name_end assignments to guard against a null/empty result before
accessing first->range, matching the defensive pattern already used later for
sep/part in the same function.
---
Nitpick comments:
In `@src/compile/compilation_unit.h`:
- Around line 210-216: The doc comment above is now attached to
is_named_module() instead of module_name(), so move or rewrite it to document
the correct method. Keep the “named module name” text with module_name(), and
add a separate brief comment for is_named_module() explaining that it reports
whether the compilation unit is a named module; use the existing symbols
is_named_module() and module_name() to place the comments correctly.
In `@src/compile/directive.h`:
- Around line 124-127: Update the documentation for `Directive::full_name` to
match the actual behavior in `moduleImport`: for unresolved imports it is
populated from `import.name`, not left empty. Also review the `tu_index.cpp`
fallback that checks `import.full_name.empty()` and adjust either the comment or
the logic so the documented behavior and downstream handling are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70196aab-e579-4964-a5a1-28f81000b605
📒 Files selected for processing (22)
src/compile/compilation_unit.cppsrc/compile/compilation_unit.hsrc/compile/directive.cppsrc/compile/directive.hsrc/feature/document_link.hsrc/feature/document_links.cppsrc/feature/feature.hsrc/index/tu_index.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/protocol/worker.hsrc/server/service/lsp_client.cppsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/worker/stateful_worker.cppsrc/server/worker/stateless_worker.cppsrc/server/worker/worker_common.hsrc/server/workspace/workspace.htests/integration/compilation/test_persistent_cache.pytests/integration/modules/test_modules.pytests/integration/utils/cache.pytests/unit/feature/document_link_tests.cpp
💤 Files with no reviewable changes (1)
- src/server/worker/worker_common.h
✅ Files skipped from review due to trivial changes (2)
- src/feature/document_link.h
- tests/integration/compilation/test_persistent_cache.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/server/service/master_server.h
- tests/unit/feature/document_link_tests.cpp
- src/feature/feature.h
- src/feature/document_links.cpp
- src/server/service/lsp_client.cpp
- src/server/worker/stateful_worker.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df0478e5d8
ℹ️ 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".
Review follow-ups: dirty sessions skip the cached preamble links (they may describe the pre-edit preamble) and the definition handler retries the index and preamble lookups after the worker compile refreshed them, so unsaved imports and preamble includes resolve on the first request. Module Definition relations carry their definition range, and the module-decl lexer guards the name token.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90c683da9d
ℹ️ 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".
decompose_expansion_range replaces the raw-location width computation: macro-spelled module names (import MOD;) previously tripped the Debug file-location assertion and indexed against the macro definition. Adds a macro_import e2e workspace.
A dirty session's eager index query could return a non-empty hit from the pre-edit merged shard and bypass the compile-then-retry path; dirty buffers now go straight to the worker compile and re-query the fresh session index.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf80dceea1
ℹ️ 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".
Review follow-ups: the definition retry and the documentLink preamble merge only consult cached data when the compile actually refreshed the session (a failed or superseded compile leaves ast_dirty set). On a worker crash the master relays the tail of the dead worker's log file into its own log — the assert message and backtrace live there, and CI only captures the master's output.
…avigation # Conflicts: # src/server/compiler/compiler.cpp # src/server/protocol/worker.h # src/server/service/lsp_client.cpp # src/server/worker/stateful_worker.cpp # src/server/worker/stateless_worker.cpp # src/server/workspace/workspace.h # tests/integration/utils/cache.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dd413421b
ℹ️ 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".
1 participant
Background
Go-to-definition did not work on
#includedirectives or module names. The include case is complicated by the PCH split: an open file's preamble is compiled into the PCH, so its include directives are invisible to the stateful worker's AST. The preamble's document links were previously shipped as a pre-serialized JSON blob and merged into the LSP reply by string surgery.Implementation
Structured links over the binary protocol.
worker::FileLink { range, target }replaces the JSON blob end to end:BuildResult::preamble_links(PCH build),PCHState::preamble_links(master cache), and a dedicated typed RPC (DocumentLinkParams→std::vector<FileLink>) replacing theQueryKind::DocumentLinkRawValue pass-through.protocol::DocumentLinkitself cannot travel over bincode (optional/LSPAny fields), hence the plain struct. The documentLink handler now merges preamble and main-region links structurally and serializes once.with_astis generalized towith_ast_orso typed handlers share the AST access path. The links type is the plainfeature::DocumentLink {range, target}end to end — the LSP DocumentLink exists only at the reply edge.Include-directive definition. Resolution order in the definition handler: positions on a preamble include line answer master-side from the cached links; main-region includes are served by the worker's previously-stubbed
GoToDefinitionviafeature::include_definition(covers#includeand__has_include); everything else falls through to the index and worker as before.Module-name definition — through the index. Module names are indexed like macro symbols: import sites and module declarations emit occurrences plus Definition/Reference relations keyed by a hash of the full module name (
SymbolKind::Module, external scope), so go-to-definition, find-references and workspace-symbol all flow through the ordinary index pipeline. Macro-expanded module names and partition imports work for free — clang resolvesimport :A;to its enclosing module'sLib:A. Coversimport a.b;(fixing a dotted-name join bug in the directive collector), interfaceexport module M;and implementation-unitmodule M;declarations. The cache format version is bumped so pre-existing shards reindex.Known limitations (recorded)
Interface → implementation-unit navigation is not implemented (the reverse works).
Tests
Unit:
IncludeDefinition(feature function, positive + negative offsets), typedDocumentLinkWithoutCompile, alignedGoToDefinitionWithoutCompile. E2E: definition on preamble and non-preamble includes, documentLink preamble merge,import Math;, dottedimport my.io;, partitionimport :A;, implementation-unitmodule Greeter;, and the keyword-cursor negative.Test plan
pixi run formatclean; parallel review subagents (correctness / style+tests) — findings fixed or recorded