Repository navigation
refactor(semantic): rewrite TemplateResolver without Sema - #560
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:
📝 WalkthroughWalkthroughTemplate resolution is refactored around ASTContext-based pseudo-instantiation and structural template unification. Dependent lookup, specialization selection, caching, recursion guards, call filtering, compilation wiring, and resolver regression tests are updated. ChangesTemplate resolution overhaul
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 💡 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
clice/src/semantic/resolver.cpp
Line 1555 in 30f461d
Substitute each overloaded operator-arrow return type
When operator-> belongs to a class template, its declared return type still contains that class's template parameters. The successful instantiator.lookup leaves the required bindings in that instantiator, but this assignment copies the raw return type and then destroys the instantiator; for template<class T> struct Ptr { T* operator->(); };, looking up p->member on Ptr<X> therefore continues with T* instead of X* and cannot find X::member. Resolve or substitute the return type while the lookup instantiator and its deduction frames are still alive.
ℹ️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/semantic/template_resolver_tests.cpp (1)
666-668: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale TODO blocks now contradict the new tests.
These commented-out placeholders describe capabilities this PR added or machinery it removed:
- Line 666-668 cites
TransformNestedNameSpecifierLoc;TreeTransformis gone.- Line 784-786 claims
checkTemplateArgumentsonly fills type defaults — NTTP and template-template defaults are now filled, andNttpDefaultArgumentcovers it.- Line 828-830 says specializing on
falseneeds unsupported NTTP matching, butConditionalFalseType(Line 2101) does exactly that.- Line 832-833 says template-template deduction is unsupported, contradicted by
TemplateTemplateReplace,TemplateSwapArgs, andTemplateThroughLayer.Dropping them avoids future contributors reading them as current limitations.
Also applies to: 784-786, 828-833, 906-907
🤖 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 `@tests/unit/semantic/template_resolver_tests.cpp` around lines 666 - 668, Remove the stale commented-out TODO blocks in the semantic template resolver tests, including the blocks around NestedClassTemplate, checkTemplateArguments, ConditionalFalseType, template-template deduction, and the additional block around lines 906-907. These comments describe limitations that the current tests and implementations no longer have, so delete the obsolete placeholders without changing the active tests.
🧹 Nitpick comments (2)
src/semantic/resolver.cpp (2)
389-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowing
namein the DTST branch.Line 409's
auto name = template_name.getName().getIdentifier()shadows thenameparameter that lines 425/431 still rely on. It is correct today, but any future edit that moves code across the block boundary silently changes which name is looked up.♻️ Suggested rename
auto& template_name = DTST->getDependentTemplateName(); - auto name = template_name.getName().getIdentifier(); - if(!name) { + auto identifier = template_name.getName().getIdentifier(); + if(!identifier) { return {}; } - if(auto decl = preferred(lookup(template_name.getQualifier(), name))) { + if(auto decl = preferred(lookup(template_name.getQualifier(), identifier))) { TD = decl; args = DTST->template_arguments(); }🤖 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/resolver.cpp` around lines 389 - 418, Rename the local identifier variable in the DependentTemplateSpecializationType branch of lookup, currently declared from template_name.getName().getIdentifier(), so it no longer shadows the lookup function’s name parameter. Update its uses in that branch while preserving the name parameter used by the later lookup logic.
844-862: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding
MemberPointerto the rewrite whitelist.
FunctionProtois rewritten butMemberPointerTypeis not, soT C::*/R (C::*)(Args...)patterns fall intodefaultand stay unsubstituted — exactly the shape thecallback_traitsregression test exercises. Rewriting pointee + class type would extend coverage for the common member-pointer traits idiom.♻️ Sketch
+ case clang::Type::MemberPointer: { + auto MPT = llvm::cast<clang::MemberPointerType>(T); + auto pointee = rewrite(MPT->getPointeeType(), policy); + /// Note: check the qualifier/class accessor shape for the + /// pinned clang version before wiring this up. + if(pointee != MPT->getPointeeType()) { + result = context.getMemberPointerType(pointee, + MPT->getQualifier(), + MPT->getMostRecentCXXRecordDecl()); + } + break; + } + default: { break; }🤖 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/resolver.cpp` around lines 844 - 862, Extend the type-rewrite switch alongside FunctionProto to handle clang::Type::MemberPointer. In the MemberPointer branch, recursively rewrite both the member-pointer pointee type and its class type, then rebuild the MemberPointerType when either changes; otherwise preserve the original type and existing rewrite 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/semantic/resolver.cpp`:
- Around line 1482-1489: Update the unresolved DTST fallback path in the
resolver logic to avoid inserting the fallback into shared resolved via
resolved.try_emplace when the result may be caused by step_budget exhaustion or
the CTD recursion guard. Return the fallback without caching it, consistent with
resolve_dependent_name and the scope_lacks handling of uncertain results.
---
Outside diff comments:
In `@tests/unit/semantic/template_resolver_tests.cpp`:
- Around line 666-668: Remove the stale commented-out TODO blocks in the
semantic template resolver tests, including the blocks around
NestedClassTemplate, checkTemplateArguments, ConditionalFalseType,
template-template deduction, and the additional block around lines 906-907.
These comments describe limitations that the current tests and implementations
no longer have, so delete the obsolete placeholders without changing the active
tests.
---
Nitpick comments:
In `@src/semantic/resolver.cpp`:
- Around line 389-418: Rename the local identifier variable in the
DependentTemplateSpecializationType branch of lookup, currently declared from
template_name.getName().getIdentifier(), so it no longer shadows the lookup
function’s name parameter. Update its uses in that branch while preserving the
name parameter used by the later lookup logic.
- Around line 844-862: Extend the type-rewrite switch alongside FunctionProto to
handle clang::Type::MemberPointer. In the MemberPointer branch, recursively
rewrite both the member-pointer pointee type and its class type, then rebuild
the MemberPointerType when either changes; otherwise preserve the original type
and existing rewrite behavior.
🪄 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: 8e3db143-81ae-4fbf-9392-ff318192c408
📒 Files selected for processing (6)
src/compile/compilation.cppsrc/semantic/resolver.cppsrc/semantic/resolver.hsrc/semantic/unifier.cppsrc/semantic/unifier.htests/unit/semantic/template_resolver_tests.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/semantic/resolver.cpp (2)
1634-1641: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not select the first unqualified template overload.
Line 1639 returns one declaration based on iteration order when overload resolution is explicitly unsupported. That can fabricate a wrong candidate; return unresolved instead, consistent with the PR’s stated fallback contract.
🤖 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/resolver.cpp` around lines 1634 - 1641, Update the unqualified overload handling in the resolver so it does not iterate through expr->decls() or construct a lookup_result from the first clang::TemplateDecl. When overload resolution is unsupported for these non-contiguous candidates, return the unresolved result required by the fallback contract.
1569-1579: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve bindings while following overloaded
operator->.Each loop iteration destroys the
PseudoInstantiatorthat deduced bindings foroperator->. A return type likeT*fromsmart_ptr<T>::operator->()is then handled by a new instantiator withTunbound, so the final member lookup fails. Reuse one instantiator and resolve the method return type before continuing.Proposed fix
+ PseudoInstantiator instantiator(context, resolved); + if(arrow) { ... - PseudoInstantiator instantiator(context, resolved); const clang::CXXMethodDecl* method = nullptr; for(auto* candidate: instantiator.lookup(type, arrow_name)) { ... } - type = method->getReturnType(); + type = instantiator.resolve(method->getReturnType()); if(type.isNull()) { return {}; } } } - PseudoInstantiator instantiator(context, resolved); return instantiator.lookup(type, name);Also applies to: 1593-1594
🤖 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/resolver.cpp` around lines 1569 - 1579, Reuse a single PseudoInstantiator across the operator-> resolution loop instead of constructing one per iteration, preserving template bindings between chained lookups. In the loop around instantiator.lookup(type, arrow_name), resolve the selected method’s return type through that same instantiator before assigning the next type, including the corresponding path near the additional referenced location.
🤖 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.
Outside diff comments:
In `@src/semantic/resolver.cpp`:
- Around line 1634-1641: Update the unqualified overload handling in the
resolver so it does not iterate through expr->decls() or construct a
lookup_result from the first clang::TemplateDecl. When overload resolution is
unsupported for these non-contiguous candidates, return the unresolved result
required by the fallback contract.
- Around line 1569-1579: Reuse a single PseudoInstantiator across the operator->
resolution loop instead of constructing one per iteration, preserving template
bindings between chained lookups. In the loop around instantiator.lookup(type,
arrow_name), resolve the selected method’s return type through that same
instantiator before assigning the next type, including the corresponding path
near the additional referenced location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b088785e-a501-4ec8-bd21-c6ef5effcd14
📒 Files selected for processing (2)
src/semantic/resolver.cpptests/unit/semantic/template_resolver_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/semantic/template_resolver_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f004e4261
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1419d0c968
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/semantic/unifier.cpp (1)
317-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOuter-parameter pinning gap for NTTP expression arguments.
The
Typecase (delegates tounify(QualType, QualType), which now falls through to structural comparison for a depth-mismatchedTemplateTypeParmType) and theTemplatecase (lines 369-379, falls through tohasSameTemplateNamefor a depth-mismatchedTemplateTemplateParmDecl) both correctly treat an enclosing template's already-substituted-but-still-symbolic parameter as concrete rather than a wildcard. TheExpressioncase here does not: whenreferenced_nttp(...)finds an NTTP whose depth doesn't match, it unconditionallyreturn true;, treating it as a non-deduced context instead of falling through to a structural check (e.g.default:'sstructurallyEquals).This only manifests when an outer NTTP itself is still symbolic at unify-time (not yet substituted to a literal
Integral— e.g. a 3+-level-deep NTTP chain wherededuce_template_arguments's pre-substitution step propagates anExpression-kind bound value rather than a resolved literal). In that case, a partial specialization pattern that pins an outer NTTP (analogous toInner<pair<O, U>>but for a non-type parameter) would incorrectly match any argument value for the pinned NTTP, silently selecting the wrong partial specialization — directly undermining this commit's stated "outer-parameter pinning" goal for the one case it doesn't cover.🐛 Suggested fix: fall through instead of unconditionally accepting
if(auto NTTP = referenced_nttp(pattern.getAsExpr()); NTTP && NTTP->getDepth() == depth) { auto bound = argument; if(argument.getKind() == clang::TemplateArgument::Expression) { auto expr = argument.getAsExpr(); if(!expr->isValueDependent()) { if(auto value = expr->getIntegerConstantExpr(context)) { bound = clang::TemplateArgument(context, *value, NTTP->getType()); } } } if(expanding && NTTP->isParameterPack()) { return collect(NTTP->getIndex(), bound); } return bind(NTTP->getIndex(), bound); } - return true; + /// A bare reference to an out-of-depth (already-pinned) NTTP is + /// concrete from this deduction's point of view; only genuinely + /// non-deduced expressions fall through as wildcards. + if(NTTP) { + auto lhs = context.getCanonicalTemplateArgument(pattern); + auto rhs = context.getCanonicalTemplateArgument(argument); + return lhs.structurallyEquals(rhs); + } + return true;Consider also adding a regression test mirroring
OuterParamMismatch/OuterParamMatchbut with a pinned outer NTTP instead of a type, to lock in the intended behavior.🤖 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/unifier.cpp` around lines 317 - 349, Update TypeUnifier::unify’s TemplateArgument::Expression case so a referenced NTTP with a depth different from depth falls through to the existing structural comparison path instead of unconditionally returning true; preserve deduction and binding for matching-depth NTTPs. Add a regression test covering mismatched and matching pinned outer NTTP expression arguments, analogous to OuterParamMismatch/OuterParamMatch.
🤖 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.
Outside diff comments:
In `@src/semantic/unifier.cpp`:
- Around line 317-349: Update TypeUnifier::unify’s TemplateArgument::Expression
case so a referenced NTTP with a depth different from depth falls through to the
existing structural comparison path instead of unconditionally returning true;
preserve deduction and binding for matching-depth NTTPs. Add a regression test
covering mismatched and matching pinned outer NTTP expression arguments,
analogous to OuterParamMismatch/OuterParamMatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ac4634a-56b4-46c2-bf9b-cb5a6605cf3a
📒 Files selected for processing (3)
src/semantic/resolver.cppsrc/semantic/unifier.cpptests/unit/semantic/template_resolver_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b91b38237
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/semantic/resolver.cpp (2)
1471-1486: 🚀 Performance & Scalability | 🔵 Trivial
any_specialization_declaresscans every partial + every registered specialization of the template on each probe.This runs
definition->lookup(name)/lookup_in_basesoverCTD->getPartialSpecializations()andCTD->specializations()(which includes implicit instantiations elsewhere in the TU) insidescope_lacks, itself invoked fromsatisfies_pattern's per-partial SFINAE probing. For heavily-instantiated templates (e.g. standard-library containers with many implicit specializations across a large TU), this is unbounded extra work per probe and isn't counted againststep_budget. Worth keeping an eye on for compile/analysis-time regressions on real-world codebases; a small per-(CTD, name)memoization would bound the cost if it turns out to matter in practice.🤖 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/resolver.cpp` around lines 1471 - 1486, The any_specialization_declares scan can repeat expensive lookups for the same (CTD, name) during scope_lacks probes. Add small per-(ClassTemplateDecl, DeclarationName) memoization around any_specialization_declares, reusing cached results before scanning partial and registered specializations while preserving the existing declaration and base lookup behavior.
1362-1429: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
member_absent'sDependentTemplateSpecializationcase doesn't recurse into its own template arguments.The sibling
TemplateSpecializationcase (lines 1391-1402) walkstemplate_arguments()looking for nested absent members; theDependentTemplateSpecializationcase (1380-1389) only checks the qualifier chain and the DTST's own name, neverDTST->template_arguments(). A probe likevoid_t<typename A::template rebind<typename Missing::type>>won't detect thatMissing::typeis provably absent, so this branch stays "Unknown" (safe direction, no wrong verdict) but misses a legitimate prune that the analogous case already supports.♻️ Proposed fix
case clang::Type::DependentTemplateSpecialization: { auto DTST = llvm::cast<clang::DependentTemplateSpecializationType>(T); auto& template_name = DTST->getDependentTemplateName(); auto identifier = template_name.getName().getIdentifier(); auto qualifier = template_name.getQualifier(); if(specifier_absent(qualifier, guard)) { return true; } - return identifier && scope_lacks(qualifier, identifier); + if(identifier && scope_lacks(qualifier, identifier)) { + return true; + } + for(auto& argument: DTST->template_arguments()) { + if(argument.getKind() == clang::TemplateArgument::Type && + member_absent(argument.getAsType(), guard + 1)) { + return true; + } + } + return 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/resolver.cpp` around lines 1362 - 1429, Update member_absent’s DependentTemplateSpecialization branch to also recurse through DTST->template_arguments(), using the same type-argument traversal and guard increment as the TemplateSpecialization branch. Preserve the existing qualifier and identifier checks, and return true when any nested argument contains a provably absent member.
🤖 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/resolver.cpp`:
- Around line 805-823: The UnaryTransform handling in the resolver should not
construct a concrete no-op via context.getUnaryTransformType for non-enum kinds.
Preserve the original dependent UnaryTransformType, or otherwise rebuild only
the dependent form, while retaining the existing EnumUnderlyingType resolution;
add coverage proving a non-enum transform rewritten to a concrete base remains
semantically intact.
---
Nitpick comments:
In `@src/semantic/resolver.cpp`:
- Around line 1471-1486: The any_specialization_declares scan can repeat
expensive lookups for the same (CTD, name) during scope_lacks probes. Add small
per-(ClassTemplateDecl, DeclarationName) memoization around
any_specialization_declares, reusing cached results before scanning partial and
registered specializations while preserving the existing declaration and base
lookup behavior.
- Around line 1362-1429: Update member_absent’s DependentTemplateSpecialization
branch to also recurse through DTST->template_arguments(), using the same
type-argument traversal and guard increment as the TemplateSpecialization
branch. Preserve the existing qualifier and identifier checks, and return true
when any nested argument contains a provably absent member.
🪄 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: dd59fc50-d842-4b0e-abb8-9b6e4a376dcc
📒 Files selected for processing (3)
src/semantic/resolver.cppsrc/semantic/unifier.cpptests/unit/semantic/template_resolver_tests.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/semantic/resolver.cpp (2)
1763-1774: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSubstitute the selected
operator->return type before discarding its frames.The lookup frame binds the smart pointer’s template arguments, but
method->getReturnType()is copied unchanged. Fortemplate<class T> S { T* operator->(); },S<X>::operator->()remainsT*, so the next hop never sees a pointer.Proposed fix
- type = method->getReturnType(); + type = instantiator.substitute(method->getReturnType());🤖 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/resolver.cpp` around lines 1763 - 1774, Update the operator-> resolution flow around PseudoInstantiator and the selected CXXMethodDecl so method->getReturnType() is substituted using the lookup frame’s bound template arguments before assigning it to type. Preserve the existing method selection and null-return handling, ensuring templated returns such as T* become the instantiated type for the next hop.
1555-1614: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKey shared resolution caching by instantiation bindings.
resolvedis shared across resolver calls, but these AST nodes belong to generic definitions and can be revisited under differentInstantiationStackbindings. For example, resolvingapply<A, X>::typeandapply<A, Y>::typecan cache the definition-siteF<T>::typeasX, then incorrectly reuse it forY. Use a cache key that includes the relevant frame bindings, or avoid caching context-dependent nodes.
src/semantic/resolver.cpp#L1555-L1614: make dependent-name cache lookup/insertion binding-aware.src/semantic/resolver.cpp#L1631-L1688: make DTST cache eligibility/key binding-aware.src/semantic/resolver.cpp#L485-L506: do not reuse nested-name-specifier resolutions across different bindings.src/semantic/resolver.cpp#L442-L447: do not treat a node-only cached DTST fallback as context-independent.🤖 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/resolver.cpp` around lines 1555 - 1614, Make resolver caches binding-aware so context-dependent AST nodes are not reused across different InstantiationStack bindings. In src/semantic/resolver.cpp:1555-1614, include relevant frame bindings in dependent-name cache lookup/insertion or skip caching; in 1631-1688, apply the same eligibility/key handling to DTST caching; in 485-506, prevent nested-name-specifier resolutions from being reused across bindings; and in 442-447, do not treat node-only cached DTST fallbacks as context-independent.src/semantic/unifier.cpp (1)
460-479: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep repeated pack references synchronized across one expansion element.
pair<Us, Us>...deduces using the same element for bothUsoccurrences;type_list<pair<Us, Us>...>should matchtype_list<pair<X, X>, pair<Y, Y>>withUs={X, Y}but nottype_list<pair<X, Y>, pair<Y, Y>>. Collect/deduce into per-element pack bindings, validate repeated uses, then append one value per pack parameter.🤖 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/unifier.cpp` around lines 460 - 479, Update the pack-expansion handling around inner and elements so each expansion element uses a separate binding scope, ensuring repeated references to the same pack parameter unify to the same value within that element. Validate all repeated uses before committing results, then append exactly one deduced value per pack parameter for each element; preserve matching for consistent pairs and reject inconsistent ones.
🤖 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.
Outside diff comments:
In `@src/semantic/resolver.cpp`:
- Around line 1763-1774: Update the operator-> resolution flow around
PseudoInstantiator and the selected CXXMethodDecl so method->getReturnType() is
substituted using the lookup frame’s bound template arguments before assigning
it to type. Preserve the existing method selection and null-return handling,
ensuring templated returns such as T* become the instantiated type for the next
hop.
- Around line 1555-1614: Make resolver caches binding-aware so context-dependent
AST nodes are not reused across different InstantiationStack bindings. In
src/semantic/resolver.cpp:1555-1614, include relevant frame bindings in
dependent-name cache lookup/insertion or skip caching; in 1631-1688, apply the
same eligibility/key handling to DTST caching; in 485-506, prevent
nested-name-specifier resolutions from being reused across bindings; and in
442-447, do not treat node-only cached DTST fallbacks as context-independent.
In `@src/semantic/unifier.cpp`:
- Around line 460-479: Update the pack-expansion handling around inner and
elements so each expansion element uses a separate binding scope, ensuring
repeated references to the same pack parameter unify to the same value within
that element. Validate all repeated uses before committing results, then append
exactly one deduced value per pack parameter for each element; preserve matching
for consistent pairs and reject inconsistent ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b45ff68-67de-4fc9-bd24-53f8f805c71c
📒 Files selected for processing (3)
src/semantic/resolver.cppsrc/semantic/unifier.cpptests/unit/semantic/template_resolver_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/resolver.cpp
Line 1773 in 40de620
Substitute overloaded-arrow return types before the next hop
For a templated smart pointer such as template<class T> struct Ptr { T* operator->(); };, lookup deduces the class's T while finding operator->, but this assignment takes the declaration's raw return type and then discards the instantiator carrying that binding. The next hop therefore sees Ptr's unbound T*, and lookup for p->foo() cannot reach members of the caller's dependent T; substitute the method return type using the lookup's deduction stack before advancing the chain.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eff0a2a5e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42c55853d1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/resolver.cpp
Lines 499 to 500 in 87cabca
Honor concrete explicit specializations during lookup
When rewriting makes a dependent template-id concrete, make_specialization can find an existing explicit specialization and store its record as the TST's underlying type, but this dispatch ignores that record and performs lookup through the primary ClassTemplateDecl. For example, with key<X>::type = int, a primary A<T>::value = char, and an explicit A<int>::value = long, resolving typename A<typename key<X>::type>::value returns the primary's char declaration after the inner argument becomes int. Inspect the concrete specialization record before routing the lookup through the primary template.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 555a2ce7a0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a58e7073d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dab81074f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/resolver.cpp
Lines 1942 to 1944 in 27a2eca
Fill alias-template defaults before deduction
When a dependent member template resolves to a type-alias template with omitted defaulted parameters, this passes only the explicitly written arguments to deduce_template_arguments, whose contract rejects every unbound non-pack parameter. Thus Outer<X>::template alias<X> remains unresolved when alias is declared as template<class U, class V = int> using alias = Pair<U, V>, instead of becoming Pair<X, int>. Run check_template_arguments for this alias-template path before deduction, as the specialization-building path already does.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c40736a11b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0045ed8d26
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89c35822ce
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/resolver.cpp
Lines 505 to 507 in 448814c
Consult explicit specializations after arguments become concrete
When rewriting a dependent qualifier produces a concrete specialization, this path still extracts only its ClassTemplateDecl and dispatches to partial/primary lookup, ignoring the explicit specialization stored as the TemplateSpecializationType's underlying record. For example, if B<X>::type resolves to void, then A<typename B<X>::type>::member is looked up in primary A<T> rather than an existing A<void> specialization, returning the wrong member declaration or type. Prefer the concrete specialization record before falling back to the template declaration.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc9028bcb8
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af8a0e5de6
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
clice/src/semantic/resolver.cpp
Lines 647 to 648 in 59dd68f
Degrade ambiguous inherited member lookups
When a dependent class has multiple bases that declare the same member, this returns the first declaration encountered instead of preserving the ambiguity. For example, template<class> struct D : B1, B2 {} with distinct B1::type and B2::type makes typename D<T>::type ambiguous upon instantiation, but the resolver returns one base's type, causing hover and indexing to report an arbitrary declaration. Detect competing base results and leave the member unresolved.
clice/src/semantic/resolver.cpp
Line 752 in 59dd68f
Do not fall back to the primary after selecting a partial
When best matches but neither that partial nor its bases declare the requested member, control falls through and looks the member up in the primary template. A partial specialization replaces the primary rather than inheriting its members: with primary P<T> { using type = int; } and partial P<T*> {}, typename P<X*>::type is always invalid, but this returns the primary's int. Once a partial has been selected, leave a missing member unresolved instead of consulting the primary.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acae6f3238
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16be8b8ac4
ℹ️ 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".
1 participant
Summary
Rewrites
TemplateResolverto work directly on the AST, dropping its dependency onSema/TreeTransformentirely. The resolver now only needs anASTContext, which removes the diagnostic-silencing layer and theTypeLocplumbing the old implementation required. On top of the rewrite, this PR went through extensive review hardening (~15 bot-review rounds, 60+ findings triaged; every confirmed defect fixed with a pinned regression test).Design
TypeUnifier(src/semantic/unifier.{h,cpp}): Sema-free structural unification of template argument lists, replacingSema::DeduceTemplateArgumentsand partial ordering. Works on sugared types (bindings stay as-written:T = std::string, not the canonical expansion) and binds template parameters to dependent arguments, which pseudo-instantiation relies on.TemplateResolver(src/semantic/resolver.{h,cpp}), key invariants:Capabilities
Primary/partial/explicit specialization member lookup (incl. bases), alias templates (defaults, template template parameter binding, dependent template names), NNS chains, pointers/references (collapsing) / arrays (constant, unbounded, dependent bounds with value forwarding) / function types (noexcept incl. deduction and substitution of bare operands, method cv/ref qualifiers, ABI info, parameter decay) / member pointers /
__underlying_type/_Atomic/ attributed and decayed wrappers; structured pack expansion element-wise (type, template and value packs, lockstep zip, per-element consistency, arity-determined non-trailing suffixes, empty-pack cardinality); overload candidate filtering by call arity (incl. C++23 explicit object parameters and pack-expansion arguments), wired through semantic tokens, hover and indexing; partial-ordering ambiguity and unverifiable constrained partials degrade instead of guessing.Crash safety
The resolver must never crash on any input, including error-recovery ASTs from mid-edit code. Every AST construction site guards its preconditions (malformed pointers/references/arrays/member pointers/function types/atomics degrade); two crash-safety sweeps (
StandardSweepover libstdc++-heavy TUs,BrokenCodeSweepover deliberately broken code) run under the assertion-enabled ASan Debug build on every CI platform — this caught a libc++-only abort (dependent template names in specialization heads) and a dangling-pointer bug in pack narrowing (fixed via stable slot handles).Known limitations (follow-up PR)
Expression-level matching was never part of this design: compound NTTP expressions (
X<N + 1>), post-deduction validation of dependent-name/decltype patterns, dependent noexcept beyond bare operands, and constraint subsumption (constrained partials currently degrade to unresolved) are deferred to a dedicated follow-up.TODO(nttp-expr)anchors mark the sites. All of these degrade — they never produce wrong answers or crashes.Tests
Resolver unit suite: 171 cases (each review finding pinned by a regression test with a negative control where applicable). All four suites pass locally on both Debug and RelWithDebInfo.