[Clang] Fix handling of pack indexing types in constraints of redeclaration

cor3ntin · cor3ntin · commit 24e912a3ded1 · 2025-05-09T12:27:37.000+02:00
When checking the constraints of a potential redeclaration we inject the template parameter as template argument to compare them. This would make clang: - try to expand these injected parameters - inject a SubstTemplateTypeParmPackType Why expanding the pack indexing is wasteful, it's recoverable, as it was only partially expanded. However, creating SubstTemplateTypeParmPackType nodes was problematic as, before we can establish that declarations are identical, these nodes won't canonicalize to the same types (at these point they have unrelated associated decl). To avoid these issues, we track whether a set of template arguments are injected template parameters. And if they are skip some substitutions/instantiation. Cleanup pack indexing profiling as a drive-by. Fixes #138255
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -667,6 +667,7 @@ Bug Fixes to C++ Support
   whose type depends on itself. (#GH51347), (#GH55872)
 - Improved parser recovery of invalid requirement expressions. In turn, this
   fixes crashes from follow-on processing of the invalid requirement. (#GH138820)
+- Fixed the handling of pack indexing types in the constraints of a member function redeclaration. (#GH138255)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
@@ -221,7 +221,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
   mutable llvm::ContextualFoldingSet<DependentDecltypeType, ASTContext &>
       DependentDecltypeTypes;
 
-  mutable llvm::FoldingSet<PackIndexingType> DependentPackIndexingTypes;
+  mutable llvm::ContextualFoldingSet<PackIndexingType, ASTContext &>
+      DependentPackIndexingTypes;
 
   mutable llvm::FoldingSet<TemplateTypeParmType> TemplateTypeParmTypes;
   mutable llvm::FoldingSet<ObjCTypeParamType> ObjCTypeParamTypes;
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
@@ -5976,7 +5976,6 @@ class PackIndexingType final
       private llvm::TrailingObjects<PackIndexingType, QualType> {
   friend TrailingObjects;
 
-  const ASTContext &Context;
   QualType Pattern;
   Expr *IndexExpr;
 
@@ -5987,9 +5986,8 @@ class PackIndexingType final
 
 protected:
   friend class ASTContext; // ASTContext creates these.
-  PackIndexingType(const ASTContext &Context, QualType Canonical,
-                   QualType Pattern, Expr *IndexExpr, bool FullySubstituted,
-                   ArrayRef<QualType> Expansions = {});
+  PackIndexingType(QualType Canonical, QualType Pattern, Expr *IndexExpr,
+                   bool FullySubstituted, ArrayRef<QualType> Expansions = {});
 
 public:
   Expr *getIndexExpr() const { return IndexExpr; }
@@ -6024,12 +6022,7 @@ class PackIndexingType final
     return T->getTypeClass() == PackIndexing;
   }
 
-  void Profile(llvm::FoldingSetNodeID &ID) {
-    if (hasSelectedType())
-      getSelectedType().Profile(ID);
-    else
-      Profile(ID, Context, getPattern(), getIndexExpr(), isFullySubstituted());
-  }
+  void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context);
   static void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
                       QualType Pattern, Expr *E, bool FullySubstituted);
 
diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h
@@ -24,6 +24,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include <cassert>
 #include <optional>
+#include <tuple>
 #include <utility>
 
 namespace clang {
@@ -76,9 +77,17 @@ enum class TemplateSubstitutionKind : char {
   class MultiLevelTemplateArgumentList {
     /// The template argument list at a certain template depth
 
+    enum ListProperties {
+      /// A 'Final' substitution.
+      IsFinal = 0x1,
+      /// Track if the arguments are injected template parameters.
+      /// Injected template parameters should not be expanded.
+      ArgumentsAreInjectedTemplateParams = 0x02,
+    };
+
     using ArgList = ArrayRef<TemplateArgument>;
     struct ArgumentListLevel {
-      llvm::PointerIntPair<Decl *, 1, bool> AssociatedDeclAndFinal;
+      llvm::PointerIntPair<Decl *, 3, unsigned> AssociatedDeclAndProperties;
       ArgList Args;
     };
     using ContainerType = SmallVector<ArgumentListLevel, 4>;
@@ -161,11 +170,19 @@ enum class TemplateSubstitutionKind : char {
     /// A template-like entity which owns the whole pattern being substituted.
     /// This will usually own a set of template parameters, or in some
     /// cases might even be a template parameter itself.
-    std::pair<Decl *, bool> getAssociatedDecl(unsigned Depth) const {
+    std::tuple<Decl *, bool, bool> getAssociatedDecl(unsigned Depth) const {
+      assert(NumRetainedOuterLevels <= Depth && Depth < getNumLevels());
+      auto AD = TemplateArgumentLists[getNumLevels() - Depth - 1]
+                    .AssociatedDeclAndProperties;
+      return {AD.getPointer(), AD.getInt() & IsFinal,
+              AD.getInt() & ArgumentsAreInjectedTemplateParams};
+    }
+
+    bool ArgumentsAreInjectedParameters(unsigned Depth) const {
       assert(NumRetainedOuterLevels <= Depth && Depth < getNumLevels());
       auto AD = TemplateArgumentLists[getNumLevels() - Depth - 1]
-                    .AssociatedDeclAndFinal;
-      return {AD.getPointer(), AD.getInt()};
+                    .AssociatedDeclAndProperties;
+      return AD.getInt() & ArgumentsAreInjectedTemplateParams;
     }
 
     /// Determine whether there is a non-NULL template argument at the
@@ -205,16 +222,15 @@ enum class TemplateSubstitutionKind : char {
 
     /// Add a new outmost level to the multi-level template argument
     /// list.
-    /// A 'Final' substitution means that Subst* nodes won't be built
-    /// for the replacements.
-    void addOuterTemplateArguments(Decl *AssociatedDecl, ArgList Args,
-                                   bool Final) {
+    void
+    addOuterTemplateArguments(Decl *AssociatedDecl, ArgList Args, bool Final,
+                              bool ArgumentsAreInjectedTemplateParams = false) {
       assert(!NumRetainedOuterLevels &&
              "substituted args outside retained args?");
       assert(getKind() == TemplateSubstitutionKind::Specialization);
       TemplateArgumentLists.push_back(
           {{AssociatedDecl ? AssociatedDecl->getCanonicalDecl() : nullptr,
-            Final},
+            getProperties(Final, ArgumentsAreInjectedTemplateParams)},
            Args});
     }
 
@@ -239,15 +255,17 @@ enum class TemplateSubstitutionKind : char {
              "Replacing in an empty list?");
 
       if (!TemplateArgumentLists.empty()) {
-        assert((TemplateArgumentLists[0].AssociatedDeclAndFinal.getPointer() ||
-                TemplateArgumentLists[0].AssociatedDeclAndFinal.getPointer() ==
+        assert((TemplateArgumentLists[0]
+                    .AssociatedDeclAndProperties.getPointer() ||
+                TemplateArgumentLists[0]
+                        .AssociatedDeclAndProperties.getPointer() ==
                     AssociatedDecl) &&
                "Trying to change incorrect declaration?");
         TemplateArgumentLists[0].Args = Args;
       } else {
         --NumRetainedOuterLevels;
         TemplateArgumentLists.push_back(
-            {{AssociatedDecl, /*Final=*/false}, Args});
+            {{AssociatedDecl, /*Properties=*/0}, Args});
       }
     }
 
@@ -292,6 +310,16 @@ enum class TemplateSubstitutionKind : char {
         llvm::errs() << "\n";
       }
     }
+
+    static unsigned getProperties(bool IsFinal, bool IsInjected) {
+      unsigned Props = 0;
+      if (IsFinal)
+        Props |= MultiLevelTemplateArgumentList::IsFinal;
+      if (IsInjected)
+        Props |=
+            MultiLevelTemplateArgumentList::ArgumentsAreInjectedTemplateParams;
+      return Props;
+    }
   };
 
   /// The context in which partial ordering of function templates occurs.
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
@@ -940,7 +940,7 @@ ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM,
       DependentSizedMatrixTypes(this_()),
       FunctionProtoTypes(this_(), FunctionProtoTypesLog2InitSize),
       DependentTypeOfExprTypes(this_()), DependentDecltypeTypes(this_()),
-      TemplateSpecializationTypes(this_()),
+      DependentPackIndexingTypes(this_()), TemplateSpecializationTypes(this_()),
       DependentTemplateSpecializationTypes(this_()),
       DependentBitIntTypes(this_()), SubstTemplateTemplateParmPacks(this_()),
       DeducedTemplates(this_()), ArrayParameterTypes(this_()),
@@ -6433,34 +6433,29 @@ QualType ASTContext::getPackIndexingType(QualType Pattern, Expr *IndexExpr,
                                          ArrayRef<QualType> Expansions,
                                          UnsignedOrNone Index) const {
   QualType Canonical;
-  if (FullySubstituted && Index) {
+  if (FullySubstituted && Index)
     Canonical = getCanonicalType(Expansions[*Index]);
-  } else {
-    llvm::FoldingSetNodeID ID;
-    PackIndexingType::Profile(ID, *this, Pattern.getCanonicalType(), IndexExpr,
-                              FullySubstituted);
-    void *InsertPos = nullptr;
-    PackIndexingType *Canon =
-        DependentPackIndexingTypes.FindNodeOrInsertPos(ID, InsertPos);
-    if (!Canon) {
-      void *Mem = Allocate(
-          PackIndexingType::totalSizeToAlloc<QualType>(Expansions.size()),
-          TypeAlignment);
-      Canon = new (Mem)
-          PackIndexingType(*this, QualType(), Pattern.getCanonicalType(),
-                           IndexExpr, FullySubstituted, Expansions);
-      DependentPackIndexingTypes.InsertNode(Canon, InsertPos);
-    }
-    Canonical = QualType(Canon, 0);
-  }
+  else if (!Pattern.isCanonical())
+    Canonical = getPackIndexingType(Pattern.getCanonicalType(), IndexExpr,
+                                    FullySubstituted, Expansions, Index);
+
+  llvm::FoldingSetNodeID ID;
+  PackIndexingType::Profile(ID, *this, Pattern, IndexExpr, FullySubstituted);
+  void *InsertPos = nullptr;
+  PackIndexingType *Canon =
+      DependentPackIndexingTypes.FindNodeOrInsertPos(ID, InsertPos);
+  if (Canon)
+    return QualType(Canon, 0);
 
   void *Mem =
       Allocate(PackIndexingType::totalSizeToAlloc<QualType>(Expansions.size()),
                TypeAlignment);
-  auto *T = new (Mem) PackIndexingType(*this, Canonical, Pattern, IndexExpr,
-                                       FullySubstituted, Expansions);
-  Types.push_back(T);
-  return QualType(T, 0);
+
+  Canon = new (Mem) PackIndexingType(Canonical, Pattern, IndexExpr,
+                                     FullySubstituted, Expansions);
+  DependentPackIndexingTypes.InsertNode(Canon, InsertPos);
+  Types.push_back(Canon);
+  return QualType(Canon, 0);
 }
 
 /// getUnaryTransformationType - We don't unique these, since the memory
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
@@ -4126,14 +4126,14 @@ void DependentDecltypeType::Profile(llvm::FoldingSetNodeID &ID,
   E->Profile(ID, Context, true);
 }
 
-PackIndexingType::PackIndexingType(const ASTContext &Context,
-                                   QualType Canonical, QualType Pattern,
+PackIndexingType::PackIndexingType(QualType Canonical, QualType Pattern,
                                    Expr *IndexExpr, bool FullySubstituted,
                                    ArrayRef<QualType> Expansions)
     : Type(PackIndexing, Canonical,
            computeDependence(Pattern, IndexExpr, Expansions)),
-      Context(Context), Pattern(Pattern), IndexExpr(IndexExpr),
-      Size(Expansions.size()), FullySubstituted(FullySubstituted) {
+      Pattern(Pattern), IndexExpr(IndexExpr), Size(Expansions.size()),
+      FullySubstituted(FullySubstituted) {
+
   llvm::uninitialized_copy(Expansions, getTrailingObjects<QualType>());
 }
 
@@ -4174,6 +4174,14 @@ PackIndexingType::computeDependence(QualType Pattern, Expr *IndexExpr,
   return TD;
 }
 
+void PackIndexingType::Profile(llvm::FoldingSetNodeID &ID,
+                               const ASTContext &Context) {
+  if (hasSelectedType() && isFullySubstituted())
+    getSelectedType().Profile(ID);
+  else
+    Profile(ID, Context, getPattern(), getIndexExpr(), isFullySubstituted());
+}
+
 void PackIndexingType::Profile(llvm::FoldingSetNodeID &ID,
                                const ASTContext &Context, QualType Pattern,
                                Expr *E, bool FullySubstituted) {
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -338,7 +338,7 @@ Response HandleFunctionTemplateDecl(Sema &SemaRef,
         const_cast<FunctionTemplateDecl *>(FTD),
         const_cast<FunctionTemplateDecl *>(FTD)->getInjectedTemplateArgs(
             SemaRef.Context),
-        /*Final=*/false);
+        /*Final=*/false, /*InjectedTemplateParams=*/true);
 
     NestedNameSpecifier *NNS = FTD->getTemplatedDecl()->getQualifier();
 
@@ -363,18 +363,20 @@ Response HandleFunctionTemplateDecl(Sema &SemaRef,
           // This meets the contract in
           // TreeTransform::TryExpandParameterPacks that the template arguments
           // for unexpanded parameters should be of a Pack kind.
+          bool Injected = false;
           if (TSTy->isCurrentInstantiation()) {
             auto *RD = TSTy->getCanonicalTypeInternal()->getAsCXXRecordDecl();
-            if (ClassTemplateDecl *CTD = RD->getDescribedClassTemplate())
+            if (ClassTemplateDecl *CTD = RD->getDescribedClassTemplate()) {
               Arguments = CTD->getInjectedTemplateArgs(SemaRef.Context);
-            else if (auto *Specialization =
-                         dyn_cast<ClassTemplateSpecializationDecl>(RD))
+              Injected = true;
+            } else if (auto *Specialization =
+                           dyn_cast<ClassTemplateSpecializationDecl>(RD))
               Arguments =
                   Specialization->getTemplateInstantiationArgs().asArray();
           }
           Result.addOuterTemplateArguments(
               TSTy->getTemplateName().getAsTemplateDecl(), Arguments,
-              /*Final=*/false);
+              /*Final=*/false, /*InjectedTemplateParams=*/Injected);
         }
       }
 
@@ -399,7 +401,7 @@ Response HandleRecordDecl(Sema &SemaRef, const CXXRecordDecl *Rec,
       Result.addOuterTemplateArguments(
           const_cast<CXXRecordDecl *>(Rec),
           ClassTemplate->getInjectedTemplateArgs(SemaRef.Context),
-          /*Final=*/false);
+          /*Final=*/false, /*InjectedTemplateParams=*/true);
   }
 
   if (const MemberSpecializationInfo *MSInfo =
@@ -1476,8 +1478,8 @@ namespace {
       }
     }
 
-    TemplateArgument
-    getTemplateArgumentPackPatternForRewrite(const TemplateArgument &TA) {
+    TemplateArgument getTemplateArgumentorUnsubstitutedExpansionPattern(
+        const TemplateArgument &TA) {
       if (TA.getKind() != TemplateArgument::Pack)
         return TA;
       if (SemaRef.ArgPackSubstIndex)
@@ -1902,7 +1904,9 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) {
 
       TemplateArgument Arg = TemplateArgs(TTP->getDepth(), TTP->getPosition());
 
-      if (TTP->isParameterPack()) {
+      if (TemplateArgs.ArgumentsAreInjectedParameters(TTP->getDepth())) {
+        Arg = getTemplateArgumentorUnsubstitutedExpansionPattern(Arg);
+      } else if (TTP->isParameterPack()) {
         assert(Arg.getKind() == TemplateArgument::Pack &&
                "Missing argument pack");
         Arg = getPackSubstitutedTemplateArgument(getSema(), Arg);
@@ -2054,20 +2058,23 @@ TemplateName TemplateInstantiator::TransformTemplateName(
       if (TemplateArgs.isRewrite()) {
         // We're rewriting the template parameter as a reference to another
         // template parameter.
-        Arg = getTemplateArgumentPackPatternForRewrite(Arg);
+        Arg = getTemplateArgumentorUnsubstitutedExpansionPattern(Arg);
         assert(Arg.getKind() == TemplateArgument::Template &&
                "unexpected nontype template argument kind in template rewrite");
         return Arg.getAsTemplate();
       }
 
-      auto [AssociatedDecl, Final] =
+      auto [AssociatedDecl, Final, ArgumentsAreInjectedTemplateParams] =
           TemplateArgs.getAssociatedDecl(TTP->getDepth());
       UnsignedOrNone PackIndex = std::nullopt;
       if (TTP->isParameterPack()) {
         assert(Arg.getKind() == TemplateArgument::Pack &&
                "Missing argument pack");
 
-        if (!getSema().ArgPackSubstIndex) {
+        if (ArgumentsAreInjectedTemplateParams)
+          Arg = getTemplateArgumentorUnsubstitutedExpansionPattern(Arg);
+
+        else if (!getSema().ArgPackSubstIndex) {
           // We have the template argument pack to substitute, but we're not
           // actually expanding the enclosing pack expansion yet. So, just
           // keep the entire argument pack.
@@ -2131,15 +2138,15 @@ TemplateInstantiator::TransformTemplateParmRefExpr(DeclRefExpr *E,
   if (TemplateArgs.isRewrite()) {
     // We're rewriting the template parameter as a reference to another
     // template parameter.
-    Arg = getTemplateArgumentPackPatternForRewrite(Arg);
+    Arg = getTemplateArgumentorUnsubstitutedExpansionPattern(Arg);
     assert(Arg.getKind() == TemplateArgument::Expression &&
            "unexpected nontype template argument kind in template rewrite");
     // FIXME: This can lead to the same subexpression appearing multiple times
     // in a complete expression.
     return Arg.getAsExpr();
   }
 
-  auto [AssociatedDecl, Final] =
+  auto [AssociatedDecl, Final, ArgumentsAreInjectedTemplateParams] =
       TemplateArgs.getAssociatedDecl(NTTP->getDepth());
   UnsignedOrNone PackIndex = std::nullopt;
   if (NTTP->isParameterPack()) {
@@ -2584,22 +2591,30 @@ TemplateInstantiator::TransformTemplateTypeParmType(TypeLocBuilder &TLB,
     if (TemplateArgs.isRewrite()) {
       // We're rewriting the template parameter as a reference to another
       // template parameter.
-      Arg = getTemplateArgumentPackPatternForRewrite(Arg);
+      Arg = getTemplateArgumentorUnsubstitutedExpansionPattern(Arg);
       assert(Arg.getKind() == TemplateArgument::Type &&
              "unexpected nontype template argument kind in template rewrite");
       QualType NewT = Arg.getAsType();
       TLB.pushTrivial(SemaRef.Context, NewT, TL.getNameLoc());
       return NewT;
     }
 
-    auto [AssociatedDecl, Final] =
+    auto [AssociatedDecl, Final, ArgumentsAreInjectedTemplateParams] =
         TemplateArgs.getAssociatedDecl(T->getDepth());
     UnsignedOrNone PackIndex = std::nullopt;
     if (T->isParameterPack()) {
       assert(Arg.getKind() == TemplateArgument::Pack &&
              "Missing argument pack");
 
       if (!getSema().ArgPackSubstIndex) {
+
+        if (ArgumentsAreInjectedTemplateParams) {
+          TemplateTypeParmTypeLoc NewTL =
+              TLB.push<TemplateTypeParmTypeLoc>(TL.getType());
+          NewTL.setNameLoc(TL.getNameLoc());
+          return TL.getType();
+        }
+
         // We have the template argument pack, but we're not expanding the
         // enclosing pack expansion yet. Just save the template argument
         // pack for later substitution.
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
