Repository navigation
feat(document links): preserve PCH document links and add #embed support - #413
Merged
Merged
Conversation
…support PCH compilation now serializes document links and stores them in PCHState. The master server merges PCH links with main-file links on DocumentLink requests, fixing missing links for includes inside the preamble. Also adds document link support for #embed and __has_embed directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds DocumentLink emission for Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant MS as Master Server
participant Compiler as Compiler
participant SW as Stateless Worker
participant PCHCache as PCH Cache
Note over Compiler,SW: PCH build flow
Client->>Compiler: request build PCH
Compiler->>SW: handle_build_pch()
SW->>SW: feature::document_links(unit)
SW->>SW: serialize DocumentLink[] -> pch_links_json
SW-->>Compiler: BuildResult (includes pch_links_json)
Note over Compiler,PCHCache: persist cache
Compiler->>PCHCache: ensure_pch() stores PCHState.document_links_json
Note over Client,MS: Document links query
Client->>MS: DocumentLinkParams
MS->>Compiler: forward_query() (main-file links)
MS->>PCHCache: read PCHState.document_links_json
MS->>MS: merge main-file links + PCH JSON array
MS-->>Client: merged DocumentLink[] result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
16bit-ykiko
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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.cpp (1)
491-498:⚠️ Potential issue | 🟠 MajorReset stale
session.pch_refon PCH build failure.
After Line 491 failure, the function returnsfalsebut keeps any priorsession.pch_ref. With PCH-link merging enabled, this can surface stale preamble links for the current buffer.💡 Suggested fix
if(!result.has_value() || !result.value().success) { LOG_WARN("PCH build failed for {}: {}", path, result.has_value() ? result.value().error : result.error().message); workspace.pch_cache[path_id].building.reset(); + session.pch_ref.reset(); completion->set(); co_return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 491 - 498, When the PCH build fails in the failure branch (the if block that checks !result.has_value() || !result.value().success in the PCH build flow), clear any stale session.pch_ref to avoid retaining prior preamble links; add a call to reset/clear session.pch_ref (the same session object used by the build) before calling completion->set() and co_return false so the session no longer references an invalid PCH.
🧹 Nitpick comments (1)
src/server/stateless_worker.cpp (1)
102-105: Normalizepch_links_jsonto an array shape for safer merges.
to_raw(...)can fallback to"null". Since downstream logic merges array strings, coercing non-array output to"[]"avoids malformed concatenation paths.💡 Suggested hardening
if(success) { tu_index_data = serialize_tu_index(unit); auto links = feature::document_links(unit); auto raw = to_raw(links); - pch_links_json = std::move(raw.data); + pch_links_json = std::move(raw.data); + if(pch_links_json.empty() || pch_links_json == "null") { + pch_links_json = "[]"; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/stateless_worker.cpp` around lines 102 - 105, The code assigns pch_links_json from to_raw(feature::document_links(unit)).data but to_raw can produce "null" (or other non-array JSON), which will break downstream string-array merges; update the assignment so after obtaining auto raw = to_raw(links) you normalize raw.data into an array-shaped string before moving it into pch_links_json: if raw.data is empty, equals "null", or does not begin with '[' (after trimming whitespace), replace it with "[]"; otherwise move the original raw.data. Reference: feature::document_links, to_raw, raw, and pch_links_json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 493-505: The merge uses workspace.pch_cache[path_id] without
validating that the cached entry matches the session's PCHRef; add a guard that
compares pch_it->second.hash and pch_it->second.bound against
sit->second.pch_ref->hash and sit->second.pch_ref->bound before reading
document_links_json, and only perform the JSON splice into links.data when both
hash and bound match (otherwise skip merging); reference sit->second.pch_ref,
workspace.pch_cache, path_id, hash, bound, document_links_json, and links.data
when locating where to add the check.
In `@tests/integration/features/test_document_links.py`:
- Line 30: The test binds an unused variable named content from the call to
client.open_and_wait in tests/integration/features/test_document_links.py;
replace the unused binding with the conventional underscore (_) so the call
becomes uri, _ = client.open_and_wait(...) (refer to the client.open_and_wait
call and the uri variable) to silence Ruff and match the rest of the file's
pattern for ignored return values.
---
Outside diff comments:
In `@src/server/compiler.cpp`:
- Around line 491-498: When the PCH build fails in the failure branch (the if
block that checks !result.has_value() || !result.value().success in the PCH
build flow), clear any stale session.pch_ref to avoid retaining prior preamble
links; add a call to reset/clear session.pch_ref (the same session object used
by the build) before calling completion->set() and co_return false so the
session no longer references an invalid PCH.
---
Nitpick comments:
In `@src/server/stateless_worker.cpp`:
- Around line 102-105: The code assigns pch_links_json from
to_raw(feature::document_links(unit)).data but to_raw can produce "null" (or
other non-array JSON), which will break downstream string-array merges; update
the assignment so after obtaining auto raw = to_raw(links) you normalize
raw.data into an array-shaped string before moving it into pch_links_json: if
raw.data is empty, equals "null", or does not begin with '[' (after trimming
whitespace), replace it with "[]"; otherwise move the original raw.data.
Reference: feature::document_links, to_raw, raw, and pch_links_json.
🪄 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: 6aae6e6d-4726-4022-810f-1b482fb692ca
⛔ Files ignored due to path filters (1)
tests/data/document_links/data.binis excluded by!**/*.bin
📒 Files selected for processing (13)
src/feature/document_links.cppsrc/server/compiler.cppsrc/server/master_server.cppsrc/server/protocol.hsrc/server/stateless_worker.cppsrc/server/workspace.htests/conftest.pytests/data/document_links/header_a.htests/data/document_links/header_b.htests/data/document_links/header_c.htests/data/document_links/main.cpptests/integration/features/test_document_links.pytests/unit/feature/document_link_tests.cpp
Cover both existing-file (produces link) and missing-file (no link) cases for __has_embed directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/features/test_document_links.py (1)
38-38:⚠️ Potential issue | 🟡 MinorUse
_for the unused open result.Line 38 binds
contentbut never uses it; this reintroduces the Ruff warning.Minimal fix
- uri, content = await client.open_and_wait(workspace / "main.cpp") + uri, _ = await client.open_and_wait(workspace / "main.cpp")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_document_links.py` at line 38, The test binds an unused variable `content` from the `client.open_and_wait` call, triggering a Ruff unused-variable warning; change the assignment to use `_` for the unused value (e.g., `uri, _ = await client.open_and_wait(workspace / "main.cpp")`) so only the used `uri` is bound and the linter warning is resolved.
🧹 Nitpick comments (1)
tests/integration/features/test_document_links.py (1)
36-138: Guaranteeclient.close(uri)viafinallyto avoid test-state leaks on assertion failures.If an assertion fails before cleanup, the document can remain open and affect later tests. Wrap test bodies with
try/finally(or centralize in a helper).Pattern to apply
`@pytest.mark.workspace`("document_links") async def test_document_links_with_pch(client, workspace): """Document links from both PCH and main compilation are returned.""" - uri, _ = await client.open_and_wait(workspace / "main.cpp") - links = await client.document_links(uri) - - assert links is not None, "document_links returned None" - - targets = sorted(Path(link.target).name for link in links) - assert targets == [ - "data.bin", - "data.bin", - "header_a.h", - "header_b.h", - "header_c.h", - ], f"Unexpected targets: {targets}" - - client.close(uri) + uri, _ = await client.open_and_wait(workspace / "main.cpp") + try: + links = await client.document_links(uri) + assert links is not None, "document_links returned None" + + targets = sorted(Path(link.target).name for link in links) + assert targets == [ + "data.bin", + "data.bin", + "header_a.h", + "header_b.h", + "header_c.h", + ], f"Unexpected targets: {targets}" + finally: + client.close(uri)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_document_links.py` around lines 36 - 138, Wrap each test body so client.close(uri) is guaranteed in a finally block to avoid leaving documents open on assertion failures; for each test function (test_document_links_with_pch, test_document_links_pch_portion, test_document_links_main_portion, test_document_links_embed, test_document_links_has_embed_exists, test_document_links_has_embed_missing) move the work that opens the file and asserts into a try: block and place client.close(uri) in the corresponding finally: block (or use a small helper that ensures close in finally) so cleanup always runs even if assertions raise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/features/test_document_links.py`:
- Line 38: The test binds an unused variable `content` from the
`client.open_and_wait` call, triggering a Ruff unused-variable warning; change
the assignment to use `_` for the unused value (e.g., `uri, _ = await
client.open_and_wait(workspace / "main.cpp")`) so only the used `uri` is bound
and the linter warning is resolved.
---
Nitpick comments:
In `@tests/integration/features/test_document_links.py`:
- Around line 36-138: Wrap each test body so client.close(uri) is guaranteed in
a finally block to avoid leaving documents open on assertion failures; for each
test function (test_document_links_with_pch, test_document_links_pch_portion,
test_document_links_main_portion, test_document_links_embed,
test_document_links_has_embed_exists, test_document_links_has_embed_missing)
move the work that opens the file and asserts into a try: block and place
client.close(uri) in the corresponding finally: block (or use a small helper
that ensures close in finally) so cleanup always runs even if assertions raise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29857cc1-5008-4294-8231-84527120834d
📒 Files selected for processing (2)
tests/data/document_links/main.cpptests/integration/features/test_document_links.py
The sessions DenseMap iterator may be invalidated during co_await (other coroutines can modify the map). Re-lookup by path_id after the await completes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 501-508: The merge logic for links.data and pch_json can produce a
trailing comma when pch_json is "[]"; update the condition that enters the
concatenation branch to verify pch_json is a non-empty array (e.g.
pch_json.size() > 2 and pch_json != "null") before trimming its leading '[' and
trailing ']' and appending — otherwise assign links.data = pch_json; modify the
block that uses links.data.pop_back(), links.data += ',' and
links.data.append(pch_json.begin() + 1, pch_json.end()) to only run when both
links.data and pch_json contain real elements to avoid producing "[a,b,]".
🪄 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: 2c163844-7d66-4fcc-a05e-d55cba396ae0
📒 Files selected for processing (1)
src/server/master_server.cpp
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/feature/document_links.cpp`:
- Line 43: The reserve call on links only accounts for directives.includes and
directives.has_includes, so update the reservation to also include the sizes of
directives.embeds and directives.has_embeds to avoid reallocations; locate the
links.reserve(...) call and change the capacity computation to sum
directives.includes.size(), directives.has_includes.size(),
directives.embeds.size(), and directives.has_embeds.size() so links is
preallocated for all four collections.
🪄 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: a8f852c0-9b98-43dd-9920-072e4c6792e7
📒 Files selected for processing (1)
src/feature/document_links.cpp
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ed/has_embed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/features/test_document_links.py (1)
8-8:⚠️ Potential issue | 🟡 MinorReplace unused
contentbinding with_.Line 8 still binds
contentbut never uses it, so Ruff will keep flagging RUF059.Minimal fix
- uri, content = await client.open_and_wait(workspace / "main.cpp") + uri, _ = await client.open_and_wait(workspace / "main.cpp")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_document_links.py` at line 8, Replace the unused `content` binding with `_` to satisfy Ruff RUF059: change the tuple unpacking on the call to client.open_and_wait (the line using `uri, content = await client.open_and_wait(workspace / "main.cpp")`) to `uri, _ = await client.open_and_wait(workspace / "main.cpp")`, leaving the call and `uri` usage unchanged.
🧹 Nitpick comments (1)
tests/integration/features/test_document_links.py (1)
7-103: Usetry/finallyto guaranteeclient.close(uri)cleanup.Right now cleanup is skipped if an assertion throws. Wrapping each test body with
try/finallywill keep test state isolated and reduce cascade failures.Suggested pattern
async def test_document_links_with_pch(client, workspace): - uri, _ = await client.open_and_wait(workspace / "main.cpp") - links = await client.document_links(uri) - - assert links is not None, "document_links returned None" - ... - client.close(uri) + uri, _ = await client.open_and_wait(workspace / "main.cpp") + try: + links = await client.document_links(uri) + assert links is not None, "document_links returned None" + ... + finally: + client.close(uri)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_document_links.py` around lines 7 - 103, Each test (e.g., test_document_links_with_pch, test_document_links_pch_portion, test_document_links_main_portion, test_document_links_embed, test_document_links_has_embed_exists, test_document_links_has_embed_missing) currently calls client.close(uri) at the end but will skip cleanup if an assertion raises; wrap the body after uri, _ = await client.open_and_wait(...) in a try/finally and move client.close(uri) into the finally block so the client is always closed even on failures (ensure the try encloses the calls to client.document_links, subsequent assertions, and any processing of links).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/features/test_document_links.py`:
- Line 8: Replace the unused `content` binding with `_` to satisfy Ruff RUF059:
change the tuple unpacking on the call to client.open_and_wait (the line using
`uri, content = await client.open_and_wait(workspace / "main.cpp")`) to `uri, _
= await client.open_and_wait(workspace / "main.cpp")`, leaving the call and
`uri` usage unchanged.
---
Nitpick comments:
In `@tests/integration/features/test_document_links.py`:
- Around line 7-103: Each test (e.g., test_document_links_with_pch,
test_document_links_pch_portion, test_document_links_main_portion,
test_document_links_embed, test_document_links_has_embed_exists,
test_document_links_has_embed_missing) currently calls client.close(uri) at the
end but will skip cleanup if an assertion raises; wrap the body after uri, _ =
await client.open_and_wait(...) in a try/finally and move client.close(uri) into
the finally block so the client is always closed even on failures (ensure the
try encloses the calls to client.document_links, subsequent assertions, and any
processing of links).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd3de452-bd91-49ec-8b49-4901bdc39116
📒 Files selected for processing (1)
tests/integration/features/test_document_links.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks done
3 tasks done
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
1 participant
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.
Summary
pch_links_jsoninBuildResultand stores them inPCHStatetextDocument/documentLinkrequests, fixing missing links for#includedirectives inside the preamble#embedand__has_embeddirectivesTest plan
DocumentLink.EmbedandDocumentLink.HasEmbedaddedtest_document_links.pyverifies PCH + main merge and#embedlinks🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores