[clang] CTAD alias: fix the transformation for the require-clause expr #90961
Conversation
This file contains 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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesIn the clang AST, constraint nodes are deliberately not instantiated unless they are actively being evaluated. Consequently, occurrences of template parameters in the require-clause expression have a subtle "depth" difference compared to normal occurrences in places, such as function parameters. When transforming the require-clause, we must take this distinction into account. The existing implementation overlooks this consideration. This patch is to rewrite the implementation of the require-clause transformation to address this issue. Fixes #90177 Full diff: https://github.com/llvm/llvm-project/pull/90961.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 7f49acc36769e9..5f1a44d7850280 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2744,31 +2744,155 @@ bool hasDeclaredDeductionGuides(DeclarationName Name, DeclContext *DC) {
return false;
}
+unsigned getTemplateDepth(NamedDecl *TemplateParam) {
+ if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(TemplateParam))
+ return TTP->getDepth();
+ if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(TemplateParam))
+ return TTP->getDepth();
+ if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(TemplateParam))
+ return NTTP->getDepth();
+ llvm_unreachable("Unhandled template parameter types");
+}
+
NamedDecl *transformTemplateParameter(Sema &SemaRef, DeclContext *DC,
NamedDecl *TemplateParam,
MultiLevelTemplateArgumentList &Args,
- unsigned NewIndex) {
+ unsigned NewIndex, unsigned NewDepth) {
if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(TemplateParam))
- return transformTemplateTypeParam(SemaRef, DC, TTP, Args, TTP->getDepth(),
+ return transformTemplateTypeParam(SemaRef, DC, TTP, Args, NewDepth,
NewIndex);
if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(TemplateParam))
return transformTemplateParam(SemaRef, DC, TTP, Args, NewIndex,
- TTP->getDepth());
+ NewDepth);
if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(TemplateParam))
return transformTemplateParam(SemaRef, DC, NTTP, Args, NewIndex,
- NTTP->getDepth());
+ NewDepth);
llvm_unreachable("Unhandled template parameter types");
}
-Expr *transformRequireClause(Sema &SemaRef, FunctionTemplateDecl *FTD,
- llvm::ArrayRef<TemplateArgument> TransformedArgs) {
- Expr *RC = FTD->getTemplateParameters()->getRequiresClause();
+// Transform the require-clause of F if any.
+// The return result is expected to be the require-clause for the synthesized
+// alias deduction guide.
+Expr *transformRequireClause(
+ Sema &SemaRef, FunctionTemplateDecl *F,
+ TypeAliasTemplateDecl *AliasTemplate,
+ ArrayRef<DeducedTemplateArgument> DeduceResults) {
+ Expr *RC = F->getTemplateParameters()->getRequiresClause();
if (!RC)
return nullptr;
+
+ auto &Context = SemaRef.Context;
+ LocalInstantiationScope Scope(SemaRef);
+
+ // In the clang AST, constraint nodes are not instantiated at all unless they
+ // are being evaluated. This means that occurrences of template parameters
+ // in the require-clause expr have subtle differences compared to occurrences
+ // in other places, such as function parameters. When transforming the
+ // require-clause, we must respect these differences, particularly regarding
+ // the 'depth' information:
+ // 1) In the transformed require-clause, occurrences of template parameters
+ // must use the "uninstantiated" depth;
+ // 2) When substituting on the require-clause expr of the underlying
+ // deduction guide, we must use the entire set of template argument lists.
+ //
+ // It's important to note that we're performing this transformation on an
+ // *instantiated* AliasTemplate.
+
+ // For 1), if the alias template is nested within a class template, we
+ // calcualte the 'uninstantiated' depth by adding the substitution level back.
+ unsigned AdjustDepth = 0;
+ if (auto *PrimaryTemplate = AliasTemplate->getInstantiatedFromMemberTemplate())
+ AdjustDepth = PrimaryTemplate->getTemplateDepth();
+
+ // We rebuild all template parameters with the uninstantiated depth, and
+ // build template arguments refer to them.
+ SmallVector<TemplateArgument> AdjustedAliasTemplateArgs;
+
+ for (auto *TP : *AliasTemplate->getTemplateParameters()) {
+ // Rebuild any internal references to earlier parameters and reindex
+ // as we go.
+ MultiLevelTemplateArgumentList Args;
+ Args.setKind(TemplateSubstitutionKind::Rewrite);
+ Args.addOuterTemplateArguments(AdjustedAliasTemplateArgs);
+ NamedDecl *NewParam = transformTemplateParameter(
+ SemaRef, AliasTemplate->getDeclContext(), TP, Args,
+ /*NewIndex=*/AdjustedAliasTemplateArgs.size(),
+ getTemplateDepth(TP) + AdjustDepth);
+
+ auto NewTemplateArgument = Context.getCanonicalTemplateArgument(
+ Context.getInjectedTemplateArg(NewParam));
+ AdjustedAliasTemplateArgs.push_back(NewTemplateArgument);
+ }
+ // Template arguments used to transform the template arguments in
+ // DeducedResults.
+ SmallVector<TemplateArgument> TemplateArgsForBuildingRC(
+ F->getTemplateParameters()->size());
+ // Transform the transformed template args
MultiLevelTemplateArgumentList Args;
Args.setKind(TemplateSubstitutionKind::Rewrite);
- Args.addOuterTemplateArguments(TransformedArgs);
- ExprResult E = SemaRef.SubstExpr(RC, Args);
+ Args.addOuterTemplateArguments(AdjustedAliasTemplateArgs);
+
+ for (unsigned Index = 0; Index < DeduceResults.size(); ++Index) {
+ const auto &D = DeduceResults[Index];
+ if (D.isNull())
+ continue;
+ TemplateArgumentLoc Input =
+ SemaRef.getTrivialTemplateArgumentLoc(D, QualType(), SourceLocation{});
+ TemplateArgumentLoc Output;
+ if (!SemaRef.SubstTemplateArgument(Input, Args, Output)) {
+ assert(TemplateArgsForBuildingRC[Index].isNull() &&
+ "InstantiatedArgs must be null before setting");
+ TemplateArgsForBuildingRC[Index] = Output.getArgument();
+ }
+ }
+
+ // A list of template arguments for transforming the require-clause of F.
+ // It must contain the entire set of template argument lists.
+ MultiLevelTemplateArgumentList ArgsForBuildingRC;
+ ArgsForBuildingRC.setKind(clang::TemplateSubstitutionKind::Rewrite);
+ ArgsForBuildingRC.addOuterTemplateArguments(TemplateArgsForBuildingRC);
+ // For 2), if the underlying F is instantiated from a member template, we need
+ // the entire template argument list, as the constraint AST in the
+ // require-clause of F remains completely uninstantiated.
+ //
+ // For example:
+ // template <typename T> // depth 0
+ // struct Outer {
+ // template <typename U>
+ // struct Foo { Foo(U); };
+ //
+ // template <typename U> // depth 1
+ // requires C<U>
+ // Foo(U) -> Foo<int>;
+ // };
+ // template <typename U>
+ // using AFoo = Outer<int>::Foo<U>;
+ //
+ // In this scenario, the deduction guide for `Foo` inside `Outer<int>`:
+ // - The occurrence of U in the require-expression is [depth:1, index:0]
+ // - The occurrence of U in the function parameter is [depth:0, index:0]
+ // - The template parameter of U is [depth:0, index:0]
+ // Particularly, for the `Foo` deduction guide inside the `Outer<int>`:
+ //
+ // - occurrence of U in the require-expression is [depth:1, index:0]
+ // - occurrence of U in the function parameter is [depth:0, index:0]
+ // - the template parameter of U is [depth:0, index:0]
+ //
+ // We add the outer template arguments which is [int] to the multi-level arg
+ // list to ensure that the occurrence U in `C<U>` will be replaced with int
+ // during the substitution.
+ if (F->getInstantiatedFromMemberTemplate()) {
+ auto OuterLevelArgs = SemaRef.getTemplateInstantiationArgs(
+ F, F->getLexicalDeclContext(),
+ /*Final=*/false, /*Innermost=*/std::nullopt,
+ /*RelativeToPrimary=*/true,
+ /*Pattern=*/nullptr,
+ /*ForConstraintInstantiation=*/true);
+ for (auto It : OuterLevelArgs)
+ ArgsForBuildingRC.addOuterTemplateArguments(It.Args);
+ }
+
+ ExprResult E = SemaRef.SubstExpr(RC, ArgsForBuildingRC);
if (E.isInvalid())
return nullptr;
return E.getAs<Expr>();
@@ -2906,7 +3030,7 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
Args.addOuterTemplateArguments(TransformedDeducedAliasArgs);
NamedDecl *NewParam = transformTemplateParameter(
SemaRef, AliasTemplate->getDeclContext(), TP, Args,
- /*NewIndex=*/FPrimeTemplateParams.size());
+ /*NewIndex=*/FPrimeTemplateParams.size(), getTemplateDepth(TP));
FPrimeTemplateParams.push_back(NewParam);
auto NewTemplateArgument = Context.getCanonicalTemplateArgument(
@@ -2923,7 +3047,8 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
// TemplateArgsForBuildingFPrime.
Args.addOuterTemplateArguments(TemplateArgsForBuildingFPrime);
NamedDecl *NewParam = transformTemplateParameter(
- SemaRef, F->getDeclContext(), TP, Args, FPrimeTemplateParams.size());
+ SemaRef, F->getDeclContext(), TP, Args, FPrimeTemplateParams.size(),
+ getTemplateDepth(TP));
FPrimeTemplateParams.push_back(NewParam);
assert(TemplateArgsForBuildingFPrime[FTemplateParamIdx].isNull() &&
@@ -2978,8 +3103,8 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
Sema::CodeSynthesisContext::BuildingDeductionGuides)) {
auto *GG = cast<CXXDeductionGuideDecl>(FPrime);
- Expr *RequiresClause =
- transformRequireClause(SemaRef, F, TemplateArgsForBuildingFPrime);
+ Expr *RequiresClause = transformRequireClause(
+ SemaRef, F, AliasTemplate, DeduceResults);
// FIXME: implement the is_deducible constraint per C++
// [over.match.class.deduct]p3.3:
diff --git a/clang/test/AST/ast-dump-ctad-alias.cpp b/clang/test/AST/ast-dump-ctad-alias.cpp
new file mode 100644
index 00000000000000..423c3454ccb73e
--- /dev/null
+++ b/clang/test/AST/ast-dump-ctad-alias.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++2a -ast-dump %s | FileCheck -strict-whitespace %s
+
+template <typename, typename>
+constexpr bool Concept = true;
+template<typename T> // depth 0
+struct Out {
+ template<typename U> // depth 1
+ struct Inner {
+ U t;
+ };
+
+ template<typename V> // depth1
+ requires Concept<T, V>
+ Inner(V) -> Inner<V>;
+};
+
+template <typename X>
+struct Out2 {
+ template<typename Y> // depth1
+ using AInner = Out<int>::Inner<Y>;
+};
+Out2<double>::AInner t(1.0);
+
+// Verify that the require-clause of alias deduction guide is transformed correctly:
+// - Occurrence T should be replaced with `int`;
+// - Occurrence V should be replaced with the Y with depth 1
+//
+// CHECK: | `-FunctionTemplateDecl {{.*}} <deduction guide for AInner>
+// CHECK-NEXT: | |-TemplateTypeParmDecl {{.*}} typename depth 0 index 0 Y
+// CHECK-NEXT: | |-UnresolvedLookupExpr {{.*}} '<dependent type>' lvalue (no ADL) = 'Concept'
+// CHECK-NEXT: | | |-TemplateArgument type 'int'
+// CHECK-NEXT: | | | `-BuiltinType {{.*}} 'int'
+// CHECK-NEXT: | | `-TemplateArgument type 'type-parameter-1-0'
+// CHECK-NEXT: | | `-TemplateTypeParmType {{.*}} 'type-parameter-1-0' dependent depth 1 index 0
+// CHECK-NEXT: | |-CXXDeductionGuideDecl {{.*}} <deduction guide for AInner> 'auto (type-parameter-0-0) -> Inner<type-parameter-0-0>'
+// CHECK-NEXT: | | `-ParmVarDecl {{.*}} 'type-parameter-0-0'
+// CHECK-NEXT: | `-CXXDeductionGuideDecl {{.*}} used <deduction guide for AInner> 'auto (double) -> Inner<double>' implicit_instantiation
+// CHECK-NEXT: | |-TemplateArgument type 'double'
+// CHECK-NEXT: | | `-BuiltinType {{.*}} 'double'
+// CHECK-NEXT: | `-ParmVarDecl {{.*}} 'double'
diff --git a/clang/test/SemaCXX/cxx20-ctad-type-alias.cpp b/clang/test/SemaCXX/cxx20-ctad-type-alias.cpp
index e8b4383f53c5ea..4c5595e409f285 100644
--- a/clang/test/SemaCXX/cxx20-ctad-type-alias.cpp
+++ b/clang/test/SemaCXX/cxx20-ctad-type-alias.cpp
@@ -321,3 +321,71 @@ AG ag(1.0);
// choosen.
static_assert(__is_same(decltype(ag.t1), int));
} // namespace test23
+
+// GH90177
+// verify that the transformed require-clause of the alias deduction gudie has
+// the right depth info.
+namespace test24 {
+class Forward;
+class Key {};
+
+template <typename D>
+constexpr bool C = sizeof(D);
+
+// Case1: the alias template and the underlying deduction guide are in the same
+// scope.
+template <typename T>
+struct Case1 {
+ template <typename U>
+ struct Foo {
+ Foo(U);
+ };
+
+ template <typename V>
+ requires (C<V>)
+ Foo(V) -> Foo<V>;
+
+ template <typename Y>
+ using Alias = Foo<Y>;
+};
+// The require-clause should be evaluated on the type Key.
+Case1<Forward>::Alias t2 = Key();
+
+
+// Case2: the alias template and underlying deduction guide are in different
+// scope.
+template <typename T>
+struct Foo {
+ Foo(T);
+};
+template <typename U>
+requires (C<U>)
+Foo(U) -> Foo<U>;
+
+template <typename T>
+struct Case2 {
+ template <typename Y>
+ using Alias = Foo<Y>;
+};
+// The require-caluse should be evaluated on the type Key.
+Case2<Forward>::Alias t1 = Key();
+
+// Case3: crashes on the constexpr evaluator due to the mixed-up depth in
+// require-expr.
+template <class T1>
+struct A1 {
+ template<class T2>
+ struct A2 {
+ template <class T3>
+ struct Foo {
+ Foo(T3);
+ };
+ template <class T3>
+ requires C<T3>
+ Foo(T3) -> Foo<T3>;
+ };
+};
+template <typename U>
+using AFoo = A1<int>::A2<int>::Foo<U>;
+AFoo case3(1);
+} // namespace test24
|
erichkeane
approved these changes
May 3, 2024
clang/lib/Sema/SemaTemplate.cpp
Outdated
| @@ -2744,31 +2744,155 @@ bool hasDeclaredDeductionGuides(DeclarationName Name, DeclContext *DC) { | |||
| return false; | |||
| } | |||
|
|
|||
| unsigned getTemplateDepth(NamedDecl *TemplateParam) { | |||
There was a problem hiding this comment.
are we in an anonymous namespace? Else this should be 'static'.
There was a problem hiding this comment.
Yeah, we're in an anonymous namespace.
clang/lib/Sema/SemaTemplate.cpp
Outdated
| // Transform the require-clause of F if any. | ||
| // The return result is expected to be the require-clause for the synthesized | ||
| // alias deduction guide. | ||
| Expr *transformRequireClause( |
There was a problem hiding this comment.
Same question/etc here.
There was a problem hiding this comment.
We're in an anonymous namespace.
mizvekov
reviewed
May 3, 2024
clang/lib/Sema/SemaTemplate.cpp
Outdated
| @@ -2744,31 +2744,155 @@ bool hasDeclaredDeductionGuides(DeclarationName Name, DeclContext *DC) { | |||
| return false; | |||
| } | |||
|
|
|||
| unsigned getTemplateDepth(NamedDecl *TemplateParam) { | |||
There was a problem hiding this comment.
I think this is potentially misleading, as we already have multiple unrelated getTemplateDepth, see Sema.h / SemaTemplate.cpp, DeclBase.h, Expr.h.
There was a problem hiding this comment.
Renamed to getTemplateParameterDepth per your suggestion.
| /*NewIndex=*/AdjustedAliasTemplateArgs.size(), | ||
| getTemplateDepth(TP) + AdjustDepth); | ||
|
|
||
| auto NewTemplateArgument = Context.getCanonicalTemplateArgument( |
There was a problem hiding this comment.
Why do we need to canonicalize the argument here?
There was a problem hiding this comment.
Probably not needed, and I think avoiding the use of canonical types can lead to a more optimal AST, see #79798. We've already used this pattern in the ConvertConstructorToDeductionGuideTransform, and the current implementation here simply follows it.
However, addressing this issue is not the primary focus of this PR. It might be more appropriate to address it in a separate patch.
mizvekov
reviewed
May 3, 2024
clang/lib/Sema/SemaTemplate.cpp
Outdated
| @@ -2744,31 +2744,155 @@ bool hasDeclaredDeductionGuides(DeclarationName Name, DeclContext *DC) { | |||
| return false; | |||
| } | |||
|
|
|||
| unsigned getTemplateDepth(NamedDecl *TemplateParam) { | |||
There was a problem hiding this comment.
Suggested change
| unsigned getTemplateDepth(NamedDecl *TemplateParam) { | |
| unsigned getTemplateParameterDepth(NamedDecl *TemplateParam) { |
zyn0217
reviewed
May 3, 2024
clang/lib/Sema/SemaTemplate.cpp
Outdated
| // For 1), if the alias template is nested within a class template, we | ||
| // calcualte the 'uninstantiated' depth by adding the substitution level back. | ||
| unsigned AdjustDepth = 0; | ||
| if (auto *PrimaryTemplate = AliasTemplate->getInstantiatedFromMemberTemplate()) |
There was a problem hiding this comment.
I've noticed that there are several places (e.g.,1, 2, 3) where we use a while-loop to retrieve the primary template. However, I'm not entirely clear on the rationale behind it. It appears that using getInstantiatedFromMemberTemplate() consistently provides the correct result for all cases I came up with. Interestingly, removing the while-loop from all occurrences [1], [2], [3] doesn't trigger any test failures on check-clang.
| // We add the outer template arguments which is [int] to the multi-level arg | ||
| // list to ensure that the occurrence U in `C<U>` will be replaced with int | ||
| // during the substitution. | ||
| if (F->getInstantiatedFromMemberTemplate()) { |
There was a problem hiding this comment.
Ditto. Do we want to use the “primary” template after possibly multiple transformations?
hokein
force-pushed
the
fix-ctad-require-ast
branch
from
94d5225
to
cec1ca8
Compare
hokein
force-pushed
the
fix-ctad-require-ast
branch
from
cec1ca8
to
9cc438e
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
expr. In the clang AST, constraint nodes are deliberately not instantiated unless they are actively being evaluated. Consequently, occurrences of template parameters in the require-clause expression have a subtle "depth" difference compared to normal occurrences in place contexts, such as function parameters. When transforming the require-clause, we must take this distinction into account. The existing implementation overlooks this consideration. This patch is to rewrite the implementation of the require-clause transformation to address this issue.
hokein
force-pushed
the
fix-ctad-require-ast
branch
from
9cc438e
to
8e2a2a4
Compare
|
I'm merging it now (happy to address any post-comments). |
hokein
added a commit
to hokein/llvm-project
that referenced
this pull request
May 28, 2024
In the llvm#90961 fix, we miss a case where the undeduced template parameters of the underlying deduction guide is not transformed, which leaves incorrect depth/index information, and causes crash when evaluating the constraints. This patch fix this missing case. Fixes llvm#92596 Fixes llvm#92212
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In the clang AST, constraint nodes are deliberately not instantiated unless they are actively being evaluated. Consequently, occurrences of template parameters in the require-clause expression have a subtle "depth" difference compared to normal occurrences in places, such as function parameters. When transforming the require-clause, we must take this distinction into account.
The existing implementation overlooks this consideration. This patch is to rewrite the implementation of the require-clause transformation to address this issue.
Fixes #90177