Repository navigation
refactor(semantic): flag instantiation subtrees at a single write point - #571
Merged
Merged
Conversation
The Semantics builder now sets in_instantiation on every recorded node
inside a template-instantiation subtree (the written explicit-directive
decl itself stays unflagged), and the head predicate moves to the shared
decls::is_instantiation. Consumers stop re-deriving TSK logic:
- inlay hints skip instantiation subtrees, fixing contradictory type
hints (": char" and ": int" stacked on one dependent auto) when a
template has several explicit instantiations
- document symbols and folding ranges drop their local re-derivations
- semantic tokens and the TU index projection skip flagged entries,
enforcing the documented "instantiations never produce occurrences"
contract at the walk level
hover's decl_for_comment now reuses decls::instantiated_from, so an
uninstantiated specialization documents itself with the matching partial
specialization instead of falling back to the primary template.
semantic tokens' has_logical_newline consults the Lexer-collected
comment ranges instead of re-parsing comment syntax by hand.
Function and variable explicit instantiation directives stay invisible
(mislocated by clang) until clang 23's ExplicitInstantiationDecl; the
workaround sites are tagged FIXME(explicit-instantiation) and the
current behavior is pinned by partial fixtures.
|
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:
📝 WalkthroughWalkthroughChangesTemplate instantiation support
Estimated code review effort: 4 (Complex) | ~45 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: a082464239
ℹ️ 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".
An explicit instantiation directive's template-argument TypeLocs are written code; flagging the whole subtree made semantic tokens and the index skip their references. Only the member decls beneath the directive are instantiated — track the directive node during traversal and flag member subtrees alone. Pinned by the explicit_instantiation fixture, which now paints a class-type argument.
An extern declaration and a definition of the same specialization each happen to own their own redecl on clang 21, so both directives paint their written name and arguments. The written info still lives on the specialization rather than the directive — record that fragility in the FIXME and pin today's behavior so a node-reuse change trips the snapshot; per-directive info arrives with ExplicitInstantiationDecl.
clang builds no node for a function or variable explicit instantiation directive (Sema reuses the specialization decl and discards the written declarator), so the name, template arguments and even the declarator's type paint nothing, in both the extern and the definition form. Pin all twelve identifiers as unpainted until ExplicitInstantiationDecl.
![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/semantic_tokens/explicit_instantiation_directives.cpp`:
- Around line 14-23: Remove all semantic-token markers from the
explicit-instantiation directives in the fixture, including convert, zero, their
template arguments, and declarator types. Keep both extern and non-extern forms
unchanged otherwise so the snapshot verifies that function and variable
explicit-instantiation directives remain fully unflagged.
🪄 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: c1952f83-b469-4ce4-93fc-9ebe6d87a128
📒 Files selected for processing (3)
docs/en/features/semantic-tokens.mdtests/snap/semantic_tokens/explicit_instantiation_directives.cpptests/snap/semantic_tokens/explicit_instantiation_directives.snap.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/snap/semantic_tokens/explicit_instantiation_directives.snap.yml
- docs/en/features/semantic-tokens.md
The class, function and variable directive forms fail (or work) for different reasons and will grow variants independently — static member templates, nested specializations. Rename the class fixture to match and give each form its own file: explicit_instantiation_class (the supported form), explicit_instantiation_function and explicit_instantiation_variable (the pinned blackouts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ab2ff0200
ℹ️ 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".
Treat a template as a duck-typed interface and each instantiation as an implementation: the occurrence layer deliberately classifies inside instantiated bodies, so a dependent name paints as its actual resolution and as a conflict when instantiations disagree. Pinned by the explicit_instantiation_member_bodies fixture and recorded in the template resolver design doc (go-to-implementation over these relations is planned; index-side modeling stays open). Walk-based features keep skipping instantiated subtrees — they emit location-keyed items where an instantiation only repeats the pattern. The in_instantiation flag now stays truthful for members an explicit instantiation delivers as top-level decls (is_member_specialization).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec4862b25
ℹ️ 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".
… args Two follow-ups on the instantiation-classification semantics: - combine() kept the first candidate's modifiers on agreeing kinds, so a dependent name's static modifier depended on the order the explicit instantiations were written in. Modifiers now intersect across all candidates: agreeing kinds keep what every instantiation shares. - The inlay walk skipped a class directive's whole subtree, losing hints on written template arguments (a call inside decltype). The subtree holds only written code, so walk into it; the mislocated function and variable directive forms stay skipped whole. Both pinned; the design doc now also states that only explicitly instantiated definitions are recorded today, with implicit instantiations a planned extension of the same direction.
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.
What
The per-feature migrations onto the Semantics node table left each feature re-deriving "is this decl a template instantiation" on its own, and one feature (inlay hints) not deriving it at all. This PR makes the builder the single write point for that fact and cleans up the duplication it left behind.
Instantiation flag (single write point)
in_instantiationon every node recorded inside a template-instantiation subtree. The decl created by an explicit instantiation directive is itself written and stays unflagged; an implicit instantiation head is not written, so the flag covers it too. The head predicate lives in the shareddecls::is_instantiation.: charand: int) on the same dependentauto. Fixed and pinned by a new fixture. A side effect consciously accepted: a dependentautolocal no longer picks up its type from a lone instantiated body (that only ever worked by accident); deducing it properly is tracked by the fixture'spartialstatus (clangd#2275).in_instantiationflag stays truthful for the members an explicit instantiation delivers as top-level decls.Hover comment lookup reuse
decl_for_commenthand-rolled a weaker version ofdecls::instantiated_from(itsTSK_Undeclaredfallback always chose the primary template). It now chasesinstantiated_fromto a fixed point, so an uninstantiated specialization likeFoo<int*>documents itself with the matching partial specialization's comment. Pinned for both class and variable templates.Comment scanning
Semantic tokens'
has_logical_newlinere-parsed/*...*/syntax by hand to decide where directive context ends; it now consults the comment ranges the Lexer scan already collects, so comment syntax is parsed in one place.Explicit instantiation directives (known limitation, now pinned)
Function and variable explicit instantiation directives (
template void f<int>(int);) are invisible today — no semantic token on the name, no outline entry, no occurrence — because clang mislocates them at the pattern. This is fixed upstream by llvm/llvm-project#191658 (ExplicitInstantiationDecl, clang 23); until the toolchain pin catches up, every workaround site is taggedFIXME(explicit-instantiation)and the current behavior is pinned bypartialfixtures in the semantic tokens and document symbol corpora. The class form (childless outline node, painted reference) keeps working and is pinned alongside.Tests
inlay_hint/type_conflicting_instantiations,semantic_tokens/explicit_instantiation_directives,document_symbol/kinds_explicit_instantiations, plus hover pins for class and variable template comment fallback.npm run check, docs check — all green.