Skip to content

[Clang] Correct the order of substituted arguments in CTAD alias guides #123022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jan 15, 2025

We missed a case of type constraints referencing deduced template parameters when constructing a deduction guide for the type alias. This patch fixes the issue by swapping the order of constructing 'template arguments not appearing in the type alias parameters' and 'template arguments that are not yet deduced'.

Fixes #122134

We missed a case of type constraints referencing deduced template parameters
when constructing a deduction guide for the type alias. This patch fixes
the issue by swapping the order of constructing 'template arguments not
appearing in the type alias parameters' and 'template arguments that are
not yet deduced'.
@zyn0217 zyn0217 marked this pull request as ready for review January 15, 2025 09:41
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

We missed a case of type constraints referencing deduced template parameters when constructing a deduction guide for the type alias. This patch fixes the issue by swapping the order of constructing 'template arguments not appearing in the type alias parameters' and 'template arguments that are not yet deduced'.

Fixes #122134


Full diff: https://github.com/llvm/llvm-project/pull/123022.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/Sema/SemaTemplateDeductionGuide.cpp (+24-26)
  • (modified) clang/test/SemaTemplate/deduction-guide.cpp (+50)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 07a1a4195427d8..9b87e0f38e5eed 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -907,6 +907,7 @@ Bug Fixes to C++ Support
   (`LWG3929 <https://wg21.link/LWG3929>`__.) (#GH121278)
 - Clang now identifies unexpanded parameter packs within the type constraint on a non-type template parameter. (#GH88866)
 - Fixed an issue while resolving type of expression indexing into a pack of values of non-dependent type (#GH121242)
+- Fixed incorrect construction of template arguments for CTAD alias guides when type constraints are applied. (#GH122134)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
index d42c3765aa534f..1eb0abb41d3e9a 100644
--- a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
+++ b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
@@ -996,7 +996,7 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
       F->getTemplateParameters()->size());
 
   // FIXME: DeduceTemplateArguments stops immediately at the first
-  // non-deducible template argument. However, this doesn't seem to casue
+  // non-deducible template argument. However, this doesn't seem to cause
   // issues for practice cases, we probably need to extend it to continue
   // performing deduction for rest of arguments to align with the C++
   // standard.
@@ -1053,25 +1053,6 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
     TransformedDeducedAliasArgs[AliasTemplateParamIdx] = NewTemplateArgument;
   }
   unsigned FirstUndeducedParamIdx = FPrimeTemplateParams.size();
-  //   ...followed by the template parameters of f that were not deduced
-  //   (including their default template arguments)
-  for (unsigned FTemplateParamIdx : NonDeducedTemplateParamsInFIndex) {
-    auto *TP = F->getTemplateParameters()->getParam(FTemplateParamIdx);
-    MultiLevelTemplateArgumentList Args;
-    Args.setKind(TemplateSubstitutionKind::Rewrite);
-    // We take a shortcut here, it is ok to reuse the
-    // TemplateArgsForBuildingFPrime.
-    Args.addOuterTemplateArguments(TemplateArgsForBuildingFPrime);
-    NamedDecl *NewParam = transformTemplateParameter(
-        SemaRef, F->getDeclContext(), TP, Args, FPrimeTemplateParams.size(),
-        getDepthAndIndex(TP).first);
-    FPrimeTemplateParams.push_back(NewParam);
-
-    assert(TemplateArgsForBuildingFPrime[FTemplateParamIdx].isNull() &&
-           "The argument must be null before setting");
-    TemplateArgsForBuildingFPrime[FTemplateParamIdx] =
-        Context.getInjectedTemplateArg(NewParam);
-  }
 
   // To form a deduction guide f' from f, we leverage clang's instantiation
   // mechanism, we construct a template argument list where the template
@@ -1086,18 +1067,14 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
   //  2) non-deduced template parameters of f: rebuild a
   //  template argument;
   //
-  // 2) has been built already (when rebuilding the new template
-  // parameters), we now perform 1).
+  // We now perform 1), as case 2) might refer to substituted 1).
   MultiLevelTemplateArgumentList Args;
   Args.setKind(TemplateSubstitutionKind::Rewrite);
   Args.addOuterTemplateArguments(TransformedDeducedAliasArgs);
   for (unsigned Index = 0; Index < DeduceResults.size(); ++Index) {
     const auto &D = DeduceResults[Index];
     if (D.isNull()) {
-      // 2): Non-deduced template parameter has been built already.
-      assert(!TemplateArgsForBuildingFPrime[Index].isNull() &&
-             "template arguments for non-deduced template parameters should "
-             "be been set!");
+      // 2): Non-deduced template parameters would be substituted later.
       continue;
     }
     TemplateArgumentLoc Input =
@@ -1110,6 +1087,27 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
     }
   }
 
+  // Case 2)
+  //   ...followed by the template parameters of f that were not deduced
+  //   (including their default template arguments)
+  for (unsigned FTemplateParamIdx : NonDeducedTemplateParamsInFIndex) {
+    auto *TP = F->getTemplateParameters()->getParam(FTemplateParamIdx);
+    MultiLevelTemplateArgumentList Args;
+    Args.setKind(TemplateSubstitutionKind::Rewrite);
+    // We take a shortcut here, it is ok to reuse the
+    // TemplateArgsForBuildingFPrime.
+    Args.addOuterTemplateArguments(TemplateArgsForBuildingFPrime);
+    NamedDecl *NewParam = transformTemplateParameter(
+        SemaRef, F->getDeclContext(), TP, Args, FPrimeTemplateParams.size(),
+        getDepthAndIndex(TP).first);
+    FPrimeTemplateParams.push_back(NewParam);
+
+    assert(TemplateArgsForBuildingFPrime[FTemplateParamIdx].isNull() &&
+           "The argument must be null before setting");
+    TemplateArgsForBuildingFPrime[FTemplateParamIdx] =
+        Context.getInjectedTemplateArg(NewParam);
+  }
+
   auto *TemplateArgListForBuildingFPrime =
       TemplateArgumentList::CreateCopy(Context, TemplateArgsForBuildingFPrime);
   // Form the f' by substituting the template arguments into f.
diff --git a/clang/test/SemaTemplate/deduction-guide.cpp b/clang/test/SemaTemplate/deduction-guide.cpp
index d03c783313dd71..39250f0617f4b5 100644
--- a/clang/test/SemaTemplate/deduction-guide.cpp
+++ b/clang/test/SemaTemplate/deduction-guide.cpp
@@ -478,3 +478,53 @@ A a{.f1 = {1}};
 // CHECK-NEXT:     `-DeclRefExpr {{.+}} <col:10> 'int' NonTypeTemplateParm {{.+}} 'N' 'int'
 
 } // namespace GH83368
+
+namespace GH122134 {
+
+template <class, class>
+concept Constraint = true;
+
+template <class T, int> struct Struct {
+  Struct(Constraint<T> auto) {}
+};
+
+template <int N = 0> using Test = Struct<int, N>;
+
+Test test(42);
+
+// CHECK-LABEL: Dumping GH122134::<deduction guide for Test>:
+// CHECK-NEXT: FunctionTemplateDecl {{.*}} implicit <deduction guide for Test>
+// CHECK-NEXT: |-NonTypeTemplateParmDecl {{.*}} 'int' depth 0 index 0 N
+// CHECK-NEXT: | `-TemplateArgument {{.*}} expr '0'
+// CHECK-NEXT: |   `-IntegerLiteral {{.*}} 'int' 0
+// CHECK-NEXT: |-TemplateTypeParmDecl {{.*}} Concept {{.*}} 'Constraint' depth 0 index 1 auto:1
+// CHECK-NEXT: | `-ConceptSpecializationExpr {{.*}} 'bool' Concept {{.*}} 'Constraint'
+// CHECK-NEXT: |   |-ImplicitConceptSpecializationDecl {{.*}}
+// CHECK-NEXT: |   | |-TemplateArgument type 'type-parameter-0-1'
+// CHECK-NEXT: |   | | `-TemplateTypeParmType {{.*}} 'type-parameter-0-1' dependent depth 0 index 1
+// CHECK-NEXT: |   | `-TemplateArgument type 'int'
+// CHECK-NEXT: |   |   `-BuiltinType {{.*}} 'int'
+// CHECK-NEXT: |   |-TemplateArgument {{.*}} type 'auto:1':'type-parameter-0-1'
+// CHECK-NEXT: |   | `-TemplateTypeParmType {{.*}} 'auto:1' dependent depth 0 index 1
+// CHECK-NEXT: |   |   `-TemplateTypeParm {{.*}} 'auto:1'
+// CHECK-NEXT: |   `-TemplateArgument {{.*}} type 'int'
+// CHECK-NEXT: |     `-BuiltinType {{.*}} 'int'
+// CHECK-NEXT: |-TypeTraitExpr {{.*}} 'bool' __is_deducible
+// CHECK-NEXT: | |-DeducedTemplateSpecializationType {{.*}} 'GH122134::Test' dependent
+// CHECK-NEXT: | | `-name: 'GH122134::Test'
+// CHECK-NEXT: | |   `-TypeAliasTemplateDecl {{.*}} Test
+// CHECK-NEXT: | `-TemplateSpecializationType {{.*}} 'Struct<int, N>' dependent
+// CHECK-NEXT: |   |-name: 'Struct':'GH122134::Struct' qualified
+// CHECK-NEXT: |   | `-ClassTemplateDecl {{.*}} Struct
+// CHECK-NEXT: |   |-TemplateArgument type 'int'
+// CHECK-NEXT: |   | `-SubstTemplateTypeParmType {{.*}} 'int' sugar class depth 0 index 0 T
+// CHECK-NEXT: |   |   |-FunctionTemplate {{.*}} '<deduction guide for Struct>'
+// CHECK-NEXT: |   |   `-BuiltinType {{.*}} 'int'
+// CHECK-NEXT: |   `-TemplateArgument expr 'N'
+// CHECK-NEXT: |     `-SubstNonTypeTemplateParmExpr {{.*}} 'int'
+// CHECK-NEXT: |       |-NonTypeTemplateParmDecl {{.*}} 'int' depth 0 index 1
+// CHECK-NEXT: |       `-DeclRefExpr {{.*}} 'int' NonTypeTemplateParm {{.*}} 'N' 'int'
+// CHECK-NEXT: |-CXXDeductionGuideDecl {{.*}} implicit <deduction guide for Test> 'auto (auto:1) -> Struct<int, N>'
+// CHECK-NEXT: | `-ParmVarDecl {{.*}} 'auto:1'
+
+} // namespace GH122134

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with suggestions

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, though please apply Corentin's suggestions.

Copy link
Collaborator

@hokein hokein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, it looks good from myside.

@zyn0217 zyn0217 merged commit fd4f94d into llvm:main Jan 16, 2025
9 checks passed
@zyn0217 zyn0217 deleted the deduction-guide-122134 branch January 16, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang] Crash on CTAD for type alias template involving default template argument and constrained ctor template
5 participants