[Clang] Fix various bugs in alias CTAD transform

[zyn0217]

This fixes the CTAD transform in two ways:

1. When the original deduction guide (i.e. the deduction guide against which we're building the alias CTAD) contains a default template parameter involving a sizeof... expression, we no longer compute the size heuristically. This is based on the fact that we're never going to create any Subst* nodes during CTAD rewrite, which makes that algorithm inapplicable. Instead, we fall through to ordinary template argument substitution wherein we'll try to expand the rewrite packs.

The bug has been facilitated by my previous patch fd4f94d before which we never correctly substituted these default arguments with the transformed template parameters.

2. We failed to handle cases where non-deduced template parameters involve packs. In some cases, the Deduced argument list is a pack containing a single null argument. These should be treated as non-deduced parameters, or we'll crash during transform.

Fixes #123591
Fixes #127539
Fixes #129077
Fixes #129620
Fixes #129998

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

(I'll add more details to the description tomorrow)

Fixes #123591
Fixes #127539
Fixes #129077
Fixes #129998

---

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

4 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateDeductionGuide.cpp (+9-2)
• (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+20-11)
• (modified) clang/lib/Sema/TreeTransform.h (+6)
• (modified) clang/test/SemaCXX/ctad.cpp (+114-1)

diff --git a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
index ee89ee8594bc4..63a100545b5e7 100644
--- a/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
+++ b/clang/lib/Sema/SemaTemplateDeductionGuide.cpp
@@ -1077,7 +1077,11 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
   // !!NOTE: DeduceResults respects the sequence of template parameters of
   // the deduction guide f.
   for (unsigned Index = 0; Index < DeduceResults.size(); ++Index) {
-    if (const auto &D = DeduceResults[Index]; !D.isNull()) // Deduced
+    const auto &D = DeduceResults[Index];
+    bool NonDeduced =
+        D.isNull() || (D.getKind() == TemplateArgument::Pack &&
+                       D.pack_size() == 1 && D.pack_begin()->isNull());
+    if (!NonDeduced)
       DeducedArgs.push_back(D);
     else
       NonDeducedTemplateParamsInFIndex.push_back(Index);
@@ -1141,7 +1145,10 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
   Args.addOuterTemplateArguments(TransformedDeducedAliasArgs);
   for (unsigned Index = 0; Index < DeduceResults.size(); ++Index) {
     const auto &D = DeduceResults[Index];
-    if (D.isNull()) {
+    bool NonDeduced =
+        D.isNull() || (D.getKind() == TemplateArgument::Pack &&
+                       D.pack_size() == 1 && D.pack_begin()->isNull());
+    if (NonDeduced) {
       // 2): Non-deduced template parameters would be substituted later.
       continue;
     }
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 19c27a76b182c..9371b578c8558 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1348,6 +1348,16 @@ std::optional<TemplateDeductionInfo *> Sema::isSFINAEContext() const {
   return std::nullopt;
 }
 
+static TemplateArgument
+getPackSubstitutedTemplateArgument(Sema &S, TemplateArgument Arg) {
+  assert(S.ArgumentPackSubstitutionIndex >= 0);
+  assert(S.ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
+  Arg = Arg.pack_begin()[S.ArgumentPackSubstitutionIndex];
+  if (Arg.isPackExpansion())
+    Arg = Arg.getPackExpansionPattern();
+  return Arg;
+}
+
 //===----------------------------------------------------------------------===/
 // Template Instantiation for Types
 //===----------------------------------------------------------------------===/
@@ -1467,10 +1477,16 @@ namespace {
       }
     }
 
-    static TemplateArgument
+    bool HeuristicallyComputeSizeOfPackExpr() const {
+      return !TemplateArgs.isRewrite();
+    }
+
+    TemplateArgument
     getTemplateArgumentPackPatternForRewrite(const TemplateArgument &TA) {
       if (TA.getKind() != TemplateArgument::Pack)
         return TA;
+      if (SemaRef.ArgumentPackSubstitutionIndex != -1)
+        return getPackSubstitutedTemplateArgument(SemaRef, TA);
       assert(TA.pack_size() == 1 &&
              "unexpected pack arguments in template rewrite");
       TemplateArgument Arg = *TA.pack_begin();
@@ -1630,6 +1646,9 @@ namespace {
       std::vector<TemplateArgument> TArgs;
       switch (Arg.getKind()) {
       case TemplateArgument::Pack:
+        assert(SemaRef.CodeSynthesisContexts.empty() ||
+               SemaRef.CodeSynthesisContexts.back().Kind ==
+                   Sema::CodeSynthesisContext::BuildingDeductionGuides);
         // Literally rewrite the template argument pack, instead of unpacking
         // it.
         for (auto &pack : Arg.getPackAsArray()) {
@@ -1869,16 +1888,6 @@ bool TemplateInstantiator::AlreadyTransformed(QualType T) {
   return true;
 }
 
-static TemplateArgument
-getPackSubstitutedTemplateArgument(Sema &S, TemplateArgument Arg) {
-  assert(S.ArgumentPackSubstitutionIndex >= 0);
-  assert(S.ArgumentPackSubstitutionIndex < (int)Arg.pack_size());
-  Arg = Arg.pack_begin()[S.ArgumentPackSubstitutionIndex];
-  if (Arg.isPackExpansion())
-    Arg = Arg.getPackExpansionPattern();
-  return Arg;
-}
-
 Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
   if (!D)
     return nullptr;
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index b5de98e3989ea..5d96c3fcf92e3 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -3660,6 +3660,8 @@ class TreeTransform {
     return SemaRef.BuildCXXNoexceptExpr(Range.getBegin(), Arg, Range.getEnd());
   }
 
+  bool HeuristicallyComputeSizeOfPackExpr() const { return true; }
+
   /// Build a new expression to compute the length of a parameter pack.
   ExprResult RebuildSizeOfPackExpr(SourceLocation OperatorLoc, NamedDecl *Pack,
                                    SourceLocation PackLoc,
@@ -16095,6 +16097,10 @@ TreeTransform<Derived>::TransformSizeOfPackExpr(SizeOfPackExpr *E) {
   // Try to compute the result without performing a partial substitution.
   std::optional<unsigned> Result = 0;
   for (const TemplateArgument &Arg : PackArgs) {
+    if (!getDerived().HeuristicallyComputeSizeOfPackExpr()) {
+      Result = std::nullopt;
+      break;
+    }
     if (!Arg.isPackExpansion()) {
       Result = *Result + 1;
       continue;
diff --git a/clang/test/SemaCXX/ctad.cpp b/clang/test/SemaCXX/ctad.cpp
index 10806f107b4ee..813d5d7098663 100644
--- a/clang/test/SemaCXX/ctad.cpp
+++ b/clang/test/SemaCXX/ctad.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused-value -std=c++20 %s
-// expected-no-diagnostics
 
 namespace GH64347 {
 
@@ -17,3 +16,117 @@ void k() {
 }
 
 } // namespace GH64347
+
+namespace GH123591 {
+
+
+template < typename... _Types >
+struct variant {
+  template <int N = sizeof...(_Types)>
+  variant(_Types...);
+};
+
+template <class T>
+using AstNode = variant<T, T, T>;
+
+AstNode tree(42, 43, 44);
+
+}
+
+namespace GH123591_2 {
+
+template <int>
+using enable_if_t = char;
+
+template < typename... Types >
+struct variant {
+  template < enable_if_t<sizeof...(Types)>>
+  variant();
+};
+
+template <int>
+using AstNode = variant<>;
+// expected-note@-1 {{couldn't infer template argument ''}} \
+// expected-note@-1 2{{implicit deduction guide declared as}} \
+// expected-note@-1 {{candidate function template not viable}}
+
+
+AstNode tree; // expected-error {{no viable constructor or deduction guide}}
+
+}
+
+namespace GH127539 {
+
+template <class...>
+struct A {
+    template <class... ArgTs>
+    A(ArgTs...) {}
+};
+
+template <class... ArgTs>
+A(ArgTs...) -> A<typename ArgTs::value_type...>;
+
+template <class... Ts>
+using AA = A<Ts...>;
+
+AA a{};
+
+}
+
+namespace GH129077 {
+
+using size_t = decltype(sizeof(0));
+
+struct index_type
+{
+  size_t value{~0ull};
+  index_type() = default;
+  constexpr index_type(size_t i) noexcept : value(i) {}
+};
+
+template <index_type... Extents>
+struct extents
+{
+  constexpr extents(decltype(Extents)...) noexcept {}
+};
+
+template <class... Extents>
+extents(Extents...) -> extents<(requires { Extents::value; } ? Extents{} : ~0ull)...>;
+
+template <index_type... Index>
+using index = extents<Index...>;
+
+int main()
+{
+  extents i{0,0};
+  auto j = extents<64,{}>({}, 42);
+
+  index k{0,0};
+  auto l = index<64,{}>({}, 42);
+
+  return 0;
+}
+
+}
+
+namespace GH129998 {
+
+struct converible_to_one {
+    constexpr operator int() const noexcept { return 1; }
+};
+
+template <int... Extents>
+struct class_template {
+    class_template() = default;
+    constexpr class_template(auto&&...) noexcept {}
+};
+
+template <class... Extents>
+class_template(Extents...) -> class_template<(true ? 0 : +Extents{})...>;
+
+template <int... Extents>
+using alias_template = class_template<Extents...>;
+
+alias_template var2{converible_to_one{}, 2};
+
+}

[cor3ntin]

Should that be a function? Should isNull be changed? When do we get in that situation?

[zyn0217]

Added explanations.

[cor3ntin]

static is gone

[zyn0217]

It is intended because we need to access SemaRef.ArgumentPackSubstitutionIndex

[cor3ntin]

This will need a comment

[cor3ntin]

I think I would prefer to add a TransformSizeOfPackExpr override in TemplateInstantiator that calls
back to the base implementation with a bool parameter, or something like that

[zyn0217]

I choose to extract a function ComputeSizeOfPackExprWithoutSubstitution, since there is some common logic for extracting the pack pattern beforehand. Thus overriding TransformSizeOfPackExpr might not be suitable due to duplication.

(We can't add boolean parameters to the function either, as it is expanded from macros.)

[cor3ntin]

getSubstitutedTemplateArgumentPattern would be a better name. Please add a comment too

[zyn0217]

This is pre-existing - do I have to rename that?

[mizvekov]

LGTM, Thanks.

[zyn0217]

I'll merge without a release note, instead I plan to backport it to the 20 release along with a changelog.

[zyn0217]

The commit message is mysteriously gone when merging with the github app 🥲

[llvm-ci]

LLVM Buildbot has detected a new failure on builder clang-armv8-quick running on linaro-clang-armv8-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/13778

Here is the relevant piece of the build log for the reference

Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'Clang :: SemaCXX/ctad.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/21/include -nostdsysteminc -fsyntax-only -verify -Wno-unused-value -std=c++20 /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/SemaCXX/ctad.cpp
+ /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/21/include -nostdsysteminc -fsyntax-only -verify -Wno-unused-value -std=c++20 /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/SemaCXX/ctad.cpp
error: 'expected-error' diagnostics seen but not expected: 
  File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/SemaCXX/ctad.cpp Line 82: constant expression evaluates to 18446744073709551615 which cannot be narrowed to type 'size_t' (aka 'unsigned int')
error: 'expected-warning' diagnostics seen but not expected: 
  File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/SemaCXX/ctad.cpp Line 82: implicit conversion from 'unsigned long long' to 'size_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295
  File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/SemaCXX/ctad.cpp Line 82: implicit conversion from 'unsigned long long' to 'size_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295
error: 'expected-note' diagnostics seen but not expected: 
  File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/SemaCXX/ctad.cpp Line 82: insert an explicit cast to silence this issue
4 errors generated.

--

********************