Repository navigation
feat: implement index system with LSP query handlers - #382
Merged
Merged
Conversation
|
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 persistent TU/project indexing and PCH caching: FlatBuffers TUIndex persistence and schema extensions (symbol names, removed bitmap), merged-index removal tracking and filtering, ProjectIndex path normalization and name round-trips, MasterServer background indexing and PCH reuse, index-driven LSP handlers, and tests for indexing and PCH behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MasterServer
participant PCHCache
participant Worker
participant Disk
Note over Client,Disk: PCH Build & Reuse Flow
Client->>MasterServer: didOpen/didChange (text)
MasterServer->>MasterServer: compute_preamble_bound(text)
MasterServer->>PCHCache: lookup by preamble_hash
alt cached and hash matches
PCHCache-->>MasterServer: cached pch_path
else build new
MasterServer->>Worker: BuildPCHParams(args, text, preamble_bound)
Worker->>Worker: build PCH
Worker->>Disk: write PCH file
Worker-->>MasterServer: BuildPCHResult(pch_path)
MasterServer->>PCHCache: store pch_path,preamble_bound,pch_hash
end
MasterServer->>Worker: CompileParams(..., pch={pch_path,preamble_bound})
Worker->>Worker: compile with PCH
Worker-->>MasterServer: CompileResult
MasterServer-->>Client: diagnostics
sequenceDiagram
actor User
participant MasterServer
participant IndexQueue
participant Worker
participant ProjectIndex
participant MergedIndices
participant Disk
Note over User,Disk: Background Indexing Pipeline
User->>MasterServer: initialize workspace
MasterServer->>Disk: load_index()
Disk-->>MasterServer: persisted ProjectIndex & MergedIndices
MasterServer->>IndexQueue: populate from CDB
MasterServer->>MasterServer: schedule_indexing()
loop idle indexing
MasterServer->>IndexQueue: dequeue file_id
MasterServer->>Worker: Index request (stateless)
Worker->>Worker: build TUIndex & serialize
Worker-->>MasterServer: serialized TUIndex
MasterServer->>ProjectIndex: merge symbols/refs
MasterServer->>MergedIndices: merge occurrences/relations
end
MasterServer->>Disk: save_index()
Disk->>Disk: persist
sequenceDiagram
actor Client
participant MasterServer
participant ProjectIndex
participant MergedIndices
participant AST
Note over Client,AST: Index-Based LSP Query Flow
Client->>MasterServer: textDocument/definition `@pos`
MasterServer->>MasterServer: lookup_symbol_at_position()
MasterServer->>ProjectIndex: find symbol
MasterServer->>MasterServer: query_index_relations(symbol, Definition)
alt index hit
MasterServer->>MergedIndices: query shards for relation
MergedIndices-->>MasterServer: locations
MasterServer-->>Client: definition locations
else fallback
MasterServer->>AST: stateful lookup
AST-->>MasterServer: definition location
MasterServer-->>Client: definition location
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index/merged_index.cpp (2)
415-436:⚠️ Potential issue | 🟠 MajorMissing removed bitmap filtering in buffer-based lookup path.
When
self.implis null butself.bufferexists, the lookup iterates over serialized occurrences without checking theremovedbitmap. This creates an inconsistency: in-memory lookups (lines 393-403) filter removed entries, but buffer-based lookups return stale data.Consider either:
- Loading the removed bitmap from the buffer and filtering, or
- Calling
load_in_memory()at the start of lookup to ensure consistent behaviorPotential fix approach
} else if(self.buffer) { auto index = fbs::GetRoot<binary::MergedIndex>(self.buffer->getBufferStart()); auto& occurrences = *index->occurrences(); + + // Deserialize removed bitmap for filtering + roaring::Roaring removed_bitmap; + if(index->removed() && index->removed()->size() > 0) { + removed_bitmap = read_bitmap(index->removed()); + } auto it = std::ranges::lower_bound(occurrences, offset, {}, [](auto o) { return o->occurrence()->range().end(); }); while(it != occurrences.end()) { auto o = safe_cast<Occurrence>(it->occurrence()); if(o->range.contains(offset)) { + // Skip if all canonical_ids are removed + if(!removed_bitmap.isEmpty()) { + auto context_bitmap = read_bitmap(it->context()); + auto remaining = context_bitmap - removed_bitmap; + if(remaining.isEmpty()) { + it++; + continue; + } + } if(!callback(*o)) { break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index/merged_index.cpp` around lines 415 - 436, The buffer-based lookup path in the lookup routine currently iterates serialized occurrences from self.buffer (using fbs::GetRoot<binary::MergedIndex> and occurrences) without applying the removed bitmap check, causing stale entries to be returned when self.impl is null; to fix, either load and consult the removed bitmap from the on-disk buffer before iterating (extract the removed bitmap field from the MergedIndex in the buffer and skip occurrences whose ordinal is marked removed) or simply call load_in_memory() at the start of lookup when self.impl is null so the existing in-memory removed-bit filtering logic is reused; update the branch that handles self.buffer to perform one of these two actions and ensure the callback(*o) is only invoked for non-removed occurrences.
465-482:⚠️ Potential issue | 🟠 MajorSame issue: missing removed filtering in buffer-based relation lookup.
The relation lookup from buffer (lines 465-482) also doesn't filter by the
removedbitmap, creating the same inconsistency as the occurrence lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index/merged_index.cpp` around lines 465 - 482, When iterating relations from the buffer-based lookup in merged_index.cpp (the branch using self.buffer, fbs::GetRoot<binary::MergedIndex>, entries and it->relations()), add the same removed-bitset filtering used by the occurrence lookup: obtain the removed bitmap from the flatbuffer index and skip any relation whose id/index is marked removed before checking r->kind and calling callback. In practice, retrieve the removed bitmap from index (the same field used elsewhere), extract each relation's identifier (from entry->relation() or the Relation object), test it against the removed bitmap, and continue/skip if removed; only then apply the kind check and invoke callback.
🧹 Nitpick comments (2)
tests/integration/test_pch.py (1)
76-104: Test may pass without meaningful completion verification.The completion test only asserts
result is not None, which passes even if completion returns an empty list. Consider asserting that completion items actually contain "Point" (fromcommon.h):Suggested improvement
result = await client.text_document_completion_async( CompletionParams( text_document=_doc(uri), position=Position(line=last_line, character=3), ) ) - # Completion should return results. - assert result is not None + # Completion should return results including Point from common.h + assert result is not None + items = result.items if hasattr(result, 'items') else result + labels = [item.label for item in items] if items else [] + assert "Point" in labels, f"Expected 'Point' in completions, got: {labels}" client.text_document_did_close(DidCloseTextDocumentParams(text_document=_doc(uri)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_pch.py` around lines 76 - 104, The test_completion_with_pch currently only checks that result is not None, which can pass for empty completions; update the assertion to verify that completion items include the expected symbol from the PCH (e.g., "Point" from common.h). Locate the test_completion_with_pch function and change the post-request assertions on result (returned by text_document_completion_async / CompletionParams) to assert that result.items (or the result list) is non-empty and contains an entry whose label equals "Point" (or contains "Point"), and keep the existing cleanup (text_document_did_close) intact.src/index/schema.fbs (1)
20-25: Consider conventional FlatBuffers field formatting.The field declarations use an unconventional multi-line format:
sha256: string;While valid, the conventional single-line format is more readable:
sha256: string;This appears throughout the file. Consider reformatting for consistency with FlatBuffers conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index/schema.fbs` around lines 20 - 25, The FlatBuffers schema uses multi-line field declarations (e.g., in table CacheEntry for sha256 and canonical_id) which is unconventional; update those fields to the conventional single-line format (e.g., "sha256: string;" and "canonical_id: uint;") throughout schema.fbs, ensuring every table/field follows the single-line "name: type;" style for consistency and readability (search for occurrences of "sha256", "canonical_id" and similar multi-line declarations and collapse them into single-line declarations).
🤖 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/index/project_index.cpp`:
- Around line 90-94: ProjectIndex::from() currently dereferences
entry->symbol()->name() without checking for null; update the loop that
populates index.symbols (the block using entry->symbol()->name(), SymbolKind,
and read_bitmap) to guard against a missing name field (as loaded by
MasterServer::load_index()). Specifically, check whether entry->symbol() and
entry->symbol()->name() are non-null before calling ->str(); if null, set
symbol.name to a safe default (e.g., empty string or a placeholder) so
deserialization of older persisted indices does not crash. Ensure the rest of
the assignments (symbol.kind = SymbolKind(...), symbol.reference_files =
read_bitmap(...)) remain unchanged.
In `@src/index/tu_index.cpp`:
- Around line 259-260: Change TUIndex::from(const void* data) to accept a size
parameter (e.g., TUIndex::from(const void* data, size_t size)) and update
callers (notably merge_index_result()) to pass the byte count; inside from() use
flatbuffers::Verifier on the reinterpret_cast<const uint8_t*>(data) and the
supplied size and call binary::VerifyTUIndexBuffer(verifier) (or the generated
verify function) and return a recoverable failure (e.g., std::nullopt or an
error) if verification fails instead of calling
fbs::GetRoot<binary::TUIndex>(data); alternatively ensure flatc is invoked with
--gen-verify in CMakeLists so VerifyTUIndexBuffer exists.
In `@src/server/master_server.cpp`:
- Around line 231-232: Before calling schedule_indexing(), re-populate
index_queue with any files that were edited/changed since workspace load so the
background indexer processes them; implement a helper (e.g.,
requeue_edited_files or merge_edited_into_index_queue) that takes the
edited/changed file set used by the compile step and pushes those paths into
index_queue (avoiding duplicates) and call that helper from the same
post-compile sites that now call schedule_indexing() (the code that calls
schedule_indexing() and uses index_queue should be updated to invoke the requeue
helper first).
- Around line 607-619: The scheduling logic allows multiple background indexing
coroutines to be queued because indexing_active only flips true after the idle
timer; introduce a scheduling guard (e.g., a new bool member like
indexing_scheduled) and use it in MasterServer::schedule_indexing() to prevent
re-queuing: check indexing_scheduled at the top, set it to true before
creating/starting index_idle_timer and calling
loop.schedule(run_background_indexing()), and then clear indexing_scheduled
inside the run_background_indexing() coroutine when it actually begins or
finishes so future idle windows can schedule again; apply the same guard to the
other similar scheduling path that currently queues run_background_indexing() so
only one idle pass drains index_queue_pos at a time (reference symbols:
schedule_indexing(), indexing_active, index_idle_timer,
run_background_indexing(), index_queue_pos).
- Around line 1031-1044: The resolve_hierarchy_item function currently ignores
the stored info.hash in TypeHierarchyItem::data and calls
lookup_symbol_at_position (which fails for unopened files); change
resolve_hierarchy_item to first extract the symbol hash from item.data (the
int64_t stored by build_type_hierarchy_item) and use a symbol-hash based lookup
path (e.g., a find/lookup-by-hash helper in the symbol table/index) to resolve
the SymbolInfo even when the file is not open, falling back only to
lookup_symbol_at_position if the data hash is missing; update any callers or
helpers accordingly so incoming/outgoing/type-hierarchy expansion uses the
stored hash instead of requiring documents to be loaded.
- Around line 419-423: When compute_preamble_bound(text) returns 0 or when a PCH
rebuild fails, we must clear any stale cache entries so pch_paths and pch_bounds
do not keep serving an invalid PCH; update the early-return branch where bound
== 0 to erase the entry for this file from pch_paths and pch_bounds before
co_return true, and likewise on any rebuild/failure path in the PCH creation
logic (the code that attempts the rebuild and currently returns false) ensure it
removes the same pch_paths/pch_bounds entries for the file (use the same key
used to index those maps) so callers no longer receive stale PCHs.
- Around line 658-663: run_background_indexing currently sends only
params.file/params.directory/params.arguments to workers so shards built from
on-disk content can be used to answer position-based queries against the live
editor buffer in query_index_relations(), causing wrong/missing results for
unsaved edits; fix by tagging these background-indexed shards as disk-only and
making query_index_relations refuse position-based index queries against
disk-only shards. Concretely: in run_background_indexing (where IndexParams
params is constructed) set a boolean flag on IndexParams such as
params.disk_only = true (or params.version/file_snapshot = empty) when no live
buffer was provided, and then in query_index_relations detect that the target
shard has disk_only true and, for queries that resolve by cursor/offset, return
a non-indexed fallback (e.g., skip the shard or signal a miss) so only shards
built with live buffer state answer position-based queries.
- Around line 760-764: The offset for completion/signature requests is computed
from doc.text after the co_await ensure_pch, so a concurrent didChange can make
wp.text and wp.offset refer to different revisions; to fix, capture the same
text snapshot used for wp.text and compute wp.offset from that snapshot before
awaiting ensure_pch (or compute both wp.text and wp.offset into local variables
from the same snapshot and then assign them to wp), updating references around
ensure_pch/ wp.pch; key symbols: ensure_pch, wp.text, wp.offset, doc.text,
pch_paths, pch_bounds, wp.pch.
In `@tests/integration/test_index.py`:
- Around line 181-203: The test test_type_hierarchy_supertypes uses the wrong
position for the 'Dog' symbol; update the Position passed into
TypeHierarchyPrepareParams in test_type_hierarchy_supertypes so the line is 8
(Position(line=8, character=7)) instead of 7, then run the test to ensure the
TypeHierarchyPrepareParams -> type_hierarchy_supertypes_async flow returns the
expected supertypes for the Dog symbol.
- Around line 106-128: The test test_call_hierarchy_incoming is preparing the
call hierarchy at the wrong source position; update the Position used in
CallHierarchyPrepareParams inside test_call_hierarchy_incoming so it queries
line 18 instead of line 17 (i.e., change Position(line=17, character=4) to
Position(line=18, character=4)) when calling
client.text_document_prepare_call_hierarchy_async so the incoming-call check for
"add" matches the actual symbol location.
- Around line 161-178: The test test_type_hierarchy_prepare uses the wrong
0-based line for 'Dog'—update the Position passed to TypeHierarchyPrepareParams
(currently Position(line=7, character=7)) to the correct 0-based line for the
struct declaration (Position(line=8, character=7)) so the prepareTypeHierarchy
query targets the actual 'struct Dog : public Animal {' location; keep the rest
of the test (client calls, asserts, and close) unchanged.
- Around line 86-104: The test test_call_hierarchy_prepare is using the wrong
Position for the 'add' definition: it sets Position(line=17, character=4) but
the function signature int add(int a, int b) { is actually at line 18
(0-indexed); update the Position in the CallHierarchyPrepareParams for
test_call_hierarchy_prepare to Position(line=18, character=4) so the prepare
call targets the actual 'add' CallHierarchyItem (refer to
CallHierarchyPrepareParams, Position, and the test_call_hierarchy_prepare
function).
- Around line 33-52: The test_goto_definition uses incorrect source coordinates
for the 'add' call and its definition; update the DefinitionParams position to
point to the call at line 24, character 12 (0-indexed) and update the expected
definition check to assert any loc.range.start.line == 18 (where int add(...) is
declared) so the test matches main.cpp; adjust only the Position(line=...,
character=...) and the assertion comparing loc.range.start.line in this test.
- Around line 131-153: The test test_call_hierarchy_outgoing is preparing the
call hierarchy at the wrong line for compute; update the Position passed to
text_document_prepare_call_hierarchy_async from line=22 to line=23 (keep
character=4) so the Prepare call targets the `int compute()` declaration; adjust
the inline comment if desired and rerun the test to confirm outgoing calls
include `add`.
- Around line 60-78: The test test_find_references uses the wrong 0-indexed
position for global_var and its usages; update the Position passed into
ReferenceParams (used by client.text_document_references_async) from line=29 to
line=30 to point at the actual declaration, and update the expected usage line
comments/expectations from lines 32 and 36 to 33 and 37 respectively (the
assertion len(result) >= 3 can remain). Also adjust any inline comment that
states the declaration/usage line numbers to match the corrected 0-indexed
lines.
In `@tests/integration/test_pch.py`:
- Around line 22-30: The pch_test workspace used by test_pch_diagnostics_on_open
lacks a CMakeLists.txt so compile_commands.json isn't generated and the test
only sees vacuous empty diagnostics; add a CMakeLists.txt to the pch_test
workspace that declares a target for main.cpp (and any headers), configures
compiler flags/include paths, and enables generation of compile_commands.json
(e.g., via setting CMAKE_EXPORT_COMPILE_COMMANDS or a simple add_executable and
target_include_directories) so the test's CDB can be generated and the PCH build
path exercised when calling client.open_and_wait in
test_pch_diagnostics_on_open.
In `@tests/unit/server/stateless_worker_tests.cpp`:
- Around line 104-106: The test dereferences result.value() after a non-fatal
check; change the first check to a fatal assertion so we never dereference an
empty optional: replace EXPECT_TRUE(result.has_value()) with
ASSERT_TRUE(result.has_value()) (keep the subsequent
EXPECT_TRUE(result.value().success) and
EXPECT_FALSE(result.value().pch_path.empty()) as-is) so the test aborts on
missing value before calling result.value().
---
Outside diff comments:
In `@src/index/merged_index.cpp`:
- Around line 415-436: The buffer-based lookup path in the lookup routine
currently iterates serialized occurrences from self.buffer (using
fbs::GetRoot<binary::MergedIndex> and occurrences) without applying the removed
bitmap check, causing stale entries to be returned when self.impl is null; to
fix, either load and consult the removed bitmap from the on-disk buffer before
iterating (extract the removed bitmap field from the MergedIndex in the buffer
and skip occurrences whose ordinal is marked removed) or simply call
load_in_memory() at the start of lookup when self.impl is null so the existing
in-memory removed-bit filtering logic is reused; update the branch that handles
self.buffer to perform one of these two actions and ensure the callback(*o) is
only invoked for non-removed occurrences.
- Around line 465-482: When iterating relations from the buffer-based lookup in
merged_index.cpp (the branch using self.buffer,
fbs::GetRoot<binary::MergedIndex>, entries and it->relations()), add the same
removed-bitset filtering used by the occurrence lookup: obtain the removed
bitmap from the flatbuffer index and skip any relation whose id/index is marked
removed before checking r->kind and calling callback. In practice, retrieve the
removed bitmap from index (the same field used elsewhere), extract each
relation's identifier (from entry->relation() or the Relation object), test it
against the removed bitmap, and continue/skip if removed; only then apply the
kind check and invoke callback.
---
Nitpick comments:
In `@src/index/schema.fbs`:
- Around line 20-25: The FlatBuffers schema uses multi-line field declarations
(e.g., in table CacheEntry for sha256 and canonical_id) which is unconventional;
update those fields to the conventional single-line format (e.g., "sha256:
string;" and "canonical_id: uint;") throughout schema.fbs, ensuring every
table/field follows the single-line "name: type;" style for consistency and
readability (search for occurrences of "sha256", "canonical_id" and similar
multi-line declarations and collapse them into single-line declarations).
In `@tests/integration/test_pch.py`:
- Around line 76-104: The test_completion_with_pch currently only checks that
result is not None, which can pass for empty completions; update the assertion
to verify that completion items include the expected symbol from the PCH (e.g.,
"Point" from common.h). Locate the test_completion_with_pch function and change
the post-request assertions on result (returned by
text_document_completion_async / CompletionParams) to assert that result.items
(or the result list) is non-empty and contains an entry whose label equals
"Point" (or contains "Point"), and keep the existing cleanup
(text_document_did_close) intact.
🪄 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: 241a7c86-a4d9-42e0-962d-6a0f29a33e01
📒 Files selected for processing (25)
src/index/merged_index.cppsrc/index/project_index.cppsrc/index/schema.fbssrc/index/tu_index.cppsrc/index/tu_index.hsrc/semantic/ast_utility.cppsrc/server/config.cppsrc/server/config.hsrc/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/stateless_worker.cpptests/data/index_features/CMakeLists.txttests/data/index_features/main.cpptests/data/pch_test/common.htests/data/pch_test/main.cpptests/data/pch_test/no_includes.cpptests/integration/test_index.pytests/integration/test_pch.pytests/unit/compile/compilation_tests.cpptests/unit/index/index_query_tests.cpptests/unit/index/merged_index_tests.cpptests/unit/index/project_index_tests.cpptests/unit/server/pch_worker_tests.cpptests/unit/server/stateless_worker_tests.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/master_server.h (1)
12-14: Add direct STL includes for header self-sufficiency.
std::vector(used at lines 96, 140, 147) andstd::optional(used at lines 195, 199, 211) require<vector>and<optional>to be directly included. Currently, these types rely on transitive includes, which creates fragility.Suggested diff
`#include` <cstdint> `#include` <memory> +#include <optional> `#include` <string> +#include <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.h` around lines 12 - 14, The header currently including "index/merged_index.h", "index/project_index.h" and "semantic/relation_kind.h" relies on transitive includes for std::vector and std::optional; add direct includes for <vector> and <optional> at the top of that header so types used (std::vector at the locations noted and std::optional at the locations noted) are available without transitive dependencies, ensuring the header is self-sufficient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/master_server.h`:
- Around line 12-14: The header currently including "index/merged_index.h",
"index/project_index.h" and "semantic/relation_kind.h" relies on transitive
includes for std::vector and std::optional; add direct includes for <vector> and
<optional> at the top of that header so types used (std::vector at the locations
noted and std::optional at the locations noted) are available without transitive
dependencies, ensuring the header is self-sufficient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28a31a7d-56cc-4302-a181-50a3fc231e1e
📒 Files selected for processing (7)
src/index/include_graph.hsrc/index/project_index.cppsrc/index/tu_index.cppsrc/server/master_server.cppsrc/server/master_server.htests/integration/test_index.pytests/unit/compile/compilation_tests.cpp
✅ Files skipped from review due to trivial changes (2)
- src/index/include_graph.h
- tests/integration/test_index.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/compile/compilation_tests.cpp
- src/index/tu_index.cpp
- src/server/master_server.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index/project_index.h (1)
17-58: Consider consolidating path normalization logic.Two separate
PathPoolclasses exist (clice::index::PathPoolhere andclice::PathPoolinsrc/support/path_pool.h) with nearly identical backslash normalization code. While both work correctly, this duplication increases maintenance burden if normalization behavior needs to change.Consider extracting a shared
normalize_path()helper or unifying these classes if their use cases permit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index/project_index.h` around lines 17 - 58, The PathPool::path_id implementation duplicates backslash-normalization logic present elsewhere; extract that logic into a shared helper (e.g., normalize_path(llvm::StringRef) or normalize_path(llvm::SmallString<>&) in a new/central header) and call it from PathPool::path_id (and the other clice::PathPool) instead of inlining the replacement. Update PathPool::path_id to call the helper before cache lookup and ensure save(...) still receives the normalized path; remove the duplicated replace loop from the other PathPool and import the shared helper so both classes use the same normalization routine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index/project_index.h`:
- Around line 17-58: The PathPool::path_id implementation duplicates
backslash-normalization logic present elsewhere; extract that logic into a
shared helper (e.g., normalize_path(llvm::StringRef) or
normalize_path(llvm::SmallString<>&) in a new/central header) and call it from
PathPool::path_id (and the other clice::PathPool) instead of inlining the
replacement. Update PathPool::path_id to call the helper before cache lookup and
ensure save(...) still receives the normalized path; remove the duplicated
replace loop from the other PathPool and import the shared helper so both
classes use the same normalization routine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29667716-d030-424f-9cda-a627cb7dde8a
📒 Files selected for processing (3)
src/index/project_index.cppsrc/index/project_index.hsrc/support/path_pool.h
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 1386-1396: The current code concatenates JSON strings (ref_json
and def_json) which is fragile; instead parse both ref_json and def_json into
concrete objects (e.g., std::vector<protocol::Location>) using your JSON
deserializer, merge the two vectors, reserialize the merged vector back to a
JSON array string, and return that in serde_raw; update the block that
constructs merged (referencing refs, defs, ref_json, def_json, and serde_raw) to
perform deserialize->concat->serialize rather than manual substr splicing so
empty/whitespace/malformed arrays are handled correctly.
🪄 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: cc287b7a-74c8-42bf-af45-06c812a507c3
📒 Files selected for processing (2)
src/index/project_index.hsrc/server/master_server.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index/project_index.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/test_index.py (1)
37-38: Replace fixed sleep with condition-based wait to reduce flakiness.Using a hardcoded delay at Line 37 can still race on slower CI and adds avoidable runtime. Prefer polling for index readiness (or a helper that retries the query until success/timeout) instead of
asyncio.sleep(15).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_index.py` around lines 37 - 38, The test currently uses a fixed delay via asyncio.sleep(15), which is flaky; replace this with a condition-based wait that polls the index until it is ready (e.g., loop that retries a simple query/assert against the index client with short sleeps and a global timeout) or call a helper like wait_for_index_ready()/retry_until_success() instead; locate the await asyncio.sleep(15) in tests/integration/test_index.py and implement polling against the indexing function/client used in the test (or add a small retry helper used by the test) so the test proceeds as soon as the index is ready and fails only after a configurable timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_index.py`:
- Line 52: The list comprehension in tests/integration/test_index.py uses an
ambiguous variable name `l` which triggers Ruff E741; rename that variable to a
clearer identifier (e.g., `loc` or `location`) in the comprehension expression
that builds the tuple list for `locs` and update any references inside the tuple
(the `.uri`, `.range.start.line`, `.range.start.character`) accordingly so the
assertion message reads something like: f"Expected line 18, got locations:
{[(loc.uri, loc.range.start.line, loc.range.start.character) for loc in locs]}".
---
Nitpick comments:
In `@tests/integration/test_index.py`:
- Around line 37-38: The test currently uses a fixed delay via
asyncio.sleep(15), which is flaky; replace this with a condition-based wait that
polls the index until it is ready (e.g., loop that retries a simple query/assert
against the index client with short sleeps and a global timeout) or call a
helper like wait_for_index_ready()/retry_until_success() instead; locate the
await asyncio.sleep(15) in tests/integration/test_index.py and implement polling
against the indexing function/client used in the test (or add a small retry
helper used by the test) so the test proceeds as soon as the index is ready and
fails only after a configurable timeout.
🪄 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: a7edbd4b-a20b-4397-a985-09bb057e652d
📒 Files selected for processing (2)
src/server/master_server.cpptests/integration/test_index.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/master_server.cpp
…andlers Add complete index infrastructure: TUIndex build/serialize in StatelessWorker, ProjectIndex/MergedIndex state management in MasterServer with background idle-triggered indexing, and index-based LSP handlers for GoToDefinition, FindReferences, CallHierarchy, TypeHierarchy, and WorkspaceSymbol. Includes unit tests for index queries and Python E2E integration tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix merge_index_result include_locations filter collecting all locations - Fix background indexing skipping all files on fresh start - Fix schedule_indexing race condition with indexing_scheduled flag - Fix CallHierarchyItem/TypeHierarchyItem with empty URI when def not found - Fix handleMacroOccurrence ignoring kind parameter - Fix const_cast in TUIndex::serialize by making path_id const - Fix signed/unsigned comparison in project_index.cpp - Remove incorrect file_text.empty() check in query_index_relations - Fix Python E2E test line positions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Normalize backslashes to forward slashes in both server PathPool and index PathPool to ensure consistent path matching across URI decoding, CDB entries, and clang FileManager on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add PathPool::find() method that normalizes backslashes before lookup, replacing raw cache.find() calls that failed on Windows where uri_to_path() returns backslash-separated paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add LOG_INFO/LOG_WARN at key points in query_index_relations, lookup_symbol_at_position, merge_index_result, schedule_indexing, and run_background_indexing to diagnose why index queries return empty on Windows CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace hardcoded 15s sleeps with _wait_for_index() that polls workspace/symbol until indexing is confirmed ready. This is both faster (7s vs 150s locally) and more reliable on slower CI machines. Also add diagnostic logging to server-side index query methods and detailed assertion messages for easier debugging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add diagnostic prints in test_goto_definition to capture: - workspace_symbol results with locations - GoToDefinition at multiple positions - FindReferences result - CRLF detection in file content This will reveal whether the issue is offset mismatch, path mismatch, or MergedIndex not being populated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dump server [diag] and [warn] log lines after each test to understand why index queries fail on Windows but work on Linux. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Python's read_text() normalizes CRLF to LF, causing byte offset mismatch between didOpen content and index built from disk files on Windows. - Add .gitattributes to force LF line endings for test data - Read test files in binary mode to preserve line endings (like real LSP clients) - Remove diagnostic logging added during debugging - Downgrade query_index_relations warnings to debug level Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use the file content that was present at index build time for offset↔position conversion instead of relying on didOpen text or disk reads. This eliminates CRLF/LF mismatches on Windows and ensures queries work even when the file is not open in the editor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Guard against null `name` field in ProjectIndex deserialization - Use stored symbol hash from `data` field in hierarchy resolution instead of re-resolving from position (works for unopened files) - Rename ambiguous variable `l` to `loc` in test assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feature/index-system
branch
from
April 1, 2026 10:43
cc5c62c to
7fdeb4b
Compare
…ests The workspace fixture now removes .clice directory before each test, preventing stale index shards (missing content field) from causing workspace/symbol and other index queries to return empty results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 3, 2026
4 tasks done
2 of 3 tasks
5 tasks done
3 tasks done
This was referenced Jul 4, 2026
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
Implement the complete index system for cross-file LSP features. This adds persistent two-tier indexing (ProjectIndex + per-file MergedIndex shards), background indexing triggered on idle, and index-based query handlers for major LSP requests.
Index Data Layer (
src/index/)PathPoolpath normalization (backslash -> forward slash), and binary persistencecontentfield to store file content for reliable offset<->position mapping; addremovedbitmap for garbage collection of deleted entries; filter removed IDs inlookup()queriesSymbol.namefield,MergedIndex.removedbitmap andMergedIndex.contentstringServer (
src/server/)IndexParamsto stateless workers, merges returnedTUIndexinto ProjectIndex/MergedIndex, and persists to.clice/index/save_index()/load_index()for startup restoration; only rewrites shards flaggedneed_rewrite()textDocument/definition-- index-first lookup with stateful worker fallbacktextDocument/references-- cross-file reference query via indexcallHierarchy/prepare,incomingCalls,outgoingCalls-- Caller/Callee relation traversaltypeHierarchy/prepare,supertypes,subtypes-- Base/Derived relation traversalworkspace/symbol-- case-insensitive substring search over ProjectIndex symbolsIndexrequest handler that buildsTUIndexfrom compiled AST and returns serialized dataenable_indexing(default true) andidle_timeout_ms(default 3000ms)Fixes and Cross-platform
decl_of()for correct Base/Derived relation emissionPathPool::intern()andProjectIndex::from()(backslash -> forward slash).gitattributes: Force LF intests/data/**to prevent CRLF byte-offset mismatches on Windows CI.clice/before each test for hermetic index stateTests
index_query_tests.cpp: unit tests for occurrence lookup, relation queries, content retrieval, removed bitmap filteringtest_index.py: E2E integration tests for GoToDefinition, FindReferences, CallHierarchy (prepare/incoming/outgoing), TypeHierarchy (prepare/supertypes/subtypes), WorkspaceSymbolTest plan