feat(hover): show tag declaration members - #580
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo 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 (5)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThis change adds bounded hover rendering for record and enum definitions. It displays selected members, access labels, aliases, nested types, and enumerators. It limits output size and suppresses large initializers. Hover fixtures, snapshots, and feature documentation were updated. ChangesHover rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change adds localized hover summaries for tag declarations and includes passing unit and snapshot validation; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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: 3710301ff9
ℹ️ 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".
| if(has_large_initializer(var->getInit(), tb)) { | ||
| policy.SuppressInitializers = true; | ||
| } | ||
| } else if(const auto* record = llvm::dyn_cast<clang::RecordDecl>(decl)) { |
There was a problem hiding this comment.
When hovering a primary class template, decl_hover() replaces its CXXRecordDecl with the described ClassTemplateDecl before calling definition(). This branch only recognizes RecordDecl, so the template falls through to Clang's terse printer and still renders an empty body, unlike instantiated uses such as the added Box<int> test. Unwrap ClassTemplateDecl::getTemplatedDecl() here so direct hovers on template <typename T> struct Box { ... } also receive the new member summary.
Useful? React with 👍 / 👎.
| if(const auto* var = llvm::dyn_cast<clang::VarDecl>(&decl)) { | ||
| return var->isStaticDataMember(); |
There was a problem hiding this comment.
For a member such as template <class T> inline static T value, Clang places a VarTemplateDecl directly in RecordDecl::decls() and nests the static VarDecl beneath it. Since this predicate only handles direct VarDecl objects, the new record summary silently omits static member variable templates while including ordinary static members and other member templates. Recognize VarTemplateDecl here and use its templated declaration when applying initializer suppression.
Useful? React with 👍 / 👎.
| if(const auto* template_decl = llvm::dyn_cast<clang::ClassTemplateDecl>(&decl)) { | ||
| record = template_decl->getTemplatedDecl(); | ||
| } | ||
| if(record && !record->isCompleteDefinition()) { |
There was a problem hiding this comment.
When summarizing an instantiated outer template such as Outer<int>, Clang commonly leaves its nested records and nested class templates lazily uninstantiated, so their specialization-side RecordDecl reports isCompleteDefinition() == false even when the corresponding member in the primary template has a definition. This branch consequently renders those members as forward declarations instead of the intended collapsed { ... } definitions. Check the instantiated-from member declaration before classifying such nested members as forwards.
Useful? React with 👍 / 👎.
![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 `@src/semantic/display.cpp`:
- Around line 1085-1091: The decl_hover dispatch must handle primary class
templates represented by ClassTemplateDecl before the generic print fallback.
When the template has a definition, pass ClassTemplateDecl::getTemplatedDecl()
through the existing record-summary path while retaining the original template
declaration for hover header and parameter rendering, and add coverage for
hovering the primary Box template alongside Box<int>.
🪄 Autofix
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: 07353880-2ebf-463f-b084-13d936efb268
📒 Files selected for processing (5)
docs/en/features/hover.mddocs/zh/features/hover.mdsrc/semantic/display.cpptests/snap/hover/tag_decls.cpptests/snap/hover/tag_decls.snap.yml
| } else if(const auto* record = llvm::dyn_cast<clang::RecordDecl>(decl)) { | ||
| if(const auto* complete = record->getDefinition()) { | ||
| return print_record_definition(*complete, policy, tb); | ||
| } | ||
| } else if(const auto* enum_decl = llvm::dyn_cast<clang::EnumDecl>(decl)) { | ||
| if(const auto* complete = enum_decl->getDefinition()) { | ||
| return print_enum_definition(*complete, policy); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the template rewrite and definition dispatch.
rg -n -C 6 'getDescribedTemplate|display::definition' src/feature/hover.cpp
rg -n -C 8 'ClassTemplateDecl|dyn_cast<clang::RecordDecl>|print_record_definition' \
src/semantic/display.cpp
# Inspect current template hover coverage.
rg -n -C 5 'template_fields|Box<int>|template <typename T>' \
tests/snap/hover/tag_decls.cpp tests/snap/hover/tag_decls.snap.ymlRepository: clice-io/clice
Length of output: 10741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the complete definition and declaration printer for class template fallthrough.
sed -n '960,1120p' src/semantic/display.cpp
sed -n '300,380p' src/feature/hover.cpp
# Find whether the hover fixture uses the template name directly or only the specialization.
rg -n '§.*Box|hover\(.*Box|template_fields::Box|class Node|static constexpr int extent' tests/snap/hover/tag_decls.cppRepository: clice-io/clice
Length of output: 9819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for existing snapshots naming a primary class template declaration.
rg -n '### .*Box|hover .*Box|template_fields::Box|template <>|template <typename T> struct Box' tests/snap/hover/tag_decls.cpp tests/snap/hover/tag_decls.snap.ymlRepository: clice-io/clice
Length of output: 405
Render primary class templates with the record summary.
decl_hover sets decl to TemplateDecl, so a primary class template can reach definition as ClassTemplateDecl instead of RecordDecl. The dispatcher falls through to decl->print, which skips member filtering, the 20-member limit, tag templates, and initializer suppression.
When the templated record has a definition, reuse ClassTemplateDecl::getTemplatedDecl() for summary traversal while preserving the template declaration for hover header/template parameter output. Add coverage for hovering the primary Box template, not only Box<int>.
🤖 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/display.cpp` around lines 1085 - 1091, The decl_hover dispatch
must handle primary class templates represented by ClassTemplateDecl before the
generic print fallback. When the template has a definition, pass
ClassTemplateDecl::getTemplatedDecl() through the existing record-summary path
while retaining the original template declaration for hover header and parameter
rendering, and add coverage for hovering the primary Box template alongside
Box<int>.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8e02515e3
ℹ️ 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".
| } | ||
| ++count; | ||
| os << '\n'; | ||
| enumerator->print(os, policy); |
There was a problem hiding this comment.
When an enumerator has a very large explicit initializer, such as a macro-expanded expression containing hundreds of operands, this call pretty-prints the entire expression without applying the new max_initializer_tokens guard. Type-level enum hovers can therefore allocate and return an enormous definition even though variable and record-member initializers are explicitly protected from the same catastrophic case; check getInitExpr() against the token buffer and use a suppressing policy before printing.
Useful? React with 👍 / 👎.
| if(llvm::isa<clang::TagDecl, clang::ClassTemplateDecl>(*member)) { | ||
| print_nested_type(*member, policy, os); |
There was a problem hiding this comment.
When a field defines an anonymous type inline, as in struct S { struct { int x; } value; };, Clang exposes the anonymous RecordDecl and its FieldDecl as sibling declarations. This branch prints the record separately as struct { ... };, after which the field is printed with Clang's synthetic, unspellable anonymous-type name; one source member is therefore duplicated, rendered misleadingly, and counted twice toward the 20-member limit. Handle the anonymous tag and its following declarator as one member instead of independently collapsing the tag.
Useful? React with 👍 / 👎.
d8e0251 to
fdc9b17
Compare
💡 Codex Reviewclice/src/semantic/display.cpp Line 931 in fdc9b17 Bound initializer size as well as token count
When a newly summarized record member has a very large string or raw-string initializer, the literal is still only one expanded token, so this guard does not suppress it and ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 456888ceb7
ℹ️ 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".
|
Some specifics about current implementation need to be settled before ready for review. Opened #628 for discussion. |
1 participant
Summary
This PR implements type-level hovers for records and enums now include a compact declaration summary with useful members. See #628 for implementation details.