GH62362: Handle constraints on "Using" inherited constructors

[erichkeane]

Summary

Two similar bugs were filed, both captured in GH62362. The problem
happens when collecting the instantiation args for a constructor with a
constraint is inherited by a child class.

When the inheriting class itself was NOT a template, the implicit
constructor that modeled the inherited constructor is in the declaration
context of the inherited type, however it is marked as implicit and as
an inheriting constructor. In order to make sure we get the correct
instantiation args, this patch recognizes this pattern, and causes us to
pick up the instantiation args from the inherited constructor. This
also corrects the 'declaration context' to be the
ClassTemplateSpecializationDecl of the inherited class, which has the
correct instantiation args.

however, if the inheriting class is already a template, the correct
constructor is actually already picked up, in the correct specialization
of the parent class. However, in the case of an explicit
specialization, we were skipping the template instantiation args. I
found that removing this early-return here actually broke no lit tests,
however I still limited the removal of this early return to concept
checks.

Fixes: #62362

[erichkeane]

This actually doesn't fix the original reproducer of #62344, or at least, it breaks it in a different way. Working to reduce that I guess!

[erichkeane]

Creduced the new crash down to:

template <bool> struct a;
template <> struct a<false> {
  template <typename b, typename c> constexpr static c e(b, c) {
    typename b ::f d;
  }
};
int h;
long g = a<false>::e(g, h);

I'll end up having to look at it tomorrow, but it isn't clear how this patch broke that.

[erichkeane]

In D149264#4299877, @erichkeane wrote:

Creduced the new crash down to:

template <bool> struct a;
template <> struct a<false> {
  template <typename b, typename c> constexpr static c e(b, c) {
    typename b ::f d;
  }
};
int h;
long g = a<false>::e(g, h);

I'll end up having to look at it tomorrow, but it isn't clear how this patch broke that.

I was mistaken! This actually does not fail, that must have been a previous version of the patch I was working on that didn't work right, but got caught in my terminal anyway. Looks like this is ready for review!

[rsmith]

I think that the bugs that this was aiming to fix were fixed by rG1e43349e321694d7fee3d77cb691887ad67fb5d7, which means that we should not ask whether the constraints of an inherited constructor are satisfied any more. Instead, overload resolution will look at whether the constraints of the original constructors are satisfied. (More broadly, I wonder whether DiagnoseUseOfDecl should be considering constraints at all -- the model in the standard is that any time you name a function, you go through overload resolution, which considers constraints, so DiagnoseUseOfDecl should only ever be duplicating work that's already been done by overload resolution, but it's possible we're not entirely following that model. Still, we should be able to avoid redoing the constraint satisfaction checks in the common case where overload resolution already did them.)

That said, I think this change would still be correct if for whatever reason we started reaching this codepath for inheriting constructors again.

clang/lib/Sema/SemaTemplateInstantiate.cpp

138–144

It seems to me that a namespace-scope explicit specialization shouldn't pick up template arguments, even during constraint checking:

template<typename T> struct A {};
template<> struct A<int> {
  template<typename U> void f() requires U::value;
};

Constraint checking for A<X>::f<Y> should produce a single level of template arguments, <Y>, not two layers <X>, <Y>, because U in U::value is a depth-0 index-0 parameter.

217–219

These checks can be simplified.

[rsmith]

clang/lib/Sema/SemaTemplateInstantiate.cpp

138–144

Complete testcase:

struct X {}; struct Y { static constexpr bool value = true; };
template<typename T> struct A {};
template<> struct A<X> {                          
  template<typename U> void f() requires U::value;
};                                                                   
void g(A<X> a) { a.f<Y>(); }

[erichkeane]

Confirmed this is all fixed by your patch. Thanks!

clang/lib/Sema/SemaTemplateInstantiate.cpp

138–144

Ah! Thanks for the reproducer. I was having a really hard time determining a case where this would break, everything I came up with was blocked by https://reviews.llvm.org/D146178

The difficulty was that the parent of the inherited constructor was an inline definition of the explicit specialization, so it DID have depth, but it wasn't clear how to differentiate the two. I think your patch as listed fixes all the issues I was trying to fix as you said, so I'll abandon this, though I'll include the tests as an NFC/RAC patch for completeness.

217–219

Ah, thanks! I must have been looking at the wrong thing, I wasn't convinced that the getInheritedConstructor call was always legal, but looking again, I see that should have been obvious.

[erichkeane]

Fixed by Richard! Tests and release note added in f539bffc

clang/lib/Sema/SemaTemplateInstantiate.cpp

for (unsigned I = 0, N = TTP->getDepth() + 1; I != N; ++I)
     Result.addOuterTemplateArguments(std::nullopt);
   return Response::Done();
 }
 
 // Add template arguments from a class template instantiation.
 Response
 HandleClassTemplateSpec(const ClassTemplateSpecializationDecl *ClassTemplSpec,
                         MultiLevelTemplateArgumentList &Result,
-                        bool SkipForSpecialization) {
+                        bool SkipForSpecialization,
+                        bool ForConstraintInstantiation) {
   if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {
     // We're done when we hit an explicit specialization.
     if (ClassTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
-        !isa<ClassTemplatePartialSpecializationDecl>(ClassTemplSpec))
+        !isa<ClassTemplatePartialSpecializationDecl>(ClassTemplSpec) &&
+        !ForConstraintInstantiation)
       return Response::Done();
 
     if (!SkipForSpecialization)
       Result.addOuterTemplateArguments(
           const_cast<ClassTemplateSpecializationDecl *>(ClassTemplSpec),
           ClassTemplSpec->getTemplateInstantiationArgs().asArray(),
           /*Final=*/false);
 
Response HandleFunction(const FunctionDecl *Function,
   if ((Function->getFriendObjectKind() || Function->isLocalExternDecl()) &&
       Function->getNonTransparentDeclContext()->isFileContext() &&
       (!Pattern || !Pattern->getLexicalDeclContext()->isFileContext())) {
     return Response::ChangeDecl(Function->getLexicalDeclContext());
   }
   return Response::UseNextDecl(Function);
 }
 
+Response HandleConstructor(const CXXConstructorDecl *Ctor,
+                           MultiLevelTemplateArgumentList &Result,
+                           const FunctionDecl *Pattern, bool RelativeToPrimary,
+                           bool ForConstraintInstantiation) {
+  if (Ctor->isImplicit() && Ctor->isInheritingConstructor() &&
+      Ctor->getInheritedConstructor().getConstructor())
+    Ctor = Ctor->getInheritedConstructor().getConstructor();
+
+  return HandleFunction(Ctor, Result, Pattern, RelativeToPrimary,
+                        ForConstraintInstantiation);
+}
+
 Response HandleRecordDecl(const CXXRecordDecl *Rec,
                           MultiLevelTemplateArgumentList &Result,
                           ASTContext &Context,
                           bool ForConstraintInstantiation) {
   if (ClassTemplateDecl *ClassTemplate = Rec->getDescribedClassTemplate()) {
     assert(
         (ForConstraintInstantiation || Result.getNumSubstitutedLevels() == 0) &&
         "Outer template not instantiated?");
MultiLevelTemplateArgumentList Sema::getTemplateInstantiationArgs(
   while (!CurDecl->isFileContextDecl()) {
     using namespace TemplateInstArgsHelpers;
     Response R;
     if (const auto *VarTemplSpec =
             dyn_cast<VarTemplateSpecializationDecl>(CurDecl)) {
       R = HandleVarTemplateSpec(VarTemplSpec, Result, SkipForSpecialization);
     } else if (const auto *ClassTemplSpec =
                    dyn_cast<ClassTemplateSpecializationDecl>(CurDecl)) {
-      R = HandleClassTemplateSpec(ClassTemplSpec, Result,
-                                  SkipForSpecialization);
+      R = HandleClassTemplateSpec(ClassTemplSpec, Result, SkipForSpecialization,
+                                  ForConstraintInstantiation);
+    } else if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(CurDecl)) {
+      R = HandleConstructor(Ctor, Result, Pattern, RelativeToPrimary,
+                            ForConstraintInstantiation);
     } else if (const auto *Function = dyn_cast<FunctionDecl>(CurDecl)) {
       R = HandleFunction(Function, Result, Pattern, RelativeToPrimary,
                          ForConstraintInstantiation);
     } else if (const auto *Rec = dyn_cast<CXXRecordDecl>(CurDecl)) {
       R = HandleRecordDecl(Rec, Result, Context, ForConstraintInstantiation);
     } else if (const auto *CSD =
                    dyn_cast<ImplicitConceptSpecializationDecl>(CurDecl)) {
       R = HandleImplicitConceptSpecializationDecl(CSD, Result);

clang/test/SemaTemplate/concepts.cpp

 
 static_assert(Parent<int>::TakesBinary<int, 0>::i == 0);
 // expected-error@+3{{constraints not satisfied for class template 'TakesBinary'}}
 // expected-note@#BINARY{{because 'C2<decltype(0ULL), int>' evaluated to false}}
 // expected-note@#61777_C2{{because 'sizeof(unsigned long long) == sizeof(int)' (8 == 4) evaluated to false}}
 static_assert(Parent<int>::TakesBinary<int, 0ULL>::i == 0);
 }
 
+namespace GH62362 {
+template<typename T>
+concept C = true;
+
+template <typename T> struct Test {
+    Test()
+          requires(C<T>);
+};
+
+struct Bar : public Test<int> {
+      using Test<int>::Test;
+};
+
+template <>
+struct Test<void> : public Test<int> {
+    using Test<int>::Test;
+};
+
+void foo() {
+      Bar();
+          Test<void>();
+}
+}