Skip to content

Conversation

@mizvekov
Copy link
Contributor

This changes the TemplateArgument representation to hold a flag indicating whether a template argument of expression type is supposed to be canonical or not.

This gets one step closer to solving #92292

This still doesn't try to unique as-written TSTs. While this would increase the amount of memory savings and make code dealing with the AST more well-behaved, profiling template argument lists is still too expensive for this to be worthwhile, at least for now. Without this uniquing, this patch stands neutral in terms of performance impact.

This also fixes the context creation of TSTs, so that they don't in some cases get incorrectly flagged as sugar over their own canonical form. This is captured in the test expectation change of some AST dumps.

This fixes some places which were unnecessarily canonicalizing these TSTs.

@mizvekov mizvekov self-assigned this Apr 10, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Apr 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 10, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangd

Author: Matheus Izvekov (mizvekov)

Changes

This changes the TemplateArgument representation to hold a flag indicating whether a template argument of expression type is supposed to be canonical or not.

This gets one step closer to solving #92292

This still doesn't try to unique as-written TSTs. While this would increase the amount of memory savings and make code dealing with the AST more well-behaved, profiling template argument lists is still too expensive for this to be worthwhile, at least for now. Without this uniquing, this patch stands neutral in terms of performance impact.

This also fixes the context creation of TSTs, so that they don't in some cases get incorrectly flagged as sugar over their own canonical form. This is captured in the test expectation change of some AST dumps.

This fixes some places which were unnecessarily canonicalizing these TSTs.


Patch is 90.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135119.diff

29 Files Affected:

  • (modified) clang-tools-extra/clangd/AST.cpp (+2-1)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/AST/ASTContext.h (+31-15)
  • (modified) clang/include/clang/AST/PropertiesBase.td (+4-1)
  • (modified) clang/include/clang/AST/TemplateBase.h (+11-2)
  • (modified) clang/include/clang/AST/Type.h (+3-4)
  • (modified) clang/include/clang/AST/TypeProperties.td (+5-25)
  • (modified) clang/include/clang/Serialization/ASTRecordReader.h (+4)
  • (modified) clang/lib/AST/ASTContext.cpp (+129-107)
  • (modified) clang/lib/AST/ASTDiagnostic.cpp (+4-4)
  • (modified) clang/lib/AST/ASTImporter.cpp (+21-13)
  • (modified) clang/lib/AST/DeclTemplate.cpp (+5-2)
  • (modified) clang/lib/AST/QualTypeNames.cpp (+2-1)
  • (modified) clang/lib/AST/TemplateBase.cpp (+15-4)
  • (modified) clang/lib/AST/Type.cpp (+27-23)
  • (modified) clang/lib/Sema/SemaCXXScopeSpec.cpp (+8-10)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+17-3)
  • (modified) clang/lib/Sema/SemaLookup.cpp (+2-1)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+78-84)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+27-18)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+6-5)
  • (modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+2-1)
  • (modified) clang/lib/Sema/TreeTransform.h (+9-4)
  • (modified) clang/test/CXX/class.derived/class.derived.general/p2.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.decls/temp.class.spec/p6.cpp (+2-2)
  • (modified) clang/test/SemaCXX/undefined-partial-specialization.cpp (+1-1)
  • (modified) clang/test/SemaTemplate/make_integer_seq.cpp (+16-45)
  • (modified) clang/test/SemaTemplate/type_pack_element.cpp (+15-42)
  • (modified) clang/unittests/AST/TypePrinterTest.cpp (+3-3)
diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index 66b587f00ff4a..3b991e5e9013f 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -439,7 +439,8 @@ QualType declaredType(const TypeDecl *D) {
   if (const auto *CTSD = llvm::dyn_cast<ClassTemplateSpecializationDecl>(D))
     if (const auto *Args = CTSD->getTemplateArgsAsWritten())
       return Context.getTemplateSpecializationType(
-          TemplateName(CTSD->getSpecializedTemplate()), Args->arguments());
+          TemplateName(CTSD->getSpecializedTemplate()), Args->arguments(),
+          /*CanonicalArgs=*/std::nullopt);
   return Context.getTypeDeclType(D);
 }
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index cd16641c25ed8..204d41476232c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -280,6 +280,8 @@ Improvements to Clang's diagnostics
 - Clang now better preserves the sugared types of pointers to member.
 - Clang now better preserves the presence of the template keyword with dependent
   prefixes.
+- Clang now in more cases avoids printing 'type-parameter-X-X' instead of the name of
+  the template parameter.
 - Clang now respects the current language mode when printing expressions in
   diagnostics. This fixes a bunch of `bool` being printed as `_Bool`, and also
   a bunch of HLSL types being printed as their C++ equivalents.
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 3ff9f308f3a5e..c339bb698e099 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -364,9 +364,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
                                      const ASTContext&>
     CanonTemplateTemplateParms;
 
-  TemplateTemplateParmDecl *
-    getCanonicalTemplateTemplateParmDecl(TemplateTemplateParmDecl *TTP) const;
-
   /// The typedef for the __int128_t type.
   mutable TypedefDecl *Int128Decl = nullptr;
 
@@ -1808,22 +1805,26 @@ class ASTContext : public RefCountedBase<ASTContext> {
                           bool ParameterPack,
                           TemplateTypeParmDecl *ParmDecl = nullptr) const;
 
-  QualType getTemplateSpecializationType(TemplateName T,
-                                         ArrayRef<TemplateArgument> Args,
-                                         QualType Canon = QualType()) const;
+  QualType getCanonicalTemplateSpecializationType(
+      TemplateName T, ArrayRef<TemplateArgument> CanonicalArgs) const;
 
   QualType
-  getCanonicalTemplateSpecializationType(TemplateName T,
-                                         ArrayRef<TemplateArgument> Args) const;
+  getTemplateSpecializationType(TemplateName T,
+                                ArrayRef<TemplateArgument> SpecifiedArgs,
+                                ArrayRef<TemplateArgument> CanonicalArgs,
+                                QualType Underlying = QualType()) const;
 
-  QualType getTemplateSpecializationType(TemplateName T,
-                                         ArrayRef<TemplateArgumentLoc> Args,
-                                         QualType Canon = QualType()) const;
+  QualType
+  getTemplateSpecializationType(TemplateName T,
+                                ArrayRef<TemplateArgumentLoc> SpecifiedArgs,
+                                ArrayRef<TemplateArgument> CanonicalArgs,
+                                QualType Canon = QualType()) const;
 
-  TypeSourceInfo *
-  getTemplateSpecializationTypeInfo(TemplateName T, SourceLocation TLoc,
-                                    const TemplateArgumentListInfo &Args,
-                                    QualType Canon = QualType()) const;
+  TypeSourceInfo *getTemplateSpecializationTypeInfo(
+      TemplateName T, SourceLocation TLoc,
+      const TemplateArgumentListInfo &SpecifiedArgs,
+      ArrayRef<TemplateArgument> CanonicalArgs,
+      QualType Canon = QualType()) const;
 
   QualType getParenType(QualType NamedType) const;
 
@@ -2939,6 +2940,21 @@ class ASTContext : public RefCountedBase<ASTContext> {
   TemplateArgument getCanonicalTemplateArgument(const TemplateArgument &Arg)
     const;
 
+  /// Canonicalize the given template argument list.
+  ///
+  /// Returns true if any arguments were non-canonical, false otherwise.
+  bool
+  canonicalizeTemplateArguments(MutableArrayRef<TemplateArgument> Args) const;
+
+  /// Canonicalize the given TemplateTemplateParmDecl.
+  TemplateTemplateParmDecl *
+  getCanonicalTemplateTemplateParmDecl(TemplateTemplateParmDecl *TTP) const;
+
+  TemplateTemplateParmDecl *findCanonicalTemplateTemplateParmDeclInternal(
+      TemplateTemplateParmDecl *TTP) const;
+  TemplateTemplateParmDecl *insertCanonicalTemplateTemplateParmDeclInternal(
+      TemplateTemplateParmDecl *CanonTTP) const;
+
   /// Type Query functions.  If the type is an instance of the specified class,
   /// return the Type pointer for the underlying maximally pretty type.  This
   /// is a member of ASTContext because this may need to do some amount of
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index 90537d47dd9c9..33336d57b6298 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -877,11 +877,14 @@ let Class = PropertyTypeCase<TemplateArgument, "Expression"> in {
   def : Property<"expression", ExprRef> {
     let Read = [{ node.getAsExpr() }];
   }
+  def : Property<"IsCanonical", Bool> {
+    let Read = [{ node.isCanonicalExpr() }];
+  }
   def : Property<"isDefaulted", Bool> {
     let Read = [{ node.getIsDefaulted() }];
   }
   def : Creator<[{
-    return TemplateArgument(expression, isDefaulted);
+    return TemplateArgument(expression, IsCanonical, isDefaulted);
   }]>;
 }
 let Class = PropertyTypeCase<TemplateArgument, "Pack"> in {
diff --git a/clang/include/clang/AST/TemplateBase.h b/clang/include/clang/AST/TemplateBase.h
index bea624eb04942..f833a1ec02455 100644
--- a/clang/include/clang/AST/TemplateBase.h
+++ b/clang/include/clang/AST/TemplateBase.h
@@ -168,6 +168,8 @@ class TemplateArgument {
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsDefaulted : 1;
     uintptr_t V;
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned IsCanonicalExpr : 1;
   };
   union {
     struct DA DeclArg;
@@ -187,7 +189,8 @@ class TemplateArgument {
 
 public:
   /// Construct an empty, invalid template argument.
-  constexpr TemplateArgument() : TypeOrValue({Null, 0, /* IsDefaulted */ 0}) {}
+  constexpr TemplateArgument()
+      : TypeOrValue{Null, /*IsDefaulted=*/0, /*V=*/0, /*IsCanonicalExpr=*/0} {}
 
   /// Construct a template type argument.
   TemplateArgument(QualType T, bool isNullPtr = false,
@@ -262,10 +265,11 @@ class TemplateArgument {
   /// This form of template argument only occurs in template argument
   /// lists used for dependent types and for expression; it will not
   /// occur in a non-dependent, canonical template argument list.
-  explicit TemplateArgument(Expr *E, bool IsDefaulted = false) {
+  TemplateArgument(Expr *E, bool IsCanonical, bool IsDefaulted = false) {
     TypeOrValue.Kind = Expression;
     TypeOrValue.IsDefaulted = IsDefaulted;
     TypeOrValue.V = reinterpret_cast<uintptr_t>(E);
+    TypeOrValue.IsCanonicalExpr = IsCanonical;
   }
 
   /// Construct a template argument that is a template argument pack.
@@ -407,6 +411,11 @@ class TemplateArgument {
     return reinterpret_cast<Expr *>(TypeOrValue.V);
   }
 
+  bool isCanonicalExpr() const {
+    assert(getKind() == Expression && "Unexpected kind");
+    return TypeOrValue.IsCanonicalExpr;
+  }
+
   /// Iterator that traverses the elements of a template argument pack.
   using pack_iterator = const TemplateArgument *;
 
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 9f6189440fabf..dc57170bf9160 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -6676,10 +6676,9 @@ class TemplateSpecializationType : public Type, public llvm::FoldingSetNode {
   /// replacement must, recursively, be one of these).
   TemplateName Template;
 
-  TemplateSpecializationType(TemplateName T,
+  TemplateSpecializationType(TemplateName T, bool IsAlias,
                              ArrayRef<TemplateArgument> Args,
-                             QualType Canon,
-                             QualType Aliased);
+                             QualType Underlying);
 
 public:
   /// Determine whether any of the given template arguments are dependent.
@@ -6747,7 +6746,7 @@ class TemplateSpecializationType : public Type, public llvm::FoldingSetNode {
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Ctx);
   static void Profile(llvm::FoldingSetNodeID &ID, TemplateName T,
-                      ArrayRef<TemplateArgument> Args,
+                      ArrayRef<TemplateArgument> Args, QualType Underlying,
                       const ASTContext &Context);
 
   static bool classof(const Type *T) {
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 66d490850678a..3bf9239e9cbf5 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -737,39 +737,19 @@ let Class = DependentAddressSpaceType in {
 }
 
 let Class = TemplateSpecializationType in {
-  def : Property<"dependent", Bool> {
-    let Read = [{ node->isDependentType() }];
-  }
   def : Property<"templateName", TemplateName> {
     let Read = [{ node->getTemplateName() }];
   }
-  def : Property<"templateArguments", Array<TemplateArgument>> {
+  def : Property<"args", Array<TemplateArgument>> {
     let Read = [{ node->template_arguments() }];
   }
-  def : Property<"underlyingType", Optional<QualType>> {
-    let Read = [{
-      node->isTypeAlias()
-        ? std::optional<QualType>(node->getAliasedType())
-        : node->isCanonicalUnqualified()
-            ? std::nullopt
-            : std::optional<QualType>(node->getCanonicalTypeInternal())
-    }];
+  def : Property<"UnderlyingType", QualType> {
+    let Read = [{ node->isCanonicalUnqualified() ? QualType() :
+                                                   node->desugar() }];
   }
 
   def : Creator<[{
-    QualType result;
-    if (!underlyingType) {
-      result = ctx.getCanonicalTemplateSpecializationType(templateName,
-                                                          templateArguments);
-    } else {
-      result = ctx.getTemplateSpecializationType(templateName,
-                                                 templateArguments,
-                                                 *underlyingType);
-    }
-    if (dependent)
-      const_cast<Type *>(result.getTypePtr())
-          ->addDependence(TypeDependence::DependentInstantiation);
-    return result;
+    return ctx.getTemplateSpecializationType(templateName, args, std::nullopt, UnderlyingType);
   }]>;
 }
 
diff --git a/clang/include/clang/Serialization/ASTRecordReader.h b/clang/include/clang/Serialization/ASTRecordReader.h
index 141804185083f..90993687e69a0 100644
--- a/clang/include/clang/Serialization/ASTRecordReader.h
+++ b/clang/include/clang/Serialization/ASTRecordReader.h
@@ -247,6 +247,10 @@ class ASTRecordReader
   void readTemplateArgumentList(SmallVectorImpl<TemplateArgument> &TemplArgs,
                                 bool Canonicalize = false);
 
+  /// Read a template argument list.
+  const TemplateArgumentList *
+  readTemplateArgumentList(bool Canonicalize = false);
+
   /// Read a UnresolvedSet structure, advancing Idx.
   void readUnresolvedSet(LazyASTUnresolvedSet &Set);
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 0fe941e063d49..6ad49c27b34d1 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -844,6 +844,31 @@ ASTContext::getCanonicalTemplateTemplateParmDecl(
   return CanonTTP;
 }
 
+TemplateTemplateParmDecl *
+ASTContext::findCanonicalTemplateTemplateParmDeclInternal(
+    TemplateTemplateParmDecl *TTP) const {
+  llvm::FoldingSetNodeID ID;
+  CanonicalTemplateTemplateParm::Profile(ID, *this, TTP);
+  void *InsertPos = nullptr;
+  CanonicalTemplateTemplateParm *Canonical =
+      CanonTemplateTemplateParms.FindNodeOrInsertPos(ID, InsertPos);
+  return Canonical ? Canonical->getParam() : nullptr;
+}
+
+TemplateTemplateParmDecl *
+ASTContext::insertCanonicalTemplateTemplateParmDeclInternal(
+    TemplateTemplateParmDecl *CanonTTP) const {
+  llvm::FoldingSetNodeID ID;
+  CanonicalTemplateTemplateParm::Profile(ID, *this, CanonTTP);
+  void *InsertPos = nullptr;
+  if (auto *Existing =
+          CanonTemplateTemplateParms.FindNodeOrInsertPos(ID, InsertPos))
+    return Existing->getParam();
+  CanonTemplateTemplateParms.InsertNode(
+      new (*this) CanonicalTemplateTemplateParm(CanonTTP), InsertPos);
+  return CanonTTP;
+}
+
 /// Check if a type can have its sanitizer instrumentation elided based on its
 /// presence within an ignorelist.
 bool ASTContext::isTypeIgnoredBySanitizer(const SanitizerMask &Mask,
@@ -3083,12 +3108,19 @@ static auto getCanonicalTemplateArguments(const ASTContext &C,
                                           ArrayRef<TemplateArgument> Args,
                                           bool &AnyNonCanonArgs) {
   SmallVector<TemplateArgument, 16> CanonArgs(Args);
-  for (auto &Arg : CanonArgs) {
+  AnyNonCanonArgs |= C.canonicalizeTemplateArguments(CanonArgs);
+  return CanonArgs;
+}
+
+bool ASTContext::canonicalizeTemplateArguments(
+    MutableArrayRef<TemplateArgument> Args) const {
+  bool AnyNonCanonArgs = false;
+  for (auto &Arg : Args) {
     TemplateArgument OrigArg = Arg;
-    Arg = C.getCanonicalTemplateArgument(Arg);
+    Arg = getCanonicalTemplateArgument(Arg);
     AnyNonCanonArgs |= !Arg.structurallyEquals(OrigArg);
   }
-  return CanonArgs;
+  return AnyNonCanonArgs;
 }
 
 //===----------------------------------------------------------------------===//
@@ -5538,129 +5570,118 @@ QualType ASTContext::getTemplateTypeParmType(unsigned Depth, unsigned Index,
   return QualType(TypeParm, 0);
 }
 
-TypeSourceInfo *
-ASTContext::getTemplateSpecializationTypeInfo(TemplateName Name,
-                                              SourceLocation NameLoc,
-                                        const TemplateArgumentListInfo &Args,
-                                              QualType Underlying) const {
-  assert(!Name.getAsDependentTemplateName() &&
-         "No dependent template names here!");
-  QualType TST =
-      getTemplateSpecializationType(Name, Args.arguments(), Underlying);
+TypeSourceInfo *ASTContext::getTemplateSpecializationTypeInfo(
+    TemplateName Name, SourceLocation NameLoc,
+    const TemplateArgumentListInfo &SpecifiedArgs,
+    ArrayRef<TemplateArgument> CanonicalArgs, QualType Underlying) const {
+  QualType TST = getTemplateSpecializationType(Name, SpecifiedArgs.arguments(),
+                                               CanonicalArgs, Underlying);
 
   TypeSourceInfo *DI = CreateTypeSourceInfo(TST);
   TemplateSpecializationTypeLoc TL =
       DI->getTypeLoc().castAs<TemplateSpecializationTypeLoc>();
   TL.setTemplateKeywordLoc(SourceLocation());
   TL.setTemplateNameLoc(NameLoc);
-  TL.setLAngleLoc(Args.getLAngleLoc());
-  TL.setRAngleLoc(Args.getRAngleLoc());
+  TL.setLAngleLoc(SpecifiedArgs.getLAngleLoc());
+  TL.setRAngleLoc(SpecifiedArgs.getRAngleLoc());
   for (unsigned i = 0, e = TL.getNumArgs(); i != e; ++i)
-    TL.setArgLocInfo(i, Args[i].getLocInfo());
+    TL.setArgLocInfo(i, SpecifiedArgs[i].getLocInfo());
   return DI;
 }
 
-QualType
-ASTContext::getTemplateSpecializationType(TemplateName Template,
-                                          ArrayRef<TemplateArgumentLoc> Args,
-                                          QualType Underlying) const {
-  assert(!Template.getAsDependentTemplateName() &&
-         "No dependent template names here!");
+QualType ASTContext::getTemplateSpecializationType(
+    TemplateName Template, ArrayRef<TemplateArgumentLoc> SpecifiedArgs,
+    ArrayRef<TemplateArgument> CanonicalArgs, QualType Underlying) const {
+  SmallVector<TemplateArgument, 4> SpecifiedArgVec;
+  SpecifiedArgVec.reserve(SpecifiedArgs.size());
+  for (const TemplateArgumentLoc &Arg : SpecifiedArgs)
+    SpecifiedArgVec.push_back(Arg.getArgument());
 
-  SmallVector<TemplateArgument, 4> ArgVec;
-  ArgVec.reserve(Args.size());
-  for (const TemplateArgumentLoc &Arg : Args)
-    ArgVec.push_back(Arg.getArgument());
-
-  return getTemplateSpecializationType(Template, ArgVec, Underlying);
+  return getTemplateSpecializationType(Template, SpecifiedArgVec, CanonicalArgs,
+                                       Underlying);
 }
 
-#ifndef NDEBUG
-static bool hasAnyPackExpansions(ArrayRef<TemplateArgument> Args) {
+[[maybe_unused]] static bool
+hasAnyPackExpansions(ArrayRef<TemplateArgument> Args) {
   for (const TemplateArgument &Arg : Args)
     if (Arg.isPackExpansion())
       return true;
-
-  return true;
+  return false;
 }
-#endif
 
-QualType
-ASTContext::getTemplateSpecializationType(TemplateName Template,
-                                          ArrayRef<TemplateArgument> Args,
-                                          QualType Underlying) const {
-  assert(!Template.getAsDependentTemplateName() &&
-         "No dependent template names here!");
+QualType ASTContext::getCanonicalTemplateSpecializationType(
+    TemplateName Template, ArrayRef<TemplateArgument> Args) const {
+  assert(Template ==
+         getCanonicalTemplateName(Template, /*IgnoreDeduced=*/true));
+  assert(!Args.empty());
+#ifndef NDEBUG
+  for (const auto &Arg : Args)
+    assert(Arg.structurallyEquals(getCanonicalTemplateArgument(Arg)));
+#endif
 
-  const auto *TD = Template.getAsTemplateDecl(/*IgnoreDeduced=*/true);
-  bool IsTypeAlias = TD && TD->isTypeAlias();
-  QualType CanonType;
-  if (!Underlying.isNull())
-    CanonType = getCanonicalType(Underlying);
-  else {
-    // We can get here with an alias template when the specialization contains
-    // a pack expansion that does not match up with a parameter pack.
-    assert((!IsTypeAlias || hasAnyPackExpansions(Args)) &&
-           "Caller must compute aliased type");
-    IsTypeAlias = false;
-    CanonType = getCanonicalTemplateSpecializationType(Template, Args);
-  }
+  llvm::FoldingSetNodeID ID;
+  TemplateSpecializationType::Profile(ID, Template, Args, QualType(), *this);
+  void *InsertPos = nullptr;
+  if (auto *T = TemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos))
+    return QualType(T, 0);
 
-  // Allocate the (non-canonical) template specialization type, but don't
-  // try to unique it: these types typically have location information that
-  // we don't unique and don't want to lose.
   void *Mem = Allocate(sizeof(TemplateSpecializationType) +
-                           sizeof(TemplateArgument) * Args.size() +
-                           (IsTypeAlias ? sizeof(QualType) : 0),
+                           sizeof(TemplateArgument) * Args.size(),
                        alignof(TemplateSpecializationType));
-  auto *Spec
-    = new (Mem) TemplateSpecializationType(Template, Args, CanonType,
-                                         IsTypeAlias ? Underlying : QualType());
-
+  auto *Spec = new (Mem)
+      TemplateSpecializationType(Template, /*IsAlias=*/false, Args, QualType());
+  assert(Spec->isDependentType() &&
+         "canonical template specialization must be dependent");
   Types.push_back(Spec);
+  TemplateSpecializationTypes.InsertNode(Spec, InsertPos);
   return QualType(Spec, 0);
 }
 
-QualType ASTContext::getCanonicalTemplateSpecializationType(
-    TemplateName Template, ArrayRef<TemplateArgument> Args) const {
-  assert(!Template.getAsDependentTemplateName() &&
+QualType ASTContext::getTemplateSpecializationType(
+    TemplateName Template, ArrayRef<TemplateArgument> SpecifiedArgs,
+    ArrayRef<TemplateArgument> CanonicalArgs, QualType Underlying) const {
+  assert(!Template.getUnderlying().getAsDependentTemplateName() &&
          "No dependent template names here!");
 
-  // Build the canonical template specialization type.
-  // Any DeducedTemplateNames are ignored, because the effective name of a TST
-  // accounts for the TST arguments laid over any default arguments contained in
-  // its name.
-  TemplateName CanonTemplate =
-      getCanonicalTemplateName(Template, /*IgnoreDeduced=*/true);
-
-  bool Any...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Apr 10, 2025

@llvm/pr-subscribers-clang-modules

Author: Matheus Izvekov (mizvekov)

Changes

This changes the TemplateArgument representation to hold a flag indicating whether a template argument of expression type is supposed to be canonical or not.

This gets one step closer to solving #92292

This still doesn't try to unique as-written TSTs. While this would increase the amount of memory savings and make code dealing with the AST more well-behaved, profiling template argument lists is still too expensive for this to be worthwhile, at least for now. Without this uniquing, this patch stands neutral in terms of performance impact.

This also fixes the context creation of TSTs, so that they don't in some cases get incorrectly flagged as sugar over their own canonical form. This is captured in the test expectation change of some AST dumps.

This fixes some places which were unnecessarily canonicalizing these TSTs.


Patch is 90.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135119.diff

29 Files Affected:

  • (modified) clang-tools-extra/clangd/AST.cpp (+2-1)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/AST/ASTContext.h (+31-15)
  • (modified) clang/include/clang/AST/PropertiesBase.td (+4-1)
  • (modified) clang/include/clang/AST/TemplateBase.h (+11-2)
  • (modified) clang/include/clang/AST/Type.h (+3-4)
  • (modified) clang/include/clang/AST/TypeProperties.td (+5-25)
  • (modified) clang/include/clang/Serialization/ASTRecordReader.h (+4)
  • (modified) clang/lib/AST/ASTContext.cpp (+129-107)
  • (modified) clang/lib/AST/ASTDiagnostic.cpp (+4-4)
  • (modified) clang/lib/AST/ASTImporter.cpp (+21-13)
  • (modified) clang/lib/AST/DeclTemplate.cpp (+5-2)
  • (modified) clang/lib/AST/QualTypeNames.cpp (+2-1)
  • (modified) clang/lib/AST/TemplateBase.cpp (+15-4)
  • (modified) clang/lib/AST/Type.cpp (+27-23)
  • (modified) clang/lib/Sema/SemaCXXScopeSpec.cpp (+8-10)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+17-3)
  • (modified) clang/lib/Sema/SemaLookup.cpp (+2-1)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+78-84)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+27-18)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+6-5)
  • (modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+2-1)
  • (modified) clang/lib/Sema/TreeTransform.h (+9-4)
  • (modified) clang/test/CXX/class.derived/class.derived.general/p2.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.decls/temp.class.spec/p6.cpp (+2-2)
  • (modified) clang/test/SemaCXX/undefined-partial-specialization.cpp (+1-1)
  • (modified) clang/test/SemaTemplate/make_integer_seq.cpp (+16-45)
  • (modified) clang/test/SemaTemplate/type_pack_element.cpp (+15-42)
  • (modified) clang/unittests/AST/TypePrinterTest.cpp (+3-3)
diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index 66b587f00ff4a..3b991e5e9013f 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -439,7 +439,8 @@ QualType declaredType(const TypeDecl *D) {
   if (const auto *CTSD = llvm::dyn_cast<ClassTemplateSpecializationDecl>(D))
     if (const auto *Args = CTSD->getTemplateArgsAsWritten())
       return Context.getTemplateSpecializationType(
-          TemplateName(CTSD->getSpecializedTemplate()), Args->arguments());
+          TemplateName(CTSD->getSpecializedTemplate()), Args->arguments(),
+          /*CanonicalArgs=*/std::nullopt);
   return Context.getTypeDeclType(D);
 }
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index cd16641c25ed8..204d41476232c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -280,6 +280,8 @@ Improvements to Clang's diagnostics
 - Clang now better preserves the sugared types of pointers to member.
 - Clang now better preserves the presence of the template keyword with dependent
   prefixes.
+- Clang now in more cases avoids printing 'type-parameter-X-X' instead of the name of
+  the template parameter.
 - Clang now respects the current language mode when printing expressions in
   diagnostics. This fixes a bunch of `bool` being printed as `_Bool`, and also
   a bunch of HLSL types being printed as their C++ equivalents.
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 3ff9f308f3a5e..c339bb698e099 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -364,9 +364,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
                                      const ASTContext&>
     CanonTemplateTemplateParms;
 
-  TemplateTemplateParmDecl *
-    getCanonicalTemplateTemplateParmDecl(TemplateTemplateParmDecl *TTP) const;
-
   /// The typedef for the __int128_t type.
   mutable TypedefDecl *Int128Decl = nullptr;
 
@@ -1808,22 +1805,26 @@ class ASTContext : public RefCountedBase<ASTContext> {
                           bool ParameterPack,
                           TemplateTypeParmDecl *ParmDecl = nullptr) const;
 
-  QualType getTemplateSpecializationType(TemplateName T,
-                                         ArrayRef<TemplateArgument> Args,
-                                         QualType Canon = QualType()) const;
+  QualType getCanonicalTemplateSpecializationType(
+      TemplateName T, ArrayRef<TemplateArgument> CanonicalArgs) const;
 
   QualType
-  getCanonicalTemplateSpecializationType(TemplateName T,
-                                         ArrayRef<TemplateArgument> Args) const;
+  getTemplateSpecializationType(TemplateName T,
+                                ArrayRef<TemplateArgument> SpecifiedArgs,
+                                ArrayRef<TemplateArgument> CanonicalArgs,
+                                QualType Underlying = QualType()) const;
 
-  QualType getTemplateSpecializationType(TemplateName T,
-                                         ArrayRef<TemplateArgumentLoc> Args,
-                                         QualType Canon = QualType()) const;
+  QualType
+  getTemplateSpecializationType(TemplateName T,
+                                ArrayRef<TemplateArgumentLoc> SpecifiedArgs,
+                                ArrayRef<TemplateArgument> CanonicalArgs,
+                                QualType Canon = QualType()) const;
 
-  TypeSourceInfo *
-  getTemplateSpecializationTypeInfo(TemplateName T, SourceLocation TLoc,
-                                    const TemplateArgumentListInfo &Args,
-                                    QualType Canon = QualType()) const;
+  TypeSourceInfo *getTemplateSpecializationTypeInfo(
+      TemplateName T, SourceLocation TLoc,
+      const TemplateArgumentListInfo &SpecifiedArgs,
+      ArrayRef<TemplateArgument> CanonicalArgs,
+      QualType Canon = QualType()) const;
 
   QualType getParenType(QualType NamedType) const;
 
@@ -2939,6 +2940,21 @@ class ASTContext : public RefCountedBase<ASTContext> {
   TemplateArgument getCanonicalTemplateArgument(const TemplateArgument &Arg)
     const;
 
+  /// Canonicalize the given template argument list.
+  ///
+  /// Returns true if any arguments were non-canonical, false otherwise.
+  bool
+  canonicalizeTemplateArguments(MutableArrayRef<TemplateArgument> Args) const;
+
+  /// Canonicalize the given TemplateTemplateParmDecl.
+  TemplateTemplateParmDecl *
+  getCanonicalTemplateTemplateParmDecl(TemplateTemplateParmDecl *TTP) const;
+
+  TemplateTemplateParmDecl *findCanonicalTemplateTemplateParmDeclInternal(
+      TemplateTemplateParmDecl *TTP) const;
+  TemplateTemplateParmDecl *insertCanonicalTemplateTemplateParmDeclInternal(
+      TemplateTemplateParmDecl *CanonTTP) const;
+
   /// Type Query functions.  If the type is an instance of the specified class,
   /// return the Type pointer for the underlying maximally pretty type.  This
   /// is a member of ASTContext because this may need to do some amount of
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index 90537d47dd9c9..33336d57b6298 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -877,11 +877,14 @@ let Class = PropertyTypeCase<TemplateArgument, "Expression"> in {
   def : Property<"expression", ExprRef> {
     let Read = [{ node.getAsExpr() }];
   }
+  def : Property<"IsCanonical", Bool> {
+    let Read = [{ node.isCanonicalExpr() }];
+  }
   def : Property<"isDefaulted", Bool> {
     let Read = [{ node.getIsDefaulted() }];
   }
   def : Creator<[{
-    return TemplateArgument(expression, isDefaulted);
+    return TemplateArgument(expression, IsCanonical, isDefaulted);
   }]>;
 }
 let Class = PropertyTypeCase<TemplateArgument, "Pack"> in {
diff --git a/clang/include/clang/AST/TemplateBase.h b/clang/include/clang/AST/TemplateBase.h
index bea624eb04942..f833a1ec02455 100644
--- a/clang/include/clang/AST/TemplateBase.h
+++ b/clang/include/clang/AST/TemplateBase.h
@@ -168,6 +168,8 @@ class TemplateArgument {
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsDefaulted : 1;
     uintptr_t V;
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned IsCanonicalExpr : 1;
   };
   union {
     struct DA DeclArg;
@@ -187,7 +189,8 @@ class TemplateArgument {
 
 public:
   /// Construct an empty, invalid template argument.
-  constexpr TemplateArgument() : TypeOrValue({Null, 0, /* IsDefaulted */ 0}) {}
+  constexpr TemplateArgument()
+      : TypeOrValue{Null, /*IsDefaulted=*/0, /*V=*/0, /*IsCanonicalExpr=*/0} {}
 
   /// Construct a template type argument.
   TemplateArgument(QualType T, bool isNullPtr = false,
@@ -262,10 +265,11 @@ class TemplateArgument {
   /// This form of template argument only occurs in template argument
   /// lists used for dependent types and for expression; it will not
   /// occur in a non-dependent, canonical template argument list.
-  explicit TemplateArgument(Expr *E, bool IsDefaulted = false) {
+  TemplateArgument(Expr *E, bool IsCanonical, bool IsDefaulted = false) {
     TypeOrValue.Kind = Expression;
     TypeOrValue.IsDefaulted = IsDefaulted;
     TypeOrValue.V = reinterpret_cast<uintptr_t>(E);
+    TypeOrValue.IsCanonicalExpr = IsCanonical;
   }
 
   /// Construct a template argument that is a template argument pack.
@@ -407,6 +411,11 @@ class TemplateArgument {
     return reinterpret_cast<Expr *>(TypeOrValue.V);
   }
 
+  bool isCanonicalExpr() const {
+    assert(getKind() == Expression && "Unexpected kind");
+    return TypeOrValue.IsCanonicalExpr;
+  }
+
   /// Iterator that traverses the elements of a template argument pack.
   using pack_iterator = const TemplateArgument *;
 
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 9f6189440fabf..dc57170bf9160 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -6676,10 +6676,9 @@ class TemplateSpecializationType : public Type, public llvm::FoldingSetNode {
   /// replacement must, recursively, be one of these).
   TemplateName Template;
 
-  TemplateSpecializationType(TemplateName T,
+  TemplateSpecializationType(TemplateName T, bool IsAlias,
                              ArrayRef<TemplateArgument> Args,
-                             QualType Canon,
-                             QualType Aliased);
+                             QualType Underlying);
 
 public:
   /// Determine whether any of the given template arguments are dependent.
@@ -6747,7 +6746,7 @@ class TemplateSpecializationType : public Type, public llvm::FoldingSetNode {
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Ctx);
   static void Profile(llvm::FoldingSetNodeID &ID, TemplateName T,
-                      ArrayRef<TemplateArgument> Args,
+                      ArrayRef<TemplateArgument> Args, QualType Underlying,
                       const ASTContext &Context);
 
   static bool classof(const Type *T) {
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 66d490850678a..3bf9239e9cbf5 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -737,39 +737,19 @@ let Class = DependentAddressSpaceType in {
 }
 
 let Class = TemplateSpecializationType in {
-  def : Property<"dependent", Bool> {
-    let Read = [{ node->isDependentType() }];
-  }
   def : Property<"templateName", TemplateName> {
     let Read = [{ node->getTemplateName() }];
   }
-  def : Property<"templateArguments", Array<TemplateArgument>> {
+  def : Property<"args", Array<TemplateArgument>> {
     let Read = [{ node->template_arguments() }];
   }
-  def : Property<"underlyingType", Optional<QualType>> {
-    let Read = [{
-      node->isTypeAlias()
-        ? std::optional<QualType>(node->getAliasedType())
-        : node->isCanonicalUnqualified()
-            ? std::nullopt
-            : std::optional<QualType>(node->getCanonicalTypeInternal())
-    }];
+  def : Property<"UnderlyingType", QualType> {
+    let Read = [{ node->isCanonicalUnqualified() ? QualType() :
+                                                   node->desugar() }];
   }
 
   def : Creator<[{
-    QualType result;
-    if (!underlyingType) {
-      result = ctx.getCanonicalTemplateSpecializationType(templateName,
-                                                          templateArguments);
-    } else {
-      result = ctx.getTemplateSpecializationType(templateName,
-                                                 templateArguments,
-                                                 *underlyingType);
-    }
-    if (dependent)
-      const_cast<Type *>(result.getTypePtr())
-          ->addDependence(TypeDependence::DependentInstantiation);
-    return result;
+    return ctx.getTemplateSpecializationType(templateName, args, std::nullopt, UnderlyingType);
   }]>;
 }
 
diff --git a/clang/include/clang/Serialization/ASTRecordReader.h b/clang/include/clang/Serialization/ASTRecordReader.h
index 141804185083f..90993687e69a0 100644
--- a/clang/include/clang/Serialization/ASTRecordReader.h
+++ b/clang/include/clang/Serialization/ASTRecordReader.h
@@ -247,6 +247,10 @@ class ASTRecordReader
   void readTemplateArgumentList(SmallVectorImpl<TemplateArgument> &TemplArgs,
                                 bool Canonicalize = false);
 
+  /// Read a template argument list.
+  const TemplateArgumentList *
+  readTemplateArgumentList(bool Canonicalize = false);
+
   /// Read a UnresolvedSet structure, advancing Idx.
   void readUnresolvedSet(LazyASTUnresolvedSet &Set);
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 0fe941e063d49..6ad49c27b34d1 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -844,6 +844,31 @@ ASTContext::getCanonicalTemplateTemplateParmDecl(
   return CanonTTP;
 }
 
+TemplateTemplateParmDecl *
+ASTContext::findCanonicalTemplateTemplateParmDeclInternal(
+    TemplateTemplateParmDecl *TTP) const {
+  llvm::FoldingSetNodeID ID;
+  CanonicalTemplateTemplateParm::Profile(ID, *this, TTP);
+  void *InsertPos = nullptr;
+  CanonicalTemplateTemplateParm *Canonical =
+      CanonTemplateTemplateParms.FindNodeOrInsertPos(ID, InsertPos);
+  return Canonical ? Canonical->getParam() : nullptr;
+}
+
+TemplateTemplateParmDecl *
+ASTContext::insertCanonicalTemplateTemplateParmDeclInternal(
+    TemplateTemplateParmDecl *CanonTTP) const {
+  llvm::FoldingSetNodeID ID;
+  CanonicalTemplateTemplateParm::Profile(ID, *this, CanonTTP);
+  void *InsertPos = nullptr;
+  if (auto *Existing =
+          CanonTemplateTemplateParms.FindNodeOrInsertPos(ID, InsertPos))
+    return Existing->getParam();
+  CanonTemplateTemplateParms.InsertNode(
+      new (*this) CanonicalTemplateTemplateParm(CanonTTP), InsertPos);
+  return CanonTTP;
+}
+
 /// Check if a type can have its sanitizer instrumentation elided based on its
 /// presence within an ignorelist.
 bool ASTContext::isTypeIgnoredBySanitizer(const SanitizerMask &Mask,
@@ -3083,12 +3108,19 @@ static auto getCanonicalTemplateArguments(const ASTContext &C,
                                           ArrayRef<TemplateArgument> Args,
                                           bool &AnyNonCanonArgs) {
   SmallVector<TemplateArgument, 16> CanonArgs(Args);
-  for (auto &Arg : CanonArgs) {
+  AnyNonCanonArgs |= C.canonicalizeTemplateArguments(CanonArgs);
+  return CanonArgs;
+}
+
+bool ASTContext::canonicalizeTemplateArguments(
+    MutableArrayRef<TemplateArgument> Args) const {
+  bool AnyNonCanonArgs = false;
+  for (auto &Arg : Args) {
     TemplateArgument OrigArg = Arg;
-    Arg = C.getCanonicalTemplateArgument(Arg);
+    Arg = getCanonicalTemplateArgument(Arg);
     AnyNonCanonArgs |= !Arg.structurallyEquals(OrigArg);
   }
-  return CanonArgs;
+  return AnyNonCanonArgs;
 }
 
 //===----------------------------------------------------------------------===//
@@ -5538,129 +5570,118 @@ QualType ASTContext::getTemplateTypeParmType(unsigned Depth, unsigned Index,
   return QualType(TypeParm, 0);
 }
 
-TypeSourceInfo *
-ASTContext::getTemplateSpecializationTypeInfo(TemplateName Name,
-                                              SourceLocation NameLoc,
-                                        const TemplateArgumentListInfo &Args,
-                                              QualType Underlying) const {
-  assert(!Name.getAsDependentTemplateName() &&
-         "No dependent template names here!");
-  QualType TST =
-      getTemplateSpecializationType(Name, Args.arguments(), Underlying);
+TypeSourceInfo *ASTContext::getTemplateSpecializationTypeInfo(
+    TemplateName Name, SourceLocation NameLoc,
+    const TemplateArgumentListInfo &SpecifiedArgs,
+    ArrayRef<TemplateArgument> CanonicalArgs, QualType Underlying) const {
+  QualType TST = getTemplateSpecializationType(Name, SpecifiedArgs.arguments(),
+                                               CanonicalArgs, Underlying);
 
   TypeSourceInfo *DI = CreateTypeSourceInfo(TST);
   TemplateSpecializationTypeLoc TL =
       DI->getTypeLoc().castAs<TemplateSpecializationTypeLoc>();
   TL.setTemplateKeywordLoc(SourceLocation());
   TL.setTemplateNameLoc(NameLoc);
-  TL.setLAngleLoc(Args.getLAngleLoc());
-  TL.setRAngleLoc(Args.getRAngleLoc());
+  TL.setLAngleLoc(SpecifiedArgs.getLAngleLoc());
+  TL.setRAngleLoc(SpecifiedArgs.getRAngleLoc());
   for (unsigned i = 0, e = TL.getNumArgs(); i != e; ++i)
-    TL.setArgLocInfo(i, Args[i].getLocInfo());
+    TL.setArgLocInfo(i, SpecifiedArgs[i].getLocInfo());
   return DI;
 }
 
-QualType
-ASTContext::getTemplateSpecializationType(TemplateName Template,
-                                          ArrayRef<TemplateArgumentLoc> Args,
-                                          QualType Underlying) const {
-  assert(!Template.getAsDependentTemplateName() &&
-         "No dependent template names here!");
+QualType ASTContext::getTemplateSpecializationType(
+    TemplateName Template, ArrayRef<TemplateArgumentLoc> SpecifiedArgs,
+    ArrayRef<TemplateArgument> CanonicalArgs, QualType Underlying) const {
+  SmallVector<TemplateArgument, 4> SpecifiedArgVec;
+  SpecifiedArgVec.reserve(SpecifiedArgs.size());
+  for (const TemplateArgumentLoc &Arg : SpecifiedArgs)
+    SpecifiedArgVec.push_back(Arg.getArgument());
 
-  SmallVector<TemplateArgument, 4> ArgVec;
-  ArgVec.reserve(Args.size());
-  for (const TemplateArgumentLoc &Arg : Args)
-    ArgVec.push_back(Arg.getArgument());
-
-  return getTemplateSpecializationType(Template, ArgVec, Underlying);
+  return getTemplateSpecializationType(Template, SpecifiedArgVec, CanonicalArgs,
+                                       Underlying);
 }
 
-#ifndef NDEBUG
-static bool hasAnyPackExpansions(ArrayRef<TemplateArgument> Args) {
+[[maybe_unused]] static bool
+hasAnyPackExpansions(ArrayRef<TemplateArgument> Args) {
   for (const TemplateArgument &Arg : Args)
     if (Arg.isPackExpansion())
       return true;
-
-  return true;
+  return false;
 }
-#endif
 
-QualType
-ASTContext::getTemplateSpecializationType(TemplateName Template,
-                                          ArrayRef<TemplateArgument> Args,
-                                          QualType Underlying) const {
-  assert(!Template.getAsDependentTemplateName() &&
-         "No dependent template names here!");
+QualType ASTContext::getCanonicalTemplateSpecializationType(
+    TemplateName Template, ArrayRef<TemplateArgument> Args) const {
+  assert(Template ==
+         getCanonicalTemplateName(Template, /*IgnoreDeduced=*/true));
+  assert(!Args.empty());
+#ifndef NDEBUG
+  for (const auto &Arg : Args)
+    assert(Arg.structurallyEquals(getCanonicalTemplateArgument(Arg)));
+#endif
 
-  const auto *TD = Template.getAsTemplateDecl(/*IgnoreDeduced=*/true);
-  bool IsTypeAlias = TD && TD->isTypeAlias();
-  QualType CanonType;
-  if (!Underlying.isNull())
-    CanonType = getCanonicalType(Underlying);
-  else {
-    // We can get here with an alias template when the specialization contains
-    // a pack expansion that does not match up with a parameter pack.
-    assert((!IsTypeAlias || hasAnyPackExpansions(Args)) &&
-           "Caller must compute aliased type");
-    IsTypeAlias = false;
-    CanonType = getCanonicalTemplateSpecializationType(Template, Args);
-  }
+  llvm::FoldingSetNodeID ID;
+  TemplateSpecializationType::Profile(ID, Template, Args, QualType(), *this);
+  void *InsertPos = nullptr;
+  if (auto *T = TemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos))
+    return QualType(T, 0);
 
-  // Allocate the (non-canonical) template specialization type, but don't
-  // try to unique it: these types typically have location information that
-  // we don't unique and don't want to lose.
   void *Mem = Allocate(sizeof(TemplateSpecializationType) +
-                           sizeof(TemplateArgument) * Args.size() +
-                           (IsTypeAlias ? sizeof(QualType) : 0),
+                           sizeof(TemplateArgument) * Args.size(),
                        alignof(TemplateSpecializationType));
-  auto *Spec
-    = new (Mem) TemplateSpecializationType(Template, Args, CanonType,
-                                         IsTypeAlias ? Underlying : QualType());
-
+  auto *Spec = new (Mem)
+      TemplateSpecializationType(Template, /*IsAlias=*/false, Args, QualType());
+  assert(Spec->isDependentType() &&
+         "canonical template specialization must be dependent");
   Types.push_back(Spec);
+  TemplateSpecializationTypes.InsertNode(Spec, InsertPos);
   return QualType(Spec, 0);
 }
 
-QualType ASTContext::getCanonicalTemplateSpecializationType(
-    TemplateName Template, ArrayRef<TemplateArgument> Args) const {
-  assert(!Template.getAsDependentTemplateName() &&
+QualType ASTContext::getTemplateSpecializationType(
+    TemplateName Template, ArrayRef<TemplateArgument> SpecifiedArgs,
+    ArrayRef<TemplateArgument> CanonicalArgs, QualType Underlying) const {
+  assert(!Template.getUnderlying().getAsDependentTemplateName() &&
          "No dependent template names here!");
 
-  // Build the canonical template specialization type.
-  // Any DeducedTemplateNames are ignored, because the effective name of a TST
-  // accounts for the TST arguments laid over any default arguments contained in
-  // its name.
-  TemplateName CanonTemplate =
-      getCanonicalTemplateName(Template, /*IgnoreDeduced=*/true);
-
-  bool Any...
[truncated]

@mizvekov mizvekov force-pushed the users/mizvekov/tst-refactor branch from 27b8454 to 03801ae Compare April 10, 2025 02:25
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.

1 nit, 1 Q.

This changes the TemplateArgument representation to hold a flag indicating
whether a tempalte argument of expression type is supposed to be canonical
or not.

This gets one step closer to solving #92292

This still doesn't try to unique as-written TSTs. While this would
increase the amount of memory savings and make code dealing with
the AST more well-behaved, profiling template argument lists is
still too expensive for this to be worthwhile, at least for now.

This also fixes the context creation of TSTs, so that they don't
in some cases get incorrectly flagged as sugar over their own canonical
form. This is captured in the test expectation change of some AST dumps.

This fixes some places which were unnecessarily canonicalizing these TSTs.
@mizvekov mizvekov force-pushed the users/mizvekov/tst-refactor branch from 03801ae to ee7360d Compare April 10, 2025 16:14
@mizvekov mizvekov merged commit 3954d25 into main Apr 10, 2025
10 of 11 checks passed
@mizvekov mizvekov deleted the users/mizvekov/tst-refactor branch April 10, 2025 17:23
@slydiman
Copy link
Contributor

slydiman commented Apr 10, 2025

Please note we got the assert (ASTContext.cpp, line 5619) at least on the following buildbots:

lldb-remote-linux-win:

UNSUPPORTED: LLDB (C:\buildbot\as-builder-10\lldb-x-aarch64\build\bin\clang.exe-aarch64) :: test_dsym (TestIteratorFromStdModule.TestCase.test_dsym)
(test case does not fall in any category of interest for this run) 
Assertion failed: Arg.structurallyEquals(getCanonicalTemplateArgument(Arg)), 
file C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\clang\lib\AST\ASTContext.cpp, line 5619
Windows fatal exception: code 0x80000003
Current thread 0x0000287c (most recent call first):
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\build\Lib\site-packages\lldb\__init__.py", line 6539 in EvaluateExpression
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 2547 in expect_expr
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\test\API\commands\expression\import-std-module\iterator\TestIteratorFromStdModule.py", line 24 in test
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\packages\Python\lldbsuite\test\decorators.py", line 148 in wrapper
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 1804 in test_method
  File "C:\Python312\Lib\unittest\case.py", line 589 in _callTestMethod
  File "C:\Python312\Lib\unittest\case.py", line 634 in run
  File "C:\Python312\Lib\unittest\case.py", line 690 in __call__
  File "C:\Python312\Lib\unittest\suite.py", line 122 in run
  File "C:\Python312\Lib\unittest\suite.py", line 84 in __call__
  File "C:\Python312\Lib\unittest\suite.py", line 122 in run
  File "C:\Python312\Lib\unittest\suite.py", line 84 in __call__
  File "C:\Python312\Lib\unittest\runner.py", line 240 in run
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\packages\Python\lldbsuite\test\dotest.py", line 1108 in run_suite
  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\test\API\dotest.py", line 8 in <module>
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Exception Code: 0x80000003
  #0 0x00007ffade6ec4b5 PyInit__lldb (C:\buildbot\as-builder-10\lldb-x-aarch64\build\Lib\site-packages\lldb\_lldb.cp312-win_amd64.pyd+0x9fc4b5)
  #1 0x00007ffb08a0bb04 (C:\Windows\System32\ucrtbase.dll+0x7bb04)
  #2 0x00007ffb08a0cad1 (C:\Windows\System32\ucrtbase.dll+0x7cad1)
  #3 0x00007ffb08a0e4a1 (C:\Windows\System32\ucrtbase.dll+0x7e4a1)
  #4 0x00007ffb08a0e6e1 (C:\Windows\System32\ucrtbase.dll+0x7e6e1)

lldb-remote-linux-ubuntu:

python3.12: /home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/clang/lib/AST/ASTContext.cpp:5619:
clang::QualType clang::ASTContext::getCanonicalTemplateSpecializationType(clang::TemplateName, llvm::ArrayRef<clang::TemplateArgument>) const:
Assertion `Arg.structurallyEquals(getCanonicalTemplateArgument(Arg))' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  _lldb.cpython-312-x86_64-linux-gnu.so 0x00007b63c144ae62
1  _lldb.cpython-312-x86_64-linux-gnu.so 0x00007b63c14480cf
2  _lldb.cpython-312-x86_64-linux-gnu.so 0x00007b63c1448214
3  libc.so.6                             0x00007b63c8445330
4  libc.so.6                             0x00007b63c849eb2c pthread_kill + 284
5  libc.so.6                             0x00007b63c844527e gsignal + 30
6  libc.so.6                             0x00007b63c84288ff abort + 223
7  libc.so.6                             0x00007b63c842881b
8  libc.so.6                             0x00007b63c843b517

slydiman added a commit that referenced this pull request Apr 11, 2025
… types" (#135354)

Reverts #135119 because of the assert in ASTContext.cpp, line 5619.
See #135352 for details.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 11, 2025
…cialization types" (#135354)

Reverts llvm/llvm-project#135119 because of the assert in ASTContext.cpp, line 5619.
See #135352 for details.
mizvekov added a commit that referenced this pull request Apr 12, 2025
… types (#135414)

This relands #135119, after
fixing crashes seen in LLDB CI reported here:
#135119 (comment)

Fixes #135119

This changes the TemplateArgument representation to hold a flag
indicating whether a tempalte argument of expression type is supposed to
be canonical or not.

This gets one step closer to solving
#92292

This still doesn't try to unique as-written TSTs. While this would
increase the amount of memory savings and make code dealing with the AST
more well-behaved, profiling template argument lists is still too
expensive for this to be worthwhile, at least for now.

This also fixes the context creation of TSTs, so that they don't in some
cases get incorrectly flagged as sugar over their own canonical form.
This is captured in the test expectation change of some AST dumps.

This fixes some places which were unnecessarily canonicalizing these
TSTs.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 12, 2025
…cialization types (#135414)

This relands llvm/llvm-project#135119, after
fixing crashes seen in LLDB CI reported here:
llvm/llvm-project#135119 (comment)

Fixes llvm/llvm-project#135119

This changes the TemplateArgument representation to hold a flag
indicating whether a tempalte argument of expression type is supposed to
be canonical or not.

This gets one step closer to solving
llvm/llvm-project#92292

This still doesn't try to unique as-written TSTs. While this would
increase the amount of memory savings and make code dealing with the AST
more well-behaved, profiling template argument lists is still too
expensive for this to be worthwhile, at least for now.

This also fixes the context creation of TSTs, so that they don't in some
cases get incorrectly flagged as sugar over their own canonical form.
This is captured in the test expectation change of some AST dumps.

This fixes some places which were unnecessarily canonicalizing these
TSTs.
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:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clang-tools-extra clangd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants