feat(document symbols): specializations, aliases, macro names, semantics walk - #566
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe document-symbol collector now uses cached semantic nodes and explicit nesting state. It supports additional declaration kinds, range normalization, macro locations, and implicit-instantiation filtering. New snapshot fixtures and generated documentation cover hierarchy, symbol kinds, details, local symbols, and macro behavior. ChangesDocument symbols
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: c35e1849ab
ℹ️ 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
🤖 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/snap/document_symbol/kinds_basic.cpp`:
- Line 3: Update the heading comment in kinds_basic.cpp to state that the listed
constructs are represented in the outline, rather than claiming each maps to a
distinct LSP symbol kind. Regenerate the document-symbols snapshot documentation
in docs/en/features/document-symbols.md.
🪄 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: b92b8895-1221-4629-949a-4e1df82f473c
📒 Files selected for processing (53)
docs/en/features/document-symbols.mdsrc/feature/document_symbols.cppsrc/semantic/semantics.cpptests/snap/document_symbol/basic.cpptests/snap/document_symbol/basic.snap.ymltests/snap/document_symbol/detail_base_classes.cpptests/snap/document_symbol/detail_default_arguments.cpptests/snap/document_symbol/detail_default_arguments.snap.ymltests/snap/document_symbol/detail_multiline_signatures.cpptests/snap/document_symbol/detail_multiline_signatures.snap.ymltests/snap/document_symbol/detail_signatures.cpptests/snap/document_symbol/detail_signatures.snap.ymltests/snap/document_symbol/detail_variable_types.cpptests/snap/document_symbol/detail_variable_types.snap.ymltests/snap/document_symbol/friend_definitions.cpptests/snap/document_symbol/friend_definitions.snap.ymltests/snap/document_symbol/hierarchy_access_specifiers.cpptests/snap/document_symbol/hierarchy_anonymous.cpptests/snap/document_symbol/hierarchy_anonymous.snap.ymltests/snap/document_symbol/hierarchy_nesting.cpptests/snap/document_symbol/hierarchy_nesting.snap.ymltests/snap/document_symbol/hierarchy_selection_ranges.cpptests/snap/document_symbol/hierarchy_selection_ranges.snap.ymltests/snap/document_symbol/hierarchy_utf16.cpptests/snap/document_symbol/hierarchy_utf16.snap.ymltests/snap/document_symbol/kinds_basic.cpptests/snap/document_symbol/kinds_basic.snap.ymltests/snap/document_symbol/kinds_specializations.cpptests/snap/document_symbol/kinds_specializations.snap.ymltests/snap/document_symbol/kinds_templates.cpptests/snap/document_symbol/kinds_templates.snap.ymltests/snap/document_symbol/kinds_type_aliases.cpptests/snap/document_symbol/kinds_type_aliases.snap.ymltests/snap/document_symbol/local_symbols.cpptests/snap/document_symbol/local_symbols.snap.ymltests/snap/document_symbol/macro_argument_names.cpptests/snap/document_symbol/macro_argument_names.snap.ymltests/snap/document_symbol/macro_symbols.cpptests/snap/document_symbol/macro_symbols.snap.ymltests/snap/document_symbol/members.snap.ymltests/snap/document_symbol/misc.cpptests/snap/document_symbol/misc.snap.ymltests/snap/document_symbol/missing_includes.cpptests/snap/document_symbol/missing_macros.cpptests/snap/document_symbol/missing_modules.cpptests/snap/document_symbol/missing_pragma_mark.cpptests/snap/document_symbol/tags_deprecated.cpptests/snap/document_symbol/tags_modifiers.cpptests/snap/document_symbol/templates.cpptests/snap/document_symbol/templates.snap.ymltests/unit/feature/document_symbol_tests.cpptools/feature_docs.tstools/snap/inspect.ts
💤 Files with no reviewable changes (8)
- tests/snap/document_symbol/members.snap.yml
- tests/snap/document_symbol/basic.snap.yml
- tests/snap/document_symbol/templates.cpp
- tests/unit/feature/document_symbol_tests.cpp
- tests/snap/document_symbol/misc.snap.yml
- tests/snap/document_symbol/templates.snap.yml
- tests/snap/document_symbol/misc.cpp
- tests/snap/document_symbol/basic.cpp
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.
Third per-feature round after semantic tokens (#564) and inlay hints (#565): document symbols gets its traversal migrated onto the
Semanticsnode table, four outline bugs fixed (plus one table-level bug in the builder), and its snap corpus rewoven into 24 doc-generating fixtures.Traversal migrated onto the semantics node table
document_symbols.cppno longer runs aFilteredASTVisitorover the TU. A singleCollectorwalks the cachedunit.semantics().node_entries()— the DFS pre-order record of the interested file's written AST. Nesting comes for free: a symbol's frame stays open while the walk index is inside itssubtree_end, so the outline tree is rebuilt with a plain(subtree_end, cursor)stack, and implicit instantiations are skipped as an index jump. The migration was proven equivalent against the pre-existing snapshots and full unit suite before any behavior was changed.One behavior improvement falls out of the table's recording contract: abbreviated function templates (
void f(Concept auto x)) used to vanish from the outline entirely — the writtenFunctionDeclhides inside an implicitFunctionTemplateDecl, which the old visitor skipped wholesale. The builder records written children of implicit decls, so these functions now appear (pinned inkinds_templates).Table-level fix: structured bindings recorded twice
Namespace-scope
BindingDecls are members of the enclosingDeclContextand explicitly traversed byTraverseDecompositionDecl's bindings loop, so the builder recorded each one twice — every table consumer saw doubles (the outline showed each binding twice). The builder now records aBindingDeclonly when its walk parent is its ownDecompositionDecl. Block-scope and TU-scope bindings were already single-visit; both are pinned.Outline fixes
is_interestedlacked their decl kinds); their members were orphaned at namespace level. NowBox<void>,Box<T*>,pi<int>,pi<T*>appear with members correctly nested.typedef,usingaliases and alias templates never appeared at all. They now render with atype aliasdetail, mapped to LSPClass(matching clangd).~Widgetselected only the~;operator==andoperator boolselected onlyoperator. Selection ranges now come fromDeclarationNameInfoand cover the full written name.VAR(name)selected the macro name instead ofname. The name range now goes throughgetFileLoc, so argument-spelled names select their written spelling while body-spelled names keep the invocation site; the symbol range is widened when needed to preserve the LSP range ⊇ selection-range invariant.Corpus rewoven: 24 fixtures, doc generated from them
The 4 seed fixtures were replaced by 24 itemized fixtures with
///doc headers;document_symbolis registered infeature_docs.tsand the checklist sections ofdocs/en/features/document-symbols.mdare now generated from the corpus. Probes confirmed several checklist items are supported by construction and are now pinned: default-argument stripping (clangd#221), multiline signature ranges (clangd#2221), macro-expansion symbol locations (clangd#475), friend function definitions, local symbols inside function bodies (clangd#616), and UTF-16 column counting (CJK fixture). Unsupported items (access-specifier grouping clangd#499, base-class detail, macro/include/module/#pragma markoutline entries, symbol tags clangd#2123) are recorded as compiled-out stubs. The count-based unit tests are retired; every case they touched is pinned structurally by a snapshot, including implicit instantiations not appearing.Verification
All four suites green: unit (1168) and snap (280) on both RelWithDebInfo and Debug (LLVM assertions), integration (336), smoke (3/3),
npm run check,feature_docs.ts check. Three-way pre-PR review (correctness / style / test coverage); findings addressed: inline comment style, reuse ofdecls::is_implicit_instantiation, and three added pins (implicit instantiation absence, block-scope bindings, static data member + named nested struct).