[Clang][Sema] Ensure that the selected candidate for a member function explicit specialization is more constrained than all others

[sdkrystian]

As pointed out by @zygoloid in this comment, the selection of the most constrained candidate for member function explicit specializations introduced in #88963 does not check whether the selected candidate is more constrained than all other candidates, which can result in ambiguities being undiagnosed. This patch addresses the issue.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

As pointed out by @zygoloid in this comment, the selection of the most constrained candidate for member function explicit specializations introduced in #88963 does not check whether the selected candidate is more constrained than all other candidates, which can result in ambiguities being undiagnosed. This patch addresses the issue.

---

Full diff: https://github.com/llvm/llvm-project/pull/101721.diff

3 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+5-3)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+51-22)
• (modified) clang/test/CXX/temp/temp.spec/temp.expl.spec/p14-23.cpp (+73-43)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 694a754646f27..004130b0b996b 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -7964,8 +7964,9 @@ NamedDecl *Sema::ActOnVariableDeclarator(
     D.setRedeclaration(CheckVariableDeclaration(NewVD, Previous));
   } else {
     // If this is an explicit specialization of a static data member, check it.
-    if (IsMemberSpecialization && !IsVariableTemplateSpecialization &&
-        !NewVD->isInvalidDecl() && CheckMemberSpecialization(NewVD, Previous))
+    if (IsMemberSpecialization && !IsVariableTemplate &&
+        !IsVariableTemplateSpecialization && !NewVD->isInvalidDecl() &&
+        CheckMemberSpecialization(NewVD, Previous))
       NewVD->setInvalidDecl();
 
     // Merge the decl with the existing one if appropriate.
@@ -10465,7 +10466,8 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
                                                 Previous))
           NewFD->setInvalidDecl();
       }
-    } else if (isMemberSpecialization && isa<CXXMethodDecl>(NewFD)) {
+    } else if (isMemberSpecialization && !FunctionTemplate &&
+               isa<CXXMethodDecl>(NewFD)) {
       if (CheckMemberSpecialization(NewFD, Previous))
           NewFD->setInvalidDecl();
     }
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 87b1f98bbe5ac..38868fb50a3c4 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -9051,7 +9051,8 @@ bool Sema::CheckFunctionTemplateSpecialization(
 
 bool
 Sema::CheckMemberSpecialization(NamedDecl *Member, LookupResult &Previous) {
-  assert(!isa<TemplateDecl>(Member) && "Only for non-template members");
+  assert(!Member->isTemplateDecl() && !Member->getDescribedTemplate() &&
+         "Only for non-template members");
 
   // Try to find the member we are instantiating.
   NamedDecl *FoundInstantiation = nullptr;
@@ -9062,21 +9063,25 @@ Sema::CheckMemberSpecialization(NamedDecl *Member, LookupResult &Previous) {
   if (Previous.empty()) {
     // Nowhere to look anyway.
   } else if (FunctionDecl *Function = dyn_cast<FunctionDecl>(Member)) {
-    SmallVector<FunctionDecl *> Candidates;
-    bool Ambiguous = false;
-    for (LookupResult::iterator I = Previous.begin(), E = Previous.end();
-           I != E; ++I) {
-      CXXMethodDecl *Method =
-          dyn_cast<CXXMethodDecl>((*I)->getUnderlyingDecl());
+    UnresolvedSet<8> Candidates;
+    for (NamedDecl *Candidate : Previous) {
+      auto *Method = dyn_cast<CXXMethodDecl>(Candidate->getUnderlyingDecl());
+      // Ignore any candidates that aren't member functions.
       if (!Method)
         continue;
+
       QualType Adjusted = Function->getType();
       if (!hasExplicitCallingConv(Adjusted))
         Adjusted = adjustCCAndNoReturn(Adjusted, Method->getType());
+      // Ignore any candidates with the wrong type.
       // This doesn't handle deduced return types, but both function
       // declarations should be undeduced at this point.
+      // FIXME: The exception specification should probably be ignored when
+      // comparing the types.
       if (!Context.hasSameType(Adjusted, Method->getType()))
         continue;
+
+      // Ignore any candidates with unsatisfied constraints.
       if (ConstraintSatisfaction Satisfaction;
           Method->getTrailingRequiresClause() &&
           (CheckFunctionConstraints(Method, Satisfaction,
@@ -9084,29 +9089,53 @@ Sema::CheckMemberSpecialization(NamedDecl *Member, LookupResult &Previous) {
                                     /*ForOverloadResolution=*/true) ||
            !Satisfaction.IsSatisfied))
         continue;
-      Candidates.push_back(Method);
-      FunctionDecl *MoreConstrained =
-          Instantiation ? getMoreConstrainedFunction(
-                              Method, cast<FunctionDecl>(Instantiation))
-                        : Method;
-      if (!MoreConstrained) {
-        Ambiguous = true;
-        continue;
+
+      Candidates.addDecl(Candidate);
+    }
+
+    // If we have no viable candidates left after filtering, we are done.
+    if (Candidates.empty())
+      return false;
+
+    // Find the function that is more constrained than every other function it
+    // has been compared to.
+    UnresolvedSetIterator Best = Candidates.begin();
+    CXXMethodDecl *BestMethod = nullptr;
+    for (UnresolvedSetIterator I = Candidates.begin(), E = Candidates.end();
+         I != E; ++I) {
+      auto *Method = cast<CXXMethodDecl>(I->getUnderlyingDecl());
+      if (I == Best ||
+          getMoreConstrainedFunction(Method, BestMethod) == Method) {
+        Best = I;
+        BestMethod = Method;
       }
-      if (MoreConstrained == Method) {
-        Ambiguous = false;
-        FoundInstantiation = *I;
-        Instantiation = Method;
-        InstantiatedFrom = Method->getInstantiatedFromMemberFunction();
-        MSInfo = Method->getMemberSpecializationInfo();
+    }
+
+    FoundInstantiation = *Best;
+    Instantiation = BestMethod;
+    InstantiatedFrom = BestMethod->getInstantiatedFromMemberFunction();
+    MSInfo = BestMethod->getMemberSpecializationInfo();
+
+    // Make sure the best candidate is more constrained than all of the others.
+    bool Ambiguous = false;
+    for (UnresolvedSetIterator I = Candidates.begin(), E = Candidates.end();
+         I != E; ++I) {
+      auto *Method = cast<CXXMethodDecl>(I->getUnderlyingDecl());
+      if (I != Best &&
+          getMoreConstrainedFunction(Method, BestMethod) != BestMethod) {
+        Ambiguous = true;
+        break;
       }
     }
+
     if (Ambiguous) {
       Diag(Member->getLocation(), diag::err_function_member_spec_ambiguous)
           << Member << (InstantiatedFrom ? InstantiatedFrom : Instantiation);
-      for (FunctionDecl *Candidate : Candidates)
+      for (NamedDecl *Candidate : Candidates) {
+        Candidate = Candidate->getUnderlyingDecl();
         Diag(Candidate->getLocation(), diag::note_function_member_spec_matched)
             << Candidate;
+      }
       return true;
     }
   } else if (isa<VarDecl>(Member)) {
diff --git a/clang/test/CXX/temp/temp.spec/temp.expl.spec/p14-23.cpp b/clang/test/CXX/temp/temp.spec/temp.expl.spec/p14-23.cpp
index dc17cea99d435..a023bf84137d7 100644
--- a/clang/test/CXX/temp/temp.spec/temp.expl.spec/p14-23.cpp
+++ b/clang/test/CXX/temp/temp.spec/temp.expl.spec/p14-23.cpp
@@ -1,60 +1,90 @@
 // RUN: %clang_cc1 -std=c++20 -verify %s
 
-template<int I>
-concept C = I >= 4;
+namespace N0 {
+  template<int I>
+  concept C = I >= 4;
 
-template<int I>
-concept D = I < 8;
+  template<int I>
+  concept D = I < 8;
 
-template<int I>
-struct A {
-  constexpr static int f() { return 0; }
-  constexpr static int f() requires C<I> && D<I> { return 1; }
-  constexpr static int f() requires C<I> { return 2; }
+  template<int I>
+  struct A {
+    constexpr static int f() { return 0; }
+    constexpr static int f() requires C<I> && D<I> { return 1; }
+    constexpr static int f() requires C<I> { return 2; }
 
-  constexpr static int g() requires C<I> { return 0; } // #candidate-0
-  constexpr static int g() requires D<I> { return 1; } // #candidate-1
+    constexpr static int g() requires C<I> { return 0; } // #candidate-0
+    constexpr static int g() requires D<I> { return 1; } // #candidate-1
 
-  constexpr static int h() requires C<I> { return 0; } // expected-note {{member declaration nearly matches}}
-};
+    constexpr static int h() requires C<I> { return 0; } // expected-note {{member declaration nearly matches}}
+  };
 
-template<>
-constexpr int A<2>::f() { return 3; }
+  template<>
+  constexpr int A<2>::f() { return 3; }
 
-template<>
-constexpr int A<4>::f() { return 4; }
+  template<>
+  constexpr int A<4>::f() { return 4; }
 
-template<>
-constexpr int A<8>::f() { return 5; }
+  template<>
+  constexpr int A<8>::f() { return 5; }
 
-static_assert(A<3>::f() == 0);
-static_assert(A<5>::f() == 1);
-static_assert(A<9>::f() == 2);
-static_assert(A<2>::f() == 3);
-static_assert(A<4>::f() == 4);
-static_assert(A<8>::f() == 5);
+  static_assert(A<3>::f() == 0);
+  static_assert(A<5>::f() == 1);
+  static_assert(A<9>::f() == 2);
+  static_assert(A<2>::f() == 3);
+  static_assert(A<4>::f() == 4);
+  static_assert(A<8>::f() == 5);
 
-template<>
-constexpr int A<0>::g() { return 2; }
+  template<>
+  constexpr int A<0>::g() { return 2; }
 
-template<>
-constexpr int A<8>::g() { return 3; }
+  template<>
+  constexpr int A<8>::g() { return 3; }
 
-template<>
-constexpr int A<6>::g() { return 4; } // expected-error {{ambiguous member function specialization 'A<6>::g' of 'A::g'}}
-                                      // expected-note@#candidate-0 {{member function specialization matches 'g'}}
-                                      // expected-note@#candidate-1 {{member function specialization matches 'g'}}
+  template<>
+  constexpr int A<6>::g() { return 4; } // expected-error {{ambiguous member function specialization 'N0::A<6>::g' of 'N0::A::g'}}
+                                        // expected-note@#candidate-0 {{member function specialization matches 'g'}}
+                                        // expected-note@#candidate-1 {{member function specialization matches 'g'}}
 
-static_assert(A<9>::g() == 0);
-static_assert(A<1>::g() == 1);
-static_assert(A<0>::g() == 2);
-static_assert(A<8>::g() == 3);
+  static_assert(A<9>::g() == 0);
+  static_assert(A<1>::g() == 1);
+  static_assert(A<0>::g() == 2);
+  static_assert(A<8>::g() == 3);
 
-template<>
-constexpr int A<4>::h() { return 1; }
+  template<>
+  constexpr int A<4>::h() { return 1; }
 
-template<>
-constexpr int A<0>::h() { return 2; } // expected-error {{out-of-line definition of 'h' does not match any declaration in 'A<0>'}}
+  template<>
+  constexpr int A<0>::h() { return 2; } // expected-error {{out-of-line definition of 'h' does not match any declaration in 'N0::A<0>'}}
 
-static_assert(A<5>::h() == 0);
-static_assert(A<4>::h() == 1);
+  static_assert(A<5>::h() == 0);
+  static_assert(A<4>::h() == 1);
+} // namespace N0
+
+namespace N1 {
+  template<int I>
+  concept C = I > 0;
+
+  template<int I>
+  concept D = I > 1;
+
+  template<int I>
+  concept E = I > 2;
+
+  template<int I>
+  struct A {
+    void f() requires C<I> && D<I>; // expected-note {{member function specialization matches 'f'}}
+    void f() requires C<I> && E<I>; // expected-note {{member function specialization matches 'f'}}
+    void f() requires C<I> && D<I> && true; // expected-note {{member function specialization matches 'f'}}
+
+    void g() requires C<I> && E<I>; // expected-note {{member function specialization matches 'g'}}
+    void g() requires C<I> && D<I>; // expected-note {{member function specialization matches 'g'}}
+    void g() requires C<I> && D<I> && true; // expected-note {{member function specialization matches 'g'}}
+  };
+
+  template<>
+  void A<3>::f(); // expected-error {{ambiguous member function specialization 'N1::A<3>::f' of 'N1::A::f'}}
+
+  template<>
+  void A<3>::g(); // expected-error {{ambiguous member function specialization 'N1::A<3>::g' of 'N1::A::g'}}
+} // namespace N1

[mizvekov]

LGTM

But please give it some time for @zygoloid to take a look in case we are missing anything else.

I see there are a few diagnostics sugar regressions, but they are not the fault of this patch, the issues are unrelated.

[zygoloid]

Changes to the algorithm for selecting the most constrained function look correct. It's not clear to me whether it's correct to stop calling CheckMemberSpecialization for member templates, though -- do we still do the necessary checks somewhere else?

[zygoloid]

LGTM too, thanks