[Concepts] Check constraints for explicit template instantiations

[royjacobson]

Summary

The standard requires member function constraints to be checked when explicitly instantiating classes. This patch adds this constraints check.

This issue is tracked as #46029.

Note that there's an related open CWG issue (2421) about what to do when multiple candidates have satisfied constraints. This is particularly an issue because mangling doesn't contain function constraints, and so the following code still ICEs with definition with same mangled name '_ZN1BIiE1fEv' as another definition:

template<class T>
struct B {
    int f() requires std::same_as<T, int> {
        return 0;
    }
    int f() requires (std::same_as<T, int> && !std::same_as<T, char>) {
        return 1;
    }
};

template struct B<int>;

Also note that the constraints checking while instantiating *functions* is still not implemented. I started looking at it but It's a bit more complicated. I believe in such a case we have to consider the partial constraints order and potentially choose the best candidate out of the set of multiple valid ones.

[erichkeane]

Please add all the context (see https://llvm.org/docs/Phabricator.html). Also, I'd like to see some level of negative tests.

Finally, I'll have to evaluate how this works with the delayed concepts build.

I don't see this as making it into Clang-14, we are pretty close to release, and I'm unsure of how it'll interact with other stuff just yet.

[royjacobson]

Updated test so it checks actual instantiation and update the description.

[erichkeane]

Hmm... doing FileCheck in a Sema test is highly irregular. I would expect us to be able to test this in the type system in some way. Something like A<3>::f() is invalid (see -verify).

Also, please add all the context (as requested above!) with the -U9999 stuff.

[royjacobson]

Adding context.
(Sorry - didn't realize you were speaking about the diff itself, thought you meant adding general context to the PR message...)

[royjacobson]

In D120255#3352755, @erichkeane wrote:

Hmm... doing FileCheck in a Sema test is highly irregular. I would expect us to be able to test this in the type system in some way. Something like A<3>::f() is invalid (see -verify).

Also, please add all the context (as requested above!) with the -U9999 stuff.

I couldn't think of a way to "internally" know whether some function got instantiated. I also saw some other instantiation tests do it this way (SemaTemplate/instantiate-friend-function.cpp, SemaTemplate/inject-templated-friend.cpp). But I'm really not familiar enough with the code structure to have an opinion about this..

Also, as I wrote in the description - function explicit instantiation doesn't check constraints either but it's a different code path, so, A<3>::f() currently fails for the wrong reason. It will just try to instantiate the first function it finds and then fail the static assert and not the constraint.
(Or worse, fail the internal assert in SemaTemplate.cpp:10161 if clang was built with assertions).

[royjacobson]

Slightly update the test command line (add -triple %itanium_abi_triple like in other similar tests).

[erichkeane]

In D120255#3352920, @royjacobson wrote:

In D120255#3352755, @erichkeane wrote:

Hmm... doing FileCheck in a Sema test is highly irregular. I would expect us to be able to test this in the type system in some way. Something like A<3>::f() is invalid (see -verify).

Also, please add all the context (as requested above!) with the -U9999 stuff.

I couldn't think of a way to "internally" know whether some function got instantiated. I also saw some other instantiation tests do it this way (SemaTemplate/instantiate-friend-function.cpp, SemaTemplate/inject-templated-friend.cpp). But I'm really not familiar enough with the code structure to have an opinion about this..

Also, as I wrote in the description - function explicit instantiation doesn't check constraints either but it's a different code path, so, A<3>::f() currently fails for the wrong reason. It will just try to instantiate the first function it finds and then fail the static assert and not the constraint.
(Or worse, fail the internal assert in SemaTemplate.cpp:10161 if clang was built with assertions).

The only other idea I have (other than finding a static-assert of some sort) is to do an AST level test instead, so use -ast-dump and check that it appears/not appears in the AST instead? That way at least the ordering will be more reliable.

[aaron.ballman]

clang/test/SemaTemplate/constraints-instantiation.cpp

4–9

I think it might be better to test this via an -ast-dump test (which lives in the AST test directory) rather than emitting LLVM IR to determine whether something was instantiated or not.

Btw, when you convert the test to use -ast-dump, be sure to make use of regexes for things like line, column numbers, pointer values, etc so that the test is easier to edit without breaking.

[royjacobson]

clang/test/SemaTemplate/constraints-instantiation.cpp

4–9

I tried to look now into doing it in the ast-dump. It seems that we create a new ClassTemplateSpecializationDecl in SemaTemplate:9663 (ActOnExplicitInstantiation) that is copied from the parent ClassTemplateDecl along with all its members and CXXMethodDecls. That means that the member functions with the unsatisfied constraints still appear in the AST.

Do you think I should add the constraints checks into the creation of ClassTemplateSpecializationDecl instead, so we don't create unnecessary CXXMethodDecl?

[erichkeane]

clang/test/SemaTemplate/constraints-instantiation.cpp

4–9

So one of the problems with that is the delayed-concepts thing I'm working on. The idea is that we shouldn't be instantiating or checking until the last moment, so unless I am misunderstanding what you mean, I would think not.

[royjacobson]

Thanks for the quick feedback :)

I looked again at the AST and found out that there is a difference between instantiated and non-instantiated functions I missed - the instantiated functions have actual code AST.
I had some trouble matching the actual generated AST, so I ended up testing that indirectly - I call a second 'canary' template function and validate that it was instantiated with the correct arguments. Couldn't think of something simpler unfortunately :/

[erichkeane]

Do you have push rights, or do you need someone to push for you? If you need someone to push for you, please post the name and email address you'd like the commit under.

[royjacobson]

In D120255#3357007, @erichkeane wrote:

Do you have push rights, or do you need someone to push for you? If you need someone to push for you, please post the name and email address you'd like the commit under.

Thanks a lot! I don't have push rights, so please push this with Roy Jacobson <[e-mail]>
(not a typo, the email has different spelling)

clang/lib/Sema/SemaTemplateInstantiate.cpp

assert(
        (TSK == TSK_ImplicitInstantiation && Instantiation->isLocalClass())) &&
       "Unexpected template specialization kind!");
   for (auto *D : Instantiation->decls()) {
     bool SuppressNew = false;
     if (auto *Function = dyn_cast<FunctionDecl>(D)) {
       if (FunctionDecl *Pattern =
               Function->getInstantiatedFromMemberFunction()) {
 
+        if (Function->getTrailingRequiresClause()) {
+          ConstraintSatisfaction Satisfaction;
+          if (CheckFunctionConstraints(Function, Satisfaction) ||
+              !Satisfaction.IsSatisfied) {
+            continue;
+          }
+        }
+
         if (Function->hasAttr<ExcludeFromExplicitInstantiationAttr>())
           continue;
 
         MemberSpecializationInfo *MSInfo =
             Function->getMemberSpecializationInfo();
         assert(MSInfo && "No member specialization information?");
         if (MSInfo->getTemplateSpecializationKind()
                                                  == TSK_ExplicitSpecialization)

clang/test/SemaTemplate/constraints-instantiation.cpp

+// RUN: %clang_cc1 -S -std=c++20 -emit-llvm %s -o - | FileCheck %s
+
+// void PR46029::A<1>::f()
+// CHECK: define {{.*}} @_ZN7PR460291AILi1EE1fEv
+// void PR46029::A<2>::f()
+// CHECK: define {{.*}} @_ZN7PR460291AILi2EE1fEv
+// void PR46029::A<3>::f()
+// CHECK-NOT: define {{.*}} @_ZN7PR460291AILi3EE1fEv
+
+namespace PR46029 {
+template <int N>
+struct A {
+  void f() requires(N == 1) {
+    static_assert(N == 1);
+  }
+  void f() requires(N == 2) {
+    static_assert(N == 2);
+  }
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct A<3>;
+}