refactor(semantic): rewrite template resolver — eliminate SubstType, fix crashes#388
Merged
16bit-ykiko merged 9 commits intomainfrom Apr 4, 2026
Merged
refactor(semantic): rewrite template resolver — eliminate SubstType, fix crashes#38816bit-ykiko merged 9 commits intomainfrom
16bit-ykiko merged 9 commits intomainfrom
Conversation
…nsform Replace the fragile double-TreeTransform architecture (PseudoInstantiator + SubstType) with a single-layer design that eliminates CodeSynthesisContexts management entirely. Key changes: - Parameter substitution done directly in TransformTemplateTypeParmType via InstantiationStack lookup, removing SubstType and DesugarOnly - Add SubstituteOnly class to separate typedef expansion from dependent name resolution, preventing typedef <-> lookup infinite cycles - Add cycle detection (active_resolutions set) for broader resolution cycles - Fix stack frame pollution: pop lookup frames before further TransformType to prevent incorrect parameter substitution across depth-0 collisions - Handle ClassTemplateDecl in DTST transform: resolve to concrete TST so member lookup works (e.g. __alloc_traits::rebind<T>::other) - Support Pack template arguments in TransformTemplateTypeParmType - Fix hole() fallback: check isDependentType on resolve result - Null-safe: all Transform methods return original type on failure - Remove manual CodeSynthesisContexts, state/rewind, std::abort - Add --log-level CLI option to unit test runner - Replace #ifndef NDEBUG debug flag with LOG_DEBUG Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughReworks template-dependent instantiation/substitution: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Resolver as TemplateResolver
participant Pseudo as PseudoInstantiator
participant Stack as InstantiationStack
participant AST as Sema/ASTContext
Client->>Resolver: request resolve/transform dependent type
Resolver->>Pseudo: TransformType / TransformDependentNameType / DTST
Pseudo->>Stack: push frame / find_argument(...)
Pseudo->>AST: run SubstituteOnly (expand dependent typedefs, substitute TemplateTypeParmType)
AST-->>Pseudo: substituted type (or null/dependent)
alt substitution produced non-dependent concrete type
Pseudo->>Resolver: return concrete type (cache result)
else substitution remains dependent or unresolved
Pseudo->>Pseudo: rebuild DTST or fallback (recurse guarded)
end
Pseudo->>Stack: pop/trim frame (restore stack size)
Resolver-->>Client: final transformed/resolved type
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/semantic/resolver.cpp`:
- Around line 427-428: The memoization stores QualType results in the `resolved`
cache keyed only by the AST node (e.g., where `resolved.try_emplace(NNS, type);
return lookup(type, name);` is used) but the returned `QualType` depends on the
current instantiation `stack`; update the cache key to include the instantiation
context (a stack signature) or restrict caching to context-free results only:
change entries where `resolved.try_emplace(...)` is used (also the similar sites
around the other occurrences you noted) to insert/get by `(node,
stackSignature)` (or skip caching when `stack` affects the result) so the same
AST node instantiated with different `stack` values cannot reuse an incompatible
specialization.
- Around line 824-843: TransformTypedefType currently substitutes the typedef's
underlying type with substitute() but returns early without running the full
TransformType pipeline if the substituted result is still dependent; update the
function so that after calling substitute(underlying) you check if the returned
clang::QualType is dependent (type->isDependentType()) and, if so, call
TransformType(TLB, type) (or the appropriate overload) to perform
lookup/resolution before proceeding; then handle ElaboratedType/unwrapping and
pushTrivial with the final resolved type, otherwise fall back to
Base::TransformTypedefType(TLB, TL) as before.
- Around line 129-137: The current getDeclType() converts RecordDecl to a
QualType which then falls through lookup(clang::QualType, ...) because that
function only handles TemplateSpecializationType and
DependentTemplateSpecializationType; update lookup(clang::QualType, ...) to also
handle clang::RecordType (and/or
clang::ElaboratedType->getNamedType()->getAsRecordType) by extracting the
RecordDecl and searching its members directly (similar to how template
specializations use getTemplateName/lookup), or alternatively stop converting
RecordDecl to QualType and pass the original RecordDecl down the resolution
path; specifically modify the lookup function to detect RecordType, obtain the
RecordDecl, and perform member lookup (using the same member-resolution logic
used for TemplateSpecializationType) so TD is not left null for nested
non-template record paths.
- Around line 103-126: findArgument() fails to handle variable template
declarations, so when visitTemplateDeclContexts() pushes
VarTemplatePartialSpecializationDecl/VarTemplateDecl frames the template args
aren't found; update findArgument() to normalize the looked-up decl via
getDescribedTemplate() (or call getDescribedTemplate() on partial
specializations) and include llvm::dyn_cast<clang::VarTemplateDecl> (and handle
VarTemplatePartialSpecializationDecl similarly) alongside ClassTemplateDecl,
ClassTemplatePartialSpecializationDecl, TypeAliasTemplateDecl, and
FunctionTemplateDecl so that depth/index checks succeed and the correct
TemplateArgument from it->second is returned.
In `@tests/unit/unit_tests.cc`:
- Around line 17-18: The current argument parsing returns argv[i+1] for a
matching option name without validating that the next token is not another
option, which allows option-token capture; update the logic in the parsing
routine (the block using argv, name, argc, and index i in unit_tests.cc) to
ensure i + 1 < argc and that argv[i+1] does not begin with '-' (i.e., is not
another option) before returning it, otherwise treat the option as missing a
value (e.g., return nullptr or continue searching).
- Around line 28-39: The code path that parses the log_level string (in
tests/unit/unit_tests.cc) silently ignores unsupported values; update the branch
after checking log_level (the log_level variable and assignments to
clice::logging::options.level) to detect invalid inputs, print a clear error
message listing accepted values (e.g., trace, debug, info, warn, err) to stderr
or the test logger, and return/exit with a non-zero status so typos are visible;
ensure the new branch executes when none of the strcmp/if checks match and use
the same function scope that currently sets clice::logging::options.level.
🪄 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
Run ID: 33ea2070-12f5-44d2-ac30-f2e2f533f257
📒 Files selected for processing (3)
src/semantic/resolver.cppsrc/semantic/resolver.htests/unit/unit_tests.cc
💤 Files with no reviewable changes (1)
- src/semantic/resolver.h
- Replace custom log() helper with direct LOG_DEBUG calls (lazy eval) - Redesign logging as narrative trace with indentation: resolve → lookup → deduce → found → substitute → result - Remove noisy low-level logs (TransformType, TTPT, typedef desugar) - Add structured deduce output: primary/partial/alias with param mapping - Cache unresolved DTST results to eliminate duplicate lookups - Fix indent inheritance for hole()'s nested PseudoInstantiator - Wrap all names/types in quotes for readability - Remove redundant 'resolver:' prefix from log lines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add active_ctd_lookups cycle detection in lookup(CTD, name, args) to prevent infinite recursion through recursive base classes (e.g. callback_traits<F> : callback_traits<decltype(&F::operator())>) - Add SubstituteOnly depth guard to prevent stack overflow from deep typedef chains - Add 22 new test cases covering: pointer/reference/const types, multiple inheritance, typedef chains, CRTP, remove_reference partial specs, indirect base classes, self-referential aliases, multiple packs, std::map::mapped_type, std::basic_string::value_type - Document 5 known limitations as TODOs: NTTP partial specialization, template template params, non-dependent qualifier nested class templates Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/semantic/resolver.cpp (3)
103-127:⚠️ Potential issue | 🟠 Major
findArgument()still misses variable-template frames pushed by context walk.Line 109 to Line 117 does not handle
clang::VarTemplateDecl(nor normalization from partial specializations), while Line 36 to Line 37 explicitly collectsVarTemplatePartialSpecializationDeclcontexts. This can leave template params unsubstituted in var-template contexts.🐛 Suggested fix
const clang::TemplateArgument* findArgument(const clang::TemplateTypeParmType* T) const { auto depth = T->getDepth(); auto index = T->getIndex(); for(auto it = data.rbegin(); it != data.rend(); ++it) { + clang::Decl* frame = it->first; + if(auto* described = frame->getDescribedTemplate()) { + frame = described; + } + clang::TemplateParameterList* params = nullptr; - if(auto* CTD = llvm::dyn_cast<clang::ClassTemplateDecl>(it->first)) { + if(auto* CTD = llvm::dyn_cast<clang::ClassTemplateDecl>(frame)) { params = CTD->getTemplateParameters(); - } else if(auto* CTPSD = llvm::dyn_cast<clang::ClassTemplatePartialSpecializationDecl>( - it->first)) { + } else if(auto* CTPSD = llvm::dyn_cast<clang::ClassTemplatePartialSpecializationDecl>(frame)) { params = CTPSD->getTemplateParameters(); - } else if(auto* TATD = llvm::dyn_cast<clang::TypeAliasTemplateDecl>(it->first)) { + } else if(auto* TATD = llvm::dyn_cast<clang::TypeAliasTemplateDecl>(frame)) { params = TATD->getTemplateParameters(); - } else if(auto* FTD = llvm::dyn_cast<clang::FunctionTemplateDecl>(it->first)) { + } else if(auto* FTD = llvm::dyn_cast<clang::FunctionTemplateDecl>(frame)) { params = FTD->getTemplateParameters(); + } else if(auto* VTD = llvm::dyn_cast<clang::VarTemplateDecl>(frame)) { + params = VTD->getTemplateParameters(); }#!/bin/bash set -euo pipefail rg -n "visitTemplateDeclContexts|VarTemplatePartialSpecializationDecl|findArgument|VarTemplateDecl|FunctionTemplateDecl|TypeAliasTemplateDecl" src/semantic/resolver.cpp -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/semantic/resolver.cpp` around lines 103 - 127, findArgument currently only checks ClassTemplateDecl, ClassTemplatePartialSpecializationDecl, TypeAliasTemplateDecl and FunctionTemplateDecl, so var-template frames are skipped causing unsubstituted params; update findArgument to also handle clang::VarTemplateDecl and clang::VarTemplatePartialSpecializationDecl by extracting their TemplateParameterList via getTemplateParameters() (normalize partial specialization to its parameters like you do for class partials), then continue the same depth/index check against data.rbegin() frames so VarTemplate contexts return the correct clang::TemplateArgument reference.
270-273:⚠️ Potential issue | 🔴 Critical
resolvedcache is still keyed only by AST node, but results are stack-dependent.The cache writes/reads at these sites can reuse a
QualTypecomputed under a different instantiation stack, which risks cross-instantiation contamination. This is a correctness issue, not just performance.🛡️ Safer short-term fix
- if(auto iter = resolved.find(DNT); iter != resolved.end()) { + if(stack.empty()) { + if(auto iter = resolved.find(DNT); iter != resolved.end()) { ... - } + } + } - resolved.try_emplace(DNT, result); + if(stack.empty()) { + resolved.try_emplace(DNT, result); + }Apply the same guard pattern for DTST/NNS cache read/write sites, or introduce a
(node, stack_signature)key.Also applies to: 758-763, 841-842, 872-876, 910-911, 930-931, 949-950, 961-962
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/semantic/resolver.cpp` around lines 270 - 273, The resolved cache is keyed only by AST node (the llvm::DenseMap<const void*, clang::QualType>& resolved in the constructor/Resolver class) but the computed QualType depends on the template/instantiation stack, causing cross-instantiation contamination; fix by making cache keys include the current instantiation-stack signature (or guard reads/writes by comparing a saved stack signature) so stored QualTypes are only reused for identical stacks—e.g., change the map to key by (node, stack_signature) or add a parallel map of signatures and check equality before returning/storing entries for the DTST/NNS cache read/write sites and any uses of resolved (the places noted using DTST/NNS read/write patterns).
965-981:⚠️ Potential issue | 🟠 Major
TransformTypedefTypereturns too early when substitution result remains dependent.After Line 970 substitution, the result may still contain unresolved dependent names. Returning it directly skips the full transform pass and can miss lookup resolution opportunities.
🐛 Suggested fix
clang::QualType TransformTypedefType(clang::TypeLocBuilder& TLB, clang::TypedefTypeLoc TL) { if(auto* TND = TL.getTypedefNameDecl()) { auto underlying = TND->getUnderlyingType(); if(underlying->isDependentType()) { auto type = substitute(underlying); if(!type.isNull()) { + if(type->isDependentType()) { + type = TransformType(type); + } + if(type.isNull()) { + return Base::TransformTypedefType(TLB, TL); + } if(auto ET = llvm::dyn_cast<clang::ElaboratedType>(type)) { type = ET->getNamedType(); } TLB.pushTrivial(context, type, {}); return type; } } } return Base::TransformTypedefType(TLB, TL); }#!/bin/bash set -euo pipefail sed -n '940,1010p' src/semantic/resolver.cpp | nl -ba rg -n "TransformTypedefType|substitute\\(|isDependentType\\(|TransformType\\(" src/semantic/resolver.cpp -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/semantic/resolver.cpp` around lines 965 - 981, TransformTypedefType currently returns immediately after substitute(underlying) even when the substituted type remains dependent, skipping the full transform/lookup pass; change the logic so you only short-circuit and push/trivially return when the substituted type is non-dependent (i.e., !type->isDependentType()), otherwise do not return early and let the normal transform path run (call Base::TransformTypedefType or invoke the general TransformType on the substituted type) so dependent names get resolved; adjust checks around llvm::dyn_cast<clang::ElaboratedType> and TLB.pushTrivial to only run when the substituted type is non-dependent and non-null.
🧹 Nitpick comments (1)
tests/unit/semantic/template_resolver_tests.cpp (1)
885-925: Extract arun_driver(...)helper for duplicated driver-based assertions.Line 885 to Line 925 duplicates the same compile/traverse/resolve/assert flow already encapsulated by
run(...)(for non-driver mode). A smallrun_driver(...)helper would reduce test boilerplate and keep future stdlib cases consistent.♻️ Suggested refactor
+void run_driver(llvm::StringRef code, llvm::StringRef standard = "-std=c++20") { + add_main("main.cpp", code); + ASSERT_TRUE(compile_driver(standard)); + + InputFinder finder(*unit); + finder.TraverseAST(unit->context()); + + auto input = unit->resolver().resolve(finder.input); + auto target = finder.expect; + ASSERT_FALSE(input.isNull() || target.isNull()); + EXPECT_EQ(input.getCanonicalType(), target.getCanonicalType()); +}Then use
run_driver(...)inStandardMapandStandardString.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/semantic/template_resolver_tests.cpp` around lines 885 - 925, The two tests duplicate the same compile_driver / InputFinder / traverse / resolve / assert flow; add a helper named run_driver(...) that mirrors the existing run(...) pattern but uses compile_driver(), creates an InputFinder, calls finder.TraverseAST(unit->context()), resolves finder.input via unit->resolver().resolve(...), and returns (or invokes a callback with) the resolved input and finder.expect so StandardMap and StandardString can call run_driver(...) to perform add_main(...), compile_driver(), traversal and the EXPECT_EQ assertions without duplicating boilerplate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/semantic/resolver.cpp`:
- Around line 103-127: findArgument currently only checks ClassTemplateDecl,
ClassTemplatePartialSpecializationDecl, TypeAliasTemplateDecl and
FunctionTemplateDecl, so var-template frames are skipped causing unsubstituted
params; update findArgument to also handle clang::VarTemplateDecl and
clang::VarTemplatePartialSpecializationDecl by extracting their
TemplateParameterList via getTemplateParameters() (normalize partial
specialization to its parameters like you do for class partials), then continue
the same depth/index check against data.rbegin() frames so VarTemplate contexts
return the correct clang::TemplateArgument reference.
- Around line 270-273: The resolved cache is keyed only by AST node (the
llvm::DenseMap<const void*, clang::QualType>& resolved in the
constructor/Resolver class) but the computed QualType depends on the
template/instantiation stack, causing cross-instantiation contamination; fix by
making cache keys include the current instantiation-stack signature (or guard
reads/writes by comparing a saved stack signature) so stored QualTypes are only
reused for identical stacks—e.g., change the map to key by (node,
stack_signature) or add a parallel map of signatures and check equality before
returning/storing entries for the DTST/NNS cache read/write sites and any uses
of resolved (the places noted using DTST/NNS read/write patterns).
- Around line 965-981: TransformTypedefType currently returns immediately after
substitute(underlying) even when the substituted type remains dependent,
skipping the full transform/lookup pass; change the logic so you only
short-circuit and push/trivially return when the substituted type is
non-dependent (i.e., !type->isDependentType()), otherwise do not return early
and let the normal transform path run (call Base::TransformTypedefType or invoke
the general TransformType on the substituted type) so dependent names get
resolved; adjust checks around llvm::dyn_cast<clang::ElaboratedType> and
TLB.pushTrivial to only run when the substituted type is non-dependent and
non-null.
---
Nitpick comments:
In `@tests/unit/semantic/template_resolver_tests.cpp`:
- Around line 885-925: The two tests duplicate the same compile_driver /
InputFinder / traverse / resolve / assert flow; add a helper named
run_driver(...) that mirrors the existing run(...) pattern but uses
compile_driver(), creates an InputFinder, calls
finder.TraverseAST(unit->context()), resolves finder.input via
unit->resolver().resolve(...), and returns (or invokes a callback with) the
resolved input and finder.expect so StandardMap and StandardString can call
run_driver(...) to perform add_main(...), compile_driver(), traversal and the
EXPECT_EQ assertions without duplicating boilerplate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 019220c9-a58b-4c31-9074-4944a2fe77ec
📒 Files selected for processing (2)
src/semantic/resolver.cpptests/unit/semantic/template_resolver_tests.cpp
…kers - Add file-level architecture comment describing PseudoInstantiator and SubstituteOnly roles, key invariant (Phase 2 must not trigger Phase 1) - Document all public/private methods with purpose, invariants, and caveats - Normalize FIXME→TODO with actionable descriptions: - Operator lookup needs DeclarationName-based approach - UnresolvedLookupExpr should handle overloaded lookups - CXXDependentScopeMemberExpr lookup not yet implemented - hole() should be replaced with general std library pattern handling - decltype only handles simple DeclRefExpr cases - Pack expansion limited to single-element packs - Add thread safety note to TemplateResolver class - Add depth-matching semantics warning to findArgument - Document stack frame preservation invariant in lookupInBases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/semantic/resolver.cpp (1)
1055-1059: Consider whether caching unresolved DTST fallback is desirable.Caching the rebuilt DTST as a fallback (line 1058) prevents repeated resolution attempts for the same unresolvable type. However, if the resolution context changes (e.g., more stack frames become available later), the cached failure prevents retry. This may be acceptable given the memoization concerns already flagged, but worth considering if resolution order matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/semantic/resolver.cpp` around lines 1055 - 1059, The current code caches a rebuilt DTST fallback via resolved.try_emplace(DTST, fallback) which prevents future resolution attempts if context later changes; update the unresolvable branch in the resolver (the block using DTST, fallback and rebuildDTST/TLB/TL) to avoid inserting this fallback into the resolved map for unresolved types — either remove the resolved.try_emplace(DTST, fallback) call so callers get the fallback but resolution can be retried later, or alternatively store a distinct sentinel/unresolved marker instead of the concrete fallback so subsequent resolution logic can detect and retry; reference symbols: DTST, fallback, rebuildDTST, resolved.try_emplace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/semantic/resolver.cpp`:
- Around line 1055-1059: The current code caches a rebuilt DTST fallback via
resolved.try_emplace(DTST, fallback) which prevents future resolution attempts
if context later changes; update the unresolvable branch in the resolver (the
block using DTST, fallback and rebuildDTST/TLB/TL) to avoid inserting this
fallback into the resolved map for unresolved types — either remove the
resolved.try_emplace(DTST, fallback) call so callers get the fallback but
resolution can be retried later, or alternatively store a distinct
sentinel/unresolved marker instead of the concrete fallback so subsequent
resolution logic can detect and retry; reference symbols: DTST, fallback,
rebuildDTST, resolved.try_emplace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5d1d595-2a60-41fd-b84f-e0bd3b6a752c
📒 Files selected for processing (2)
src/semantic/resolver.cppsrc/semantic/resolver.h
✅ Files skipped from review due to trivial changes (1)
- src/semantic/resolver.h
- Reject option-like tokens as values in --name parsing (unit_tests.cc) - Report unknown --log-level values with error message - Document resolved cache safety: keyed by AST node pointer which is unique per syntactic occurrence within a TU Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…unds safety Correctness fixes: - Add null check for TD in hole() after getAsTemplateDecl() - Add bounds check for arguments in hole() before accessing [0] - Add bounds check in ResugarOnly for lists[depth] access - Add null check in visitTemplateDeclContexts after getDeclContext cast - Add TODO comment for NTTP default argument limitation in checkTemplateArguments API cleanup (resolver.h): - Add `explicit` to TemplateResolver constructor - Rename resolve(UnresolvedUsingTypenameDecl) → lookup() for consistency - Add const to CXXDependentScopeMemberExpr* parameter Test improvement: - Document RecursiveBaseClass test as crash-only regression test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/unit_tests.cc`:
- Around line 9-23: find_arg currently returns an empty std::string_view for
both "argument not provided" and "argument provided but empty", which hides
malformed inputs like `--log-level=`; change find_arg to return
std::optional<std::string_view> (or another sentinel) so absence vs.
present-empty can be distinguished, update the function signature and its return
sites (callers that validate log level and other options) to check has_value()
before validating the string contents, and ensure the branch that treats an
explicit empty value as malformed triggers when the optional is present but
contains an empty string.
🪄 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
Run ID: ea6995ae-a40a-4ae4-ac8a-de7a4114db9b
📒 Files selected for processing (2)
src/semantic/resolver.htests/unit/unit_tests.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/semantic/resolver.h
16bit-ykiko
changed the title
- Rename camelCase variables to snake_case: parentIndent → parent_indent, stackSize → stack_size, declKind → decl_kind, declName → decl_name - Replace std::fprintf with std::print (C++23) - Shorten test names to ≤4 words per project convention Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feature/template-resolver
branch
from
2650adb to
d82e3af
Compare
…sing - Rename all internal functions to snake_case per project convention: findArgument, getDeclType, checkTemplateArguments, deduceTemplateArguments, lookupInBases, rebuildDTST, visitTemplateDeclContexts - Replace hand-rolled CLI parsing in unit_tests.cc with deco::cli::parse - Use DecoKVStyled for --test-filter, --log-level, --test-dir options - Normalize FIXME/TODO comments in resolver.h Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feature/template-resolver
branch
from
4f1f52c to
cc5b443
Compare
Use `names = {"--name", "--name="}` pattern for both separate and joined
KV styles in unit test CLI options.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feature/template-resolver
branch
from
cc5b443 to
c776737
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/unit_tests.cc (1)
40-53:⚠️ Potential issue | 🟡 MinorReject unsupported
--log-levelvalues instead of silently ignoring them.Invalid values currently fall through with no feedback, which makes typos hard to detect and can hide misconfigured test runs.
Suggested fix
if(parsed.has_value() && parsed->options.log_level.has_value()) { auto level = *parsed->options.log_level; if(level == "trace") { clice::logging::options.level = clice::logging::Level::trace; } else if(level == "debug") { clice::logging::options.level = clice::logging::Level::debug; } else if(level == "info") { clice::logging::options.level = clice::logging::Level::info; } else if(level == "warn") { clice::logging::options.level = clice::logging::Level::warn; } else if(level == "err") { clice::logging::options.level = clice::logging::Level::err; + } else { + std::fprintf(stderr, + "Unknown --log-level '%s'. Valid: trace, debug, info, warn, err\n", + level.c_str()); + return 2; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/unit_tests.cc` around lines 40 - 53, The code that maps parsed->options.log_level to clice::logging::options.level currently ignores unknown strings; update the block around parsed/parsed->options.log_level (the mapping that sets clice::logging::options.level to clice::logging::Level::trace/debug/info/warn/err) to detect unsupported values and fail fast: on an unrecognized level string, emit an explicit error (e.g., to stderr or process logger) mentioning the invalid value and the accepted options and then abort/return non-zero (or throw an exception) so invalid --log-level values are rejected instead of silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/unit_tests.cc`:
- Around line 24-26: The --test-dir option (declared via DecoKVStyled and stored
in the variable test_dir) is parsed but never used; wire it into the test runner
setup so the value actually controls where test data is loaded (or remove/hide
the option if you don't want to support it yet). Locate the DecoKVStyled
declaration and the std::string test_dir and propagate test_dir into the test
initialization code (e.g., pass it into the runner configuration or set the
global/test fixture data directory before tests start), and apply the same
change for the duplicate declaration referenced around the second occurrence so
the option is consistently used or removed.
---
Duplicate comments:
In `@tests/unit/unit_tests.cc`:
- Around line 40-53: The code that maps parsed->options.log_level to
clice::logging::options.level currently ignores unknown strings; update the
block around parsed/parsed->options.log_level (the mapping that sets
clice::logging::options.level to
clice::logging::Level::trace/debug/info/warn/err) to detect unsupported values
and fail fast: on an unrecognized level string, emit an explicit error (e.g., to
stderr or process logger) mentioning the invalid value and the accepted options
and then abort/return non-zero (or throw an exception) so invalid --log-level
values are rejected instead of silently ignored.
🪄 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
Run ID: 671eee33-b24b-4bd2-967f-8651b0055979
📒 Files selected for processing (4)
src/semantic/resolver.cppsrc/semantic/resolver.htests/unit/semantic/template_resolver_tests.cpptests/unit/unit_tests.cc
✅ Files skipped from review due to trivial changes (1)
- src/semantic/resolver.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/semantic/template_resolver_tests.cpp
- src/semantic/resolver.h
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
Rewrites the template resolver to eliminate
SubstType/CodeSynthesisContextsdependency, fixing widespread crashes on real-world C++ code.What changed
Architecture: replaced double-TreeTransform (PseudoInstantiator + SubstType) with single-layer design:
SubstituteOnly— new lightweight TreeTransform for Phase 2 (typedef expansion + parameter substitution). Does NOT overrideTransformDependentNameType, breaking the typedef ↔ lookup infinite cycle.PseudoInstantiator— retains heuristic lookup (the unique value clang doesn't provide), delegates substitution toSubstituteOnly.Deleted:
DesugarOnlyclassinstantiate()method and allCodeSynthesisContexts/SubstTypeusagestate()/rewind()stack managementstd::abort()on valid NNS kinds#ifndef NDEBUGdebug flag +std::printloggingAdded:
SubstituteOnlyclass with depth guardInstantiationStack::findArgument()— depth/index based parameter lookupDependentTemplateSpecializationType(enables__alloc_traits::rebind<T>::otherresolution)active_resolutions(DNT cycle detection) +active_ctd_lookups(CTD cycle detection via RAII guard)TransformTypeLOG_DEBUGtrace logging with indentation--log-level/--test-filterCLI options for unit test runnerhole(),ResugarOnly,visitTemplateDeclContextsTests: 20 → 36 passing test cases (+5 documented TODOs for known limitations). New coverage: recursive base classes, multiple inheritance, typedef chains, CRTP,
remove_referencepartial specs,std::map,std::basic_string, pack forwarding.Stress test result
Before this PR, the same test produced ~52% crash rate (413 crashes in 800 files).
Known limitations (documented as TODOs)
enable_if<true, X>,A<X, 0>)Outer<int>::Inner<X>)CXXDependentScopeMemberExprlookup (unimplemented)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests