[Clang] use non-instantiated function declaration for constraints partial ordering

[ychen]

Summary

Per wordings in

• https://eel.is/c++draft/over.match#best.general-2.6
• https://eel.is/c++draft/temp.constr.order
• https://eel.is/c++draft/temp.constr#atomic-1

constraints partial ordering should use the unsubstituted template
parameters of the constrained entity, not the instantiated entity.

Fix #56154

[ychen]

clang/test/CXX/over/over.match/over.match.best/p2.cpp

10

Remove this case because it compiles by accident. https://eel.is/c++draft/over.match#best.general-2.6 take precedence over https://eel.is/c++draft/over.match#best.general-2.8.

[ychen]

• add a release note.

[royjacobson]

LGTM, thanks for picking it up! I wonder how much effect this has on compilation times given that SubsumptionCache caches subsumption by the Decl * pair...

One Q - do you think that adding an assert to IsAtLeastAsConstrained to not accept 'instantiated' functions/structs again is possible somehow?

[erichkeane]

clang/test/CXX/over/over.match/over.match.best/p2.cpp

10

Could you instead replace it with a better comment + a 'fixme' or something if we need to fix it in the future? I don't want to lose the test.

[mizvekov]

LGTM as well!

[ychen]

• do the same when 1. taking the address of a non-template function (with a test) 2. computing SMF eligibility (unfortunately it's hard to compose a test, suggestions are appreciated).
• restore the removed test case

[ychen]

In D136545#3877899, @royjacobson wrote:

LGTM, thanks for picking it up! I wonder how much effect this has on compilation times given that SubsumptionCache caches subsumption by the Decl * pair...

It seems neutral https://llvm-compile-time-tracker.com/compare.php?from=0ec6373e29f4c129490abef9d38015e9ec0b10ce&to=060fafdcf819578b89505c73d3235e44f56caa60&stat=instructions

One Q - do you think that adding an assert to IsAtLeastAsConstrained to not accept 'instantiated' functions/structs again is possible somehow?

Great idea. For sure. I added the assertion and found two more places needing updates.

[ychen]

clang/test/CXX/over/over.match/over.match.best/p2.cpp

10

Sure. I restored the test.

[ychen]

• rebase

[chapuni]

clang/lib/Sema/SemaConcept.cpp

1310

IsExpectedEntity, FD2, (and possibly FD1) are used only in assert.

clang/docs/ReleaseNotes.rst

- Reject non-type template arguments formed by casting a non-zero integer
   pointers.
 - Fix template arguments of pointer and reference not taking the type as
   part of their identity.
   `Issue 47136 <https://github.com/llvm/llvm-project/issues/47136>`_
 - Fix a crash when trying to form a recovery expression on a call inside a
   constraint, which re-evaluated the same constraint.
   `Issue 53213 <https://github.com/llvm/llvm-project/issues/53213>`_
   `Issue 45736 <https://github.com/llvm/llvm-project/issues/45736>`_
+- Fix an issue when performing constraints partial ordering on non-template
+  functions. `Issue 56154 <https://github.com/llvm/llvm-project/issues/56154>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/lib/Sema/SemaConcept.cpp

static bool subsumes(Sema &S, NamedDecl *DP, ArrayRef<const Expr *> P,
   return false;
 }
 
 bool Sema::IsAtLeastAsConstrained(NamedDecl *D1,
                                   MutableArrayRef<const Expr *> AC1,
                                   NamedDecl *D2,
                                   MutableArrayRef<const Expr *> AC2,
                                   bool &Result) {
+  if (const auto *FD1 = dyn_cast<FunctionDecl>(D1)) {
+    auto IsExpectedEntity = [](const FunctionDecl *FD) {
+      FunctionDecl::TemplatedKind Kind = FD->getTemplatedKind();
+      return Kind == FunctionDecl::TK_NonTemplate ||
+             Kind == FunctionDecl::TK_FunctionTemplate;
+    };
+    const auto *FD2 = dyn_cast<FunctionDecl>(D2);
+    assert(IsExpectedEntity(FD1) && FD2 && IsExpectedEntity(FD2) &&
+           "use non-instantiated function declaration for constraints partial "
+           "ordering");
+  }
+
   if (AC1.empty()) {
     Result = AC2.empty();
     return false;
   }
   if (AC2.empty()) {
     // TD1 has associated constraints and TD2 does not.
     Result = true;
     return false;

clang/lib/Sema/SemaDecl.cpp

else {
         SatisfactionStatus.push_back(Satisfaction.IsSatisfied);
     }
   }
 
   for (size_t i = 0; i < Methods.size(); i++) {
     if (!SatisfactionStatus[i])
       continue;
     CXXMethodDecl *Method = Methods[i];
-    const Expr *Constraints = Method->getTrailingRequiresClause();
+    CXXMethodDecl *OrigMethod = Method;
+    if (FunctionDecl *MF = OrigMethod->getInstantiatedFromMemberFunction())
+      OrigMethod = cast<CXXMethodDecl>(MF);
+
+    const Expr *Constraints = OrigMethod->getTrailingRequiresClause();
     bool AnotherMethodIsMoreConstrained = false;
     for (size_t j = 0; j < Methods.size(); j++) {
       if (i == j || !SatisfactionStatus[j])
         continue;
       CXXMethodDecl *OtherMethod = Methods[j];
-      if (!AreSpecialMemberFunctionsSameKind(S.Context, Method, OtherMethod,
+      if (FunctionDecl *MF = OtherMethod->getInstantiatedFromMemberFunction())
+        OtherMethod = cast<CXXMethodDecl>(MF);
+
+      if (!AreSpecialMemberFunctionsSameKind(S.Context, OrigMethod, OtherMethod,
                                              CSM))
         continue;
 
       const Expr *OtherConstraints = OtherMethod->getTrailingRequiresClause();
       if (!OtherConstraints)
         continue;
       if (!Constraints) {
         AnotherMethodIsMoreConstrained = true;
         break;
       }
-      if (S.IsAtLeastAsConstrained(OtherMethod, {OtherConstraints}, Method,
+      if (S.IsAtLeastAsConstrained(OtherMethod, {OtherConstraints}, OrigMethod,
                                    {Constraints},
                                    AnotherMethodIsMoreConstrained)) {
         // There was an error with the constraints comparison. Exit the loop
         // and don't consider this function eligible.
         AnotherMethodIsMoreConstrained = true;
       }
       if (AnotherMethodIsMoreConstrained)
         break;

clang/lib/Sema/SemaOverload.cpp

if (FunctionTemplateDecl *BetterTemplate = S.getMoreSpecializedTemplate(
             Cand1.isReversed() ^ Cand2.isReversed()))
       return BetterTemplate == Cand1.Function->getPrimaryTemplate();
   }
 
   //   -— F1 and F2 are non-template functions with the same
   //      parameter-type-lists, and F1 is more constrained than F2 [...],
   if (!Cand1IsSpecialization && !Cand2IsSpecialization &&
       sameFunctionParameterTypeLists(S, Cand1, Cand2)) {
-    const Expr *RC1 = Cand1.Function->getTrailingRequiresClause();
-    const Expr *RC2 = Cand2.Function->getTrailingRequiresClause();
+    FunctionDecl *Function1 = Cand1.Function;
+    FunctionDecl *Function2 = Cand2.Function;
+    if (FunctionDecl *MF = Function1->getInstantiatedFromMemberFunction())
+      Function1 = MF;
+    if (FunctionDecl *MF = Function2->getInstantiatedFromMemberFunction())
+      Function2 = MF;
+
+    const Expr *RC1 = Function1->getTrailingRequiresClause();
+    const Expr *RC2 = Function2->getTrailingRequiresClause();
     if (RC1 && RC2) {
       bool AtLeastAsConstrained1, AtLeastAsConstrained2;
-      if (S.IsAtLeastAsConstrained(Cand1.Function, RC1, Cand2.Function, RC2,
+      if (S.IsAtLeastAsConstrained(Function1, RC1, Function2, RC2,
                                    AtLeastAsConstrained1) ||
-          S.IsAtLeastAsConstrained(Cand2.Function, RC2, Cand1.Function, RC1,
+          S.IsAtLeastAsConstrained(Function2, RC2, Function1, RC1,
                                    AtLeastAsConstrained2))
         return false;
       if (AtLeastAsConstrained1 != AtLeastAsConstrained2)
         return AtLeastAsConstrained1;
     } else if (RC1 || RC2) {
       return RC1 != nullptr;
     }
   }

 Sema::resolveAddressOfSingleOverloadCandidate(Expr *E, DeclAccessPair &Pair) {
   OverloadExpr::FindResult R = OverloadExpr::find(E);
   OverloadExpr *Ovl = R.Expression;
   bool IsResultAmbiguous = false;
   FunctionDecl *Result = nullptr;
   DeclAccessPair DAP;
   SmallVector<FunctionDecl *, 2> AmbiguousDecls;
 
-  auto CheckMoreConstrained =
-      [&] (FunctionDecl *FD1, FunctionDecl *FD2) -> Optional<bool> {
-        SmallVector<const Expr *, 1> AC1, AC2;
-        FD1->getAssociatedConstraints(AC1);
-        FD2->getAssociatedConstraints(AC2);
-        bool AtLeastAsConstrained1, AtLeastAsConstrained2;
-        if (IsAtLeastAsConstrained(FD1, AC1, FD2, AC2, AtLeastAsConstrained1))
-          return None;
-        if (IsAtLeastAsConstrained(FD2, AC2, FD1, AC1, AtLeastAsConstrained2))
-          return None;
-        if (AtLeastAsConstrained1 == AtLeastAsConstrained2)
-          return None;
-        return AtLeastAsConstrained1;
-      };
+  auto CheckMoreConstrained = [&](FunctionDecl *FD1,
+                                  FunctionDecl *FD2) -> Optional<bool> {
+    if (FunctionDecl *MF = FD1->getInstantiatedFromMemberFunction())
+      FD1 = MF;
+    if (FunctionDecl *MF = FD2->getInstantiatedFromMemberFunction())
+      FD2 = MF;
 
   // Don't use the AddressOfResolver because we're specifically looking for
   // cases where we have one overload candidate that lacks
   // enable_if/pass_object_size/...
   for (auto I = Ovl->decls_begin(), E = Ovl->decls_end(); I != E; ++I) {
     auto *FD = dyn_cast<FunctionDecl>(I->getUnderlyingDecl());
     if (!FD)
       return nullptr;

clang/test/CXX/over/over.match/over.match.best/p2.cpp

 // RUN: %clang_cc1 -std=c++2a -verify %s
 
 namespace PR44761 {
   template<typename T> concept X = (sizeof(T) == sizeof(T));
 
   template<typename T> struct A {
     bool operator<(const A&) const & requires X<T>; // #1
     int operator<=>(const A&) const & requires X<T> && X<int> = delete; // #2
   };
-  bool k1 = A<int>() < A<int>(); // not ordered by constraints: prefer non-rewritten form
+  bool k1 = A<int>() < A<int>(); // prefer more-constrained 'operator<=>'
+  // expected-error@-1 {{deleted}}
+  // expected-note@#1 {{candidate}}
+  // expected-note@#2 {{candidate function has been explicitly deleted}}
+  // expected-note@#2 {{candidate function (with reversed parameter order) has been explicitly deleted}}
   bool k2 = A<float>() < A<float>(); // prefer more-constrained 'operator<=>'
   // expected-error@-1 {{deleted}}
   // expected-note@#1 {{candidate}}
   // expected-note@#2 {{candidate function has been explicitly deleted}}
   // expected-note@#2 {{candidate function (with reversed parameter order) has been explicitly deleted}}
 }

clang/test/CXX/temp/temp.constr/temp.constr.order/non-template-functions.cpp

+// RUN: %clang_cc1 -std=c++2a -x c++ -verify %s
+// expected-no-diagnostics
+
+template<typename T, typename U>
+constexpr static bool is_same_v = false;
+
+template<typename T>
+constexpr static bool is_same_v<T, T> = true;
+
+namespace PR56154 {
+  template <int N> concept C0 = (N == 0);
+  template <int N, int N2> concept C0x = C0<N>;
+  template <int N1, int N2> concept C00 = C0x<N1, N2> && C0<N2>;
+
+  template<int N1, int N2>
+  struct A {
+    void f() requires C00<N1, N2>;
+    void f() requires C0x<N1, N2> = delete;
+
+    static short g() requires C00<N1, N2>;
+    static int g() requires C0x<N1, N2>;
+  };
+  void h(A<0, 0> a) {
+    a.f();
+    static_assert(is_same_v<decltype(&A<0, 0>::g), short(*)()>);
+  }
+}