Repository navigation
refactor(semantic): serve features from a unified semantic map - #557
Merged
Merged
Conversation
…ntic map One traversal after parse claims every expanded token for its innermost AST node (same claim rules as the selection algorithm always used) and attributes it back to a spelled main-file token; preprocessor directives (macros, includes, imports) are appended as first-class SemanticNode rows owning their name tokens. The result is a flat node table (DFS pre-order, parent links, subtree extents) plus a token-to-node ownership CSR. SemanticNode flattens the recorded AST node kinds and the preprocessor entities into one tagged union - no nested discriminant, uniform dispatch. SelectionTree becomes a pure query over this map: binary-search the selected tokens, fold per-token selectedness into owning nodes, and materialize the selected ancestor chains. The per-query traversal, hit-testing and pruning machinery (SelectionTester, may_hit, maybe-selected estimation) is deleted; per-query cost drops from ~15-20us to ~1us and the map builds once per compile in ~10ms.
The resolve_facts family becomes the single implementation of node-to- decl extraction, ported verbatim from SemanticVisitor's visit methods (same decls, roles, locations - including the historical asymmetries the serialized index format pins). Semantic tokens iterate the semantic map's node rows: decl occurrences through resolve, macros as first-class rows. TUIndex becomes a projection: occurrences and relations from resolve facts over the map (whole-TU shape for the background index, the cached interested-only shape otherwise), macros from the directives. The enclosing-function context for Caller/Callee comes from the map's parent chain, replacing the traversal-time decl stack; it deliberately reproduces the old exact-FunctionDecl behavior (methods never counted as callers - a quirk to revisit with the index format). SemanticVisitor is deleted: the codebase now has one AST traversal (the Semantics builder) and one semantic extraction (resolve).
…ions
Semantic tokens now classify every spelled token in one ordered pass:
lexical kinds straight from the token kind (the spelled stream has real
keyword resolution - the hand-rolled lexer pass is gone, a slim raw scan
only collects comments), macros/includes/imports/attributes from the
owning SemanticNode, and declaration names by collecting decls anchored
at the token from its owner chain. Conflicts settle on the spot and
adjacent runs merge on emission, so the sort-and-merge pass disappears.
Tokens inside macro definition bodies keep lexical kinds only; semantic
highlighting of expansions belongs to the future expansion-preview
feature. Under a PCH the preamble's directives live in the PCH compile,
so a directive-context state machine covers e.g. #define names there.
resolve_facts' callback pair is gone: resolve_occurrences returns its
occurrences by value and lives in semantics.{h,cpp}; the relation facts
only the index consumes are inlined into its Projector. Two historical
index bugs are fixed on the way: calls inside methods now produce
Caller/Callee rows (the old traversal stack missed them), and using
declaration relation rows are keyed by the referenced decl rather than
the using decl itself.
Remove the visitor-era shapes instead of patching around them: the trivial modifier/encoding wrappers and the dead comment branch are gone, decl classification is a plain value-returning function, and the token collector splits into lexical and semantic classification that read top-to-bottom. The index projector loses the handleXxx naming and the four-way kind dispatch inside handleRelation: each call site now uses the shape-specific adder it means (self-relation mirror, symbol pair, call edge), so the unreachable branch disappears.
Three regression tests for behavior the migration fixed or changed: calls inside method bodies produce Caller/Callee rows, using-decl relation rows share the occurrence's symbol key, and macro definition bodies keep lexical-only highlighting (the expansion-preview feature owns semantic tokens for expansions).
The incremental-parse doc now states the boundary the preamble state blob implements: mutable content answers from the live AST, immutable content from results precomputed while its one live AST existed, merged per feature - which is also why PCH state never feeds the main file's semantic map, and how user-chosen cache bounds extend naturally. The symbol-index doc gains the read-only mode direction: serving unmodified files straight from the disk index with no AST or PCH, sliding between snapshot and live session as content mutability changes.
Address the pre-PR review: missing standard includes, a stale node-kind list and parameter name in the selection docs, the m_root member prefix, and a dead structured binding. Strengthen BaseAndDerived from a soft kind-collection into hard Base/Derived edge assertions, and pin the previously untested projection paths: TypeDefinition rows for fields, aliases and enum constants, constructor/destructor ownership, macro definition and reference rows, module name indexing, angled include merging and conditional directive classification.
📝 WalkthroughWalkthroughChangesUnified semantic map migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/feature/semantic_tokens.cpp (2)
499-509: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoalescing ignores
modifiers, so adjacent same-kind tokens can silently lose them.
emitmerges onlast.kind == kindonly. Any two adjacent ranges with the same kind but different modifier masks collapse into the first token's modifiers. Comparing modifiers as well keeps the intended directive/header/comment merges while making the invariant explicit.♻️ Proposed tightening
void emit(LocalSourceRange range, SymbolKind kind, std::uint32_t modifiers) { if(!tokens.empty()) { auto& last = tokens.back(); - if(last.range.end == range.begin && last.kind == kind) { + if(last.range.end == range.begin && last.kind == kind && + last.modifiers == modifiers) { last.range.end = range.end; return; } }🤖 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/semantic_tokens.cpp` around lines 499 - 509, Update emit so adjacent ranges are coalesced only when their kinds and modifier masks both match, adding modifiers equality to the existing last.range.end and last.kind checks. Preserve the current merge behavior and token insertion for all other cases.
381-439: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid resolving ancestor occurrences in every owner chain walk.
Semantics::owners(index)already returns the node(s) that claim the token; the rest of the chain walk is mainly for preprocessor/import/Attrhandling. In thedefaultbranch, walking every ancestor and callingresolve_occurrences(node)can resolve the same ancestor declaration repeatedly for child tokens. Restrict declaration-name resolution to the exact owner/token where it is anchored, or cache resolved occurrences per node if ancestors are needed.🤖 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/semantic_tokens.cpp` around lines 381 - 439, Update classify_semantic so the default declaration-resolution path only calls resolve_occurrences for the exact owner/token anchor, rather than for every ancestor visited in the parent chain. Preserve the existing ancestor walk for MacroDefine, MacroReference, MacroUndef, Include, Import, and Attr handling, and ensure declaration occurrences are not repeatedly resolved for child tokens.src/semantic/semantics.h (1)
220-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
implicitis never set by the builder — document or drop it.
SemanticsBuilder::push(src/semantic/semantics.cpp:550-556) constructs nodes as{node, stack.back()}, so both flags stayfalsefor every node.in_instantiationsays "Reserved";implicitreads as if it were populated, which invites consumers to branch on a value that is alwaysfalse.♻️ Mark it reserved too (or remove until populated)
struct NodeFlags { /// The node is implicit (e.g. an implicit cast wrapper kept for - /// parent chains). + /// parent chains). Reserved: the builder does not set this yet. bool implicit : 1 = false;🤖 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/semantic/semantics.h` around lines 220 - 228, Update the NodeFlags::implicit declaration and its surrounding documentation to mark it as reserved and not currently populated, matching in_instantiation, or remove the flag until SemanticsBuilder::push initializes it. Keep the change scoped to preventing consumers from treating the always-false value as meaningful.src/semantic/semantics.cpp (1)
39-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
macro_slotto the variant with astatic_assert.The
9and the+ 2encode the header's alternative order. If anyone reordersSemanticNode::storageorKind, every kind after the macro slot silently shifts (anIncludenode would report asAttr) with no build error, and every consumer switching onkind()misbehaves.♻️ Make the coupling a compile-time check
/// The variant alternatives before the macro slot map 1:1 onto Kind; the /// slots after it are shifted by the two extra macro kinds. constexpr std::size_t macro_slot = 9; + static_assert(std::variant_size_v<decltype(storage)> == macro_slot + 3, + "SemanticNode::storage layout changed; revisit kind() mapping"); auto index = storage.index();Plus an assertion that alternative
macro_slotisconst MacroRef*(e.g. viastd::variant_alternative_t).🤖 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/semantic/semantics.cpp` around lines 39 - 47, In the kind-mapping function containing macro_slot, add compile-time assertions tying the hard-coded slot to SemanticNode::storage: verify that alternative macro_slot is const MacroRef* and that the expected variant/Kind ordering remains intact, while preserving the existing index conversion behavior.
🤖 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 145-153: Remove the hard assertion on fid == def_fid in the
decl/definition handling around unit.decompose_expansion_range; only call
relation.set_definition_range when the decomposed file matches fid, and
otherwise skip the range while preserving the self-relation.
- Around line 491-536: Update TUIndex::build and project_semantics so
Semantics::build(unit, false) is invoked only when the resulting index requires
full-TU semantics. Add the targeted full-index guard around the transient full
construction, while continuing to reuse unit.semantics() for interested-only
paths and preserving existing projection behavior.
In `@src/semantic/selection.cpp`:
- Around line 176-265: The selection tests must cover nodes whose owned tokens
are only partially selected, distinguishing SelectionTree::Partial from
SelectionTree::Complete. Add at least one case in selection_tests.cpp that
exercises both coverage outcomes, and retain or add a FIXME for macro-expanded
ownership if that remains outside the current target.
In `@src/semantic/semantics.cpp`:
- Around line 1145-1173: Update nns_occurrences to remove the unavailable
NestedNameSpecifier::NamespaceAlias case and getAsNamespaceAlias() usage for
Clang 22.1.8. Handle namespace aliases through the supported
getAsNamespace()/NamespaceBaseDecl path while preserving the existing reference
occurrence and source-location behavior.
- Around line 762-769: Update first_token_at so it does not dereference the
iterator returned by std::ranges::partition_point when it equals the iota end;
compute and return the corresponding index from that iterator instead,
preserving the existing i < count boundary behavior.
In `@tests/unit/index/tu_index_tests.cpp`:
- Around line 294-295: Guard every select(...).front() dereference with a
non-empty assertion: in tests/unit/index/tu_index_tests.cpp lines 294-295,
assert select("base") and select("derived"); at lines 455-456, assert
select(source) and select(target) inside has_type_definition; and at lines
484-485, assert select("s") and select("ctor").
---
Nitpick comments:
In `@src/feature/semantic_tokens.cpp`:
- Around line 499-509: Update emit so adjacent ranges are coalesced only when
their kinds and modifier masks both match, adding modifiers equality to the
existing last.range.end and last.kind checks. Preserve the current merge
behavior and token insertion for all other cases.
- Around line 381-439: Update classify_semantic so the default
declaration-resolution path only calls resolve_occurrences for the exact
owner/token anchor, rather than for every ancestor visited in the parent chain.
Preserve the existing ancestor walk for MacroDefine, MacroReference, MacroUndef,
Include, Import, and Attr handling, and ensure declaration occurrences are not
repeatedly resolved for child tokens.
In `@src/semantic/semantics.cpp`:
- Around line 39-47: In the kind-mapping function containing macro_slot, add
compile-time assertions tying the hard-coded slot to SemanticNode::storage:
verify that alternative macro_slot is const MacroRef* and that the expected
variant/Kind ordering remains intact, while preserving the existing index
conversion behavior.
In `@src/semantic/semantics.h`:
- Around line 220-228: Update the NodeFlags::implicit declaration and its
surrounding documentation to mark it as reserved and not currently populated,
matching in_instantiation, or remove the flag until SemanticsBuilder::push
initializes it. Keep the change scoped to preventing consumers from treating the
always-false value as meaningful.
🪄 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 Plus
Run ID: 54ae4200-2758-4c75-ae35-83f293618c1e
📒 Files selected for processing (15)
docs/zh/design/incremental-parse.mddocs/zh/design/symbol-index.mdsrc/compile/compilation_unit.cppsrc/compile/compilation_unit.hsrc/compile/implement.hsrc/feature/semantic_tokens.cppsrc/index/preamble_state.hsrc/index/tu_index.cppsrc/semantic/selection.cppsrc/semantic/selection.hsrc/semantic/semantic_visitor.hsrc/semantic/semantics.cppsrc/semantic/semantics.htests/unit/feature/semantic_tokens_tests.cpptests/unit/index/tu_index_tests.cpp
💤 Files with no reviewable changes (1)
- src/semantic/semantic_visitor.h
![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: a54d90ae27
ℹ️ 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".
Names written as macro arguments regained their semantic highlighting: occurrence locations from expansions now normalize to their spelling before matching the spelled token (with a regression test). The include filename context ends at the filename instead of the line end, escaped newlines no longer terminate directive context, and alternative operator spellings (and, or, not) classify as keywords again. The whole-TU Semantics shape skips the token-claim machinery entirely - only the index projection consumes it and never queries ownership; the preamble-state build paid that cost on every PCH build, which is the likely source of the Debug CI timeouts. The cross-file definition-range assert becomes a guard (the old code silently stored a wrong-file range in release builds), iota end iterators are no longer dereferenced, and selection gains Partial/Complete coverage tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e77c09afe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A macro expanding to a ~32k-term expression attributes every produced node to one spelled invocation token, which made three consumers degrade: semantic tokens walked every owner's ancestor chain per token (quadratic), selection materialized ancestor chains recursively (stack overflow on a tight worker stack), and call-edge projection rescanned the same ancestors for every call. Semantic tokens now precompute per-token classification in one pass over the node table, selection materializes chains iteratively, and the enclosing-function lookup memoizes with path compression. The DeepExpressionChain test now also runs semantic tokens and a selection at the invocation on the tight stack. Also from review: directive context survives multi-line block comments, the global module fragment keyword classifies again, and imports spelled through macros keep their module-name tokens (locations normalize to their spelling).
16bit-ykiko
changed the title
16bit-ykiko
added a commit
that referenced
this pull request
Jul 29, 2026
…558) ## Background clice inherited two parallel answers to "which declaration does this token name": - `find_target` — a per-node targeting helper ported from clangd, driven by `HeuristicResolver` (clangd's shallow dependent-name resolver: direct lookup in the primary template, no partial specializations, no argument deduction, no typedef-chain expansion). Hover was its only consumer. - clice's own `TemplateResolver` — a deep dependent-name resolver built on Sema pseudo-instantiation (tracks parameter propagation, matches partial specializations, expands typedef chains). It was implemented long ago but **never wired into any feature**. #557 replaced the per-query SelectionTree machinery with the unified per-compilation `Semantics` map: one AST traversal, one extraction function (`resolve_occurrences`) serving hover, semantic tokens and the index. That left find_target as the last private AST walker — redundant machinery on a weaker resolver. This PR finishes the story: find_target and the HeuristicResolver dependency are deleted, and `TemplateResolver` is activated as the single dependent-name authority for every consumer of the semantic map. ## What changed - **`resolve_occurrences` takes a `TemplateResolver*`**: dependent constructs (`DependentNameType`, `DependentTemplateSpecializationType`, dependent nested-name specifiers, `DependentScopeDeclRefExpr`, `UnresolvedUsing{Value,Typename}Decl`) resolve through pseudo-instantiation and emit `WeakReference` occurrences. All three consumers pass the unit's resolver, so the index and semantic tokens now see dependent names too — not just hover. - **`TemplateResolver::lookup(CXXDependentScopeMemberExpr)` is now real** (it was a hardcoded empty stub): the base type is resolved through pseudo-instantiation — including unwrapping the injected class name for `this` — and the member is looked up in the resolved record. `this->foo()` inherited from `Base<T>` resolves again, and deeper than HeuristicResolver ever did. - **Hover reads the map**: a hover-local `decls_at` walks the touched token's owner and ancestor chain and collects every declaration whose occurrence sits on that token — the formal answer to "one position, several declarations". `find_target.{h,cpp}` deleted. - **Extraction coverage completed** (review-driven, each pinned by a fixture or unit test): ctor-initializer members, designated-initializer fields, rewritten comparison operators, CTAD placeholders, injected class names, `UsingType`, using-shadow unwrapping (declaration site and overload sets), labels, template-template arguments, `sizeof...(pack)`, class-provided `operator new`/`delete` (anchored on the keyword even when `::`-qualified), constructor references on construction punctuation, `__super`, namespace-alias-preserving using-directives, `UnresolvedMemberExpr` candidate sets. ## Hardening surfaced by the migration - **Windows worker crashes (0xC0000005) root-caused to the resolver's TreeTransform derivatives**: they never overrode `getBaseLocation()` (the base-class `setBase` is a no-op CRTP trap), so every synthesized `TypeLoc` carried invalid locations, and `TypeLocBuilder::push` records were left fully uninitialized — garbage qualifier pointers that crashed any later `getSourceRange()` (Debug builds hit the equivalent Sema assertions). All transformers now carry a valid base location and every pushed record is initialized. - **Speculative diagnostics no longer leak**: resolver lookups on real-world headers emitted error-level diagnostics into the unit (and even tripped Sema's error limit). Lookups now run under a consumer swap; error *counting* deliberately still accumulates — per-lookup resets hand every pathological instantiation chain a fresh budget, which measures as minutes of resolver time on STL-heavy TUs. The trade-off is documented in code; a resolver-owned work budget is planned follow-up work. - **Integration workspace lock is now FIFO**: the mkdir-poll lock let a worker whose tests run back-to-back re-acquire within microseconds while cross-process waiters polled on a 100 ms clock — structural starvation behind the long-standing `socket mode connects` CI timeouts. A ticket queue (dead-owner tickets self-clean) restores fairness. ## Cost Full-TU indexing of an STL-heavy TU: ~80 ms with the resolver vs ~55 ms without (warm) — the resolver only engages on dependent constructs and caches per node. The interactive path is unaffected: per-edit, main-file-only indexing stays sub-millisecond. ## Testing All previously failing hover fixtures reproduce their old snapshots byte-identically; no snapshot was regenerated over a divergence. New coverage: dependent-name and dependent-member hover fixtures pinning the resolver end to end, plus index unit tests for the new relation rows. All four suites pass locally in both configurations (unit 1064, snap 76, integration 332, smoke).
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
This PR introduces
Semantics, a unified per-compilation semantic map, and rebuilds three features on top of it: selection, semantic tokens and the TU index. The codebase now has exactly one AST traversal (the map builder) and one node-to-decl extraction (resolve_occurrences), replacing the per-querySelectionTreemachinery and the CRTPSemanticVisitorthat previously duplicated this logic across consumers.Design
Semanticsis built once after a successful parse and stores only structure:SemanticNoderows owning their name tokens;Everything semantic is derived on demand from the live AST; the map never stores decls or relations. Consumers take the shape natural to them:
Behavior changes
FunctionDecls, so methods never counted as callers.All are pinned by regression tests, including a hardened
DeepExpressionChainthat now runs indexing, semantic tokens and a selection at a ~32k-term macro expansion on a deliberately tight stack — the shapes that previously degraded quadratically or recursed per AST level.Docs
The design docs record the boundary this settles: mutable content is served by the live AST's semantic map, immutable content (the preamble under a PCH) by results precomputed at PCH build time and merged per feature — plus the future read-only mode where unmodified files are served straight from the disk index.
Testing
Unit 1059 / snap 76 / integration 332 / smoke 3, all green across native (incl. Debug+ASan), cross (linux-arm64, macos-x64, windows-arm64) and editor pipelines.