[clang] fix profiling of template arguments of template and declaration kind

[mizvekov]

Summary

Template arguments of template and declaration kind were being profiled
only by their canonical properties, which would cause incorrect
uniquing of constrained AutoTypes, leading to a crash in some cases.

This exposed some places in CheckTemplateArgumentList where non-canonical
arguments where being pushed into the resulting converted list.

We also throw in some asserts to catch early and explain the crashes.

Note that the fix for the 'declaration' kind is untestable at this point,
because there should be no cases right now in the AST where we try
to unique a non-canonical converted template argument.

This fixes GH55567.

Depends on D133082

Signed-off-by: Matheus Izvekov <[e-mail]>

[erichkeane]

Generally happy here. Two quick suggestions, otherewise LGTM.

clang/lib/AST/ASTContext.cpp

5119–5121

Instead of maybe_unused put this and the assert inside of a #if NDEBUG so we don't do the Nothing work. Same below.

clang/lib/Sema/SemaTemplate.cpp

5817–5818

Ooh, thats a transform!

llvm::transform(ArgumentPack, std::back_inserter(Converted), [](const auto &I) { return Context.getCanonicalTemplateArgument(I));

[mizvekov]

clang/lib/AST/ASTContext.cpp

5119–5121

We still need to do the FindNodeOrInsertPos bit, to refresh the insert position.
So I think this would end up too noisy to add two sets of blocks.

clang/lib/Sema/SemaTemplate.cpp

5817–5818

That is true, though unless we can come up with a clearer spelling for that, it looks less readable to me.

[erichkeane]

clang/lib/AST/ASTContext.cpp

5119–5121

Ah! I didn't realize we were counting on the side-effects here. Thanks!

clang/lib/Sema/SemaTemplate.cpp

5817–5818

But I like transform :(

[mizvekov]

clang/lib/Sema/SemaTemplate.cpp

5817–5818

I will think of something, even if we have to add a new helper :)

[mizvekov]

clang/lib/Sema/SemaTemplate.cpp

5817–5818

I'll try to address this concern, about having a helper which returns a transformed copy of a container, in a way that reads better than the pure transform as suggested, in a follow up patch. Meanwhile I'll offload this patch, as my stack is too big right now.

clang/lib/AST/ASTContext.cpp

ASTContext::getDependentTemplateSpecializationType(
 
   QualType Canon;
   if (AnyNonCanonArgs || CanonNNS != NNS || CanonKeyword != Keyword) {
     Canon = getDependentTemplateSpecializationType(CanonKeyword, CanonNNS,
                                                    Name,
                                                    CanonArgs);
 
     // Find the insert position again.
+    [[maybe_unused]] auto *Nothing =
         DependentTemplateSpecializationTypes.FindNodeOrInsertPos(ID, InsertPos);
+    assert(!Nothing && "canonical type broken");
   }
 
   void *Mem = Allocate((sizeof(DependentTemplateSpecializationType) +
                         sizeof(TemplateArgument) * Args.size()),
                        TypeAlignment);
   T = new (Mem) DependentTemplateSpecializationType(Keyword, NNS,
                                                     Name, Args, Canon);
   Types.push_back(T);
if (!IsCanon) {
     if (DeducedType.isNull()) {
       SmallVector<TemplateArgument, 4> CanonArgs;
       bool AnyNonCanonArgs =
           ::getCanonicalTemplateArguments(*this, TypeConstraintArgs, CanonArgs);
       if (AnyNonCanonArgs) {
         Canon = getAutoTypeInternal(QualType(), Keyword, IsDependent, IsPack,
                                     TypeConstraintConcept, CanonArgs, true);
         // Find the insert position again.
+        [[maybe_unused]] auto *Nothing =
             AutoTypes.FindNodeOrInsertPos(ID, InsertPos);
+        assert(!Nothing && "canonical type broken");
       }
     } else {
       Canon = DeducedType.getCanonicalType();
     }
   }
 
   void *Mem = Allocate(sizeof(AutoType) +
                            sizeof(TemplateArgument) * TypeConstraintArgs.size(),

clang/lib/AST/TemplateBase.cpp

case Type:
     break;
 
   case NullPtr:
     getNullPtrType().Profile(ID);
     break;
 
   case Declaration:
     getParamTypeForDecl().Profile(ID);
-    ID.AddPointer(getAsDecl()? getAsDecl()->getCanonicalDecl() : nullptr);
+    ID.AddPointer(getAsDecl());
     break;
 
+  case TemplateExpansion:
+    ID.AddInteger(TemplateArg.NumExpansions);
+    LLVM_FALLTHROUGH;
   case Template:
-  case TemplateExpansion: {
-    TemplateName Template = getAsTemplateOrTemplatePattern();
-    if (TemplateTemplateParmDecl *TTP
-          = dyn_cast_or_null<TemplateTemplateParmDecl>(
-                                                Template.getAsTemplateDecl())) {
-      ID.AddBoolean(true);
-      ID.AddInteger(TTP->getDepth());
-      ID.AddInteger(TTP->getPosition());
-      ID.AddBoolean(TTP->isParameterPack());
-    } else {
-      ID.AddBoolean(false);
-      ID.AddPointer(Context.getCanonicalTemplateName(Template)
-                                                          .getAsVoidPointer());
-    }
+    getAsTemplateOrTemplatePattern().Profile(ID);
     break;
-  }
 
   case Integral:
     getAsIntegral().Profile(ID);
     getIntegralType().Profile(ID);
     break;
 
   case Expression:
     getAsExpr()->Profile(ID, Context, true);

clang/lib/Sema/SemaTemplate.cpp

bool Sema::CheckTemplateArgument(NamedDecl *Param,
   case TemplateArgument::Null:
     llvm_unreachable("Should never see a NULL template argument here");
 
   case TemplateArgument::Template:
   case TemplateArgument::TemplateExpansion:
     if (CheckTemplateTemplateArgument(TempParm, Params, Arg))
       return true;
 
-    Converted.push_back(Arg.getArgument());
+    Converted.push_back(
+        Context.getCanonicalTemplateArgument(Arg.getArgument()));
     break;
 
   case TemplateArgument::Expression:
   case TemplateArgument::Type:
     // We have a template template parameter but the template
     // argument does not refer to a template.
     Diag(Arg.getLocation(), diag::err_template_arg_must_be_template)
       << getLangOpts().CPlusPlus11;
if (ArgIdx < NumArgs) {
 
       // If we just saw a pack expansion into a non-pack, then directly convert
       // the remaining arguments, because we don't know what parameters they'll
       // match up with.
       if (PackExpansionIntoNonPack) {
         if (!ArgumentPack.empty()) {
           // If we were part way through filling in an expanded parameter pack,
           // fall back to just producing individual arguments.
-          Converted.insert(Converted.end(),
-                           ArgumentPack.begin(), ArgumentPack.end());
+          for (const TemplateArgument &I : ArgumentPack)
+            Converted.push_back(Context.getCanonicalTemplateArgument(I));
           ArgumentPack.clear();
         }
 
         while (ArgIdx < NumArgs) {
-          Converted.push_back(NewArgs[ArgIdx].getArgument());
+          Converted.push_back(Context.getCanonicalTemplateArgument(
+              NewArgs[ArgIdx].getArgument()));
           ++ArgIdx;
         }
 
         return false;
       }
 
       continue;
     }
bool Sema::CheckTemplateArgumentList(
 
   // If we're performing a partial argument substitution, allow any trailing
   // pack expansions; they might be empty. This can happen even if
   // PartialTemplateArgs is false (the list of arguments is complete but
   // still dependent).
   if (ArgIdx < NumArgs && CurrentInstantiationScope &&
       CurrentInstantiationScope->getPartiallySubstitutedPack()) {
     while (ArgIdx < NumArgs && NewArgs[ArgIdx].getArgument().isPackExpansion())
-      Converted.push_back(NewArgs[ArgIdx++].getArgument());
+      Converted.push_back(Context.getCanonicalTemplateArgument(
+          NewArgs[ArgIdx++].getArgument()));
   }
 
   // If we have any leftover arguments, then there were too many arguments.
   // Complain and fail.
   if (ArgIdx < NumArgs) {
     Diag(TemplateLoc, diag::err_template_arg_list_different_arity)
         << /*too many args*/1
         << (int)getTemplateNameKindForDiagnostics(TemplateName(Template))

clang/test/SemaTemplate/concepts.cpp

 concept C = false; // expected-note 3 {{because 'false' evaluated to false}}
 
 int **const &g();
 
 C auto **j1 = g();   // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace GH55567 {
+template<class, template <class> class> concept C = true;
+template <class> struct S {};
+void f(C<GH55567::S> auto);
+} // namespace GH55567