[clang] Compare constraints before diagnosing mismatched ref qualifiers (GH58962)

[royjacobson]

Summary

As noticed in GH58962, we should only diagnose illegal overloads of member functions
when the ref qualifiers don't match if the trailing constraints are the same.

The fix is to move the existing constraints check earlier in Sema::IsOverload.

Closes https://github.com/llvm/llvm-project/issues/58962

[cor3ntin]

clang/lib/Sema/SemaOverload.cpp

1324

I know it's preexisting but, I'm not sure that comment adds anything.
If we want to keep it, "requires clauses are different, - these are overloads."

[erichkeane]

I can't find any example that breaks and needs the AreConstraintExpressionsEqual, so feel free to do it if you'd like.

clang/lib/Sema/SemaOverload.cpp

1324

The comment is definitely low quality here, I would probably prefer a little more elaboration if we're going to keep it.

1327

I did some work that extracted these at one point, and it matters because we have to 'fix' the depths sometimes. I suspect it wont' really come up here, since we have already checked that these are both the same 'type' of template parameter by now? But at some point (perhaps not in this patch?) we might find calling Sema::AreConstraintExpressionsEqual necessary.

[erichkeane]

Feel free to clarify the comment yourself.

[royjacobson]

Remove redundant comments, use Sema::AreConstraintExpressionsEqual instead.

[royjacobson]

clang/lib/Sema/SemaOverload.cpp

1324

removed it.

1327

Went for Sema::AreConstraintExpressionsEqual to reduce code duplication. Only drawback is calling CalculateTemplateDepthForConstraints unnecessarily which seems reasonable to me.

clang/lib/Sema/SemaOverload.cpp

if (NewTemplate) {
     // decide whether a member introduced by a shadow declaration is hidden.
     if (UseMemberUsingDeclRules && ConstraintsInTemplateHead &&
         !SameTemplateParameterList)
       return true;
     if (!UseMemberUsingDeclRules &&
         (!SameTemplateParameterList || !SameReturnType))
       return true;
   }
+
+  if (ConsiderRequiresClauses) {
+    Expr *NewRC = New->getTrailingRequiresClause(),
+         *OldRC = Old->getTrailingRequiresClause();
+    if ((NewRC != nullptr) != (OldRC != nullptr))
+      return true;
+
+    if (NewRC && !AreConstraintExpressionsEqual(Old, OldRC, New, NewRC))
+        return true;
+  }
+
   // If the function is a class member, its signature includes the
   // cv-qualifiers (if any) and ref-qualifier (if any) on the function itself.
   //
   // As part of this, also check whether one of the member functions
   // is static, in which case they are not overloads (C++
   // 13.1p2). While not part of the definition of the signature,
   // this check is important to determine whether these functions
   // can be overloaded.
   CXXMethodDecl *OldMethod = dyn_cast<CXXMethodDecl>(Old);
   CXXMethodDecl *NewMethod = dyn_cast<CXXMethodDecl>(New);
   if (OldMethod && NewMethod &&
       !OldMethod->isStatic() && !NewMethod->isStatic()) {
     if (OldMethod->getRefQualifier() != NewMethod->getRefQualifier()) {
       if (!UseMemberUsingDeclRules &&
           (OldMethod->getRefQualifier() == RQ_None ||
            NewMethod->getRefQualifier() == RQ_None)) {
-        // C++0x [over.load]p2:
-        //   - Member function declarations with the same name and the same
-        //     parameter-type-list as well as member function template
-        //     declarations with the same name, the same parameter-type-list, and
-        //     the same template parameter lists cannot be overloaded if any of
-        //     them, but not all, have a ref-qualifier (8.3.5).
+        // C++20 [over.load]p2:
+        //   - Member function declarations with the same name, the same
+        //     parameter-type-list, and the same trailing requires-clause (if
+        //     any), as well as member function template declarations with the
+        //     same name, the same parameter-type-list, the same trailing
+        //     requires-clause (if any), and the same template-head, cannot be
+        //     overloaded if any of them, but not all, have a ref-qualifier.
         Diag(NewMethod->getLocation(), diag::err_ref_qualifier_overload)
             << NewMethod->getRefQualifier() << OldMethod->getRefQualifier();
         Diag(OldMethod->getLocation(), diag::note_previous_declaration);
       }
       return true;
     }
 
     // We may not have applied the implicit const for a constexpr member
     // function yet (because we haven't yet resolved whether this is a static
     // or non-static member function). Add it now, on the assumption that this
if (!isa<CXXDestructorDecl>(New)) {
         // Allow overloading of functions with same signature and different CUDA
         // target attributes.
         if (NewTarget != OldTarget)
           return true;
       }
     }
   }
 
-  if (ConsiderRequiresClauses) {
-    Expr *NewRC = New->getTrailingRequiresClause(),
-         *OldRC = Old->getTrailingRequiresClause();
-    if ((NewRC != nullptr) != (OldRC != nullptr))
-      // RC are most certainly different - these are overloads.
-      return true;
-
-    if (NewRC) {
-      llvm::FoldingSetNodeID NewID, OldID;
-      NewRC->Profile(NewID, Context, /*Canonical=*/true);
-      OldRC->Profile(OldID, Context, /*Canonical=*/true);
-      if (NewID != OldID)
-        // RCs are not equivalent - these are overloads.
-        return true;
-    }
-  }
-
   // The signatures match; this is not an overload.
   return false;
 }
 
 /// Tries a user-defined conversion from From to ToType.
 ///
 /// Produces an implicit conversion sequence for when a standard conversion
 /// is not an option. See TryImplicitConversion for more information.

clang/test/CXX/over/over.load/p2-0x.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
 
 // Member function declarations with the same name and the same
 // parameter-type-list as well as mem- ber function template
 // declarations with the same name, the same parameter-type-list, and
 // the same template parameter lists cannot be overloaded if any of
 // them, but not all, have a ref-qualifier (8.3.5).
 
 class Y { 
   void h() &; 
   void h() const &; 
   void h() &&; 
   void i() &; // expected-note{{previous declaration}}
   void i() const; // expected-error{{cannot overload a member function without a ref-qualifier with a member function with ref-qualifier '&'}}
 
   template<typename T> void f(T*) &;
   template<typename T> void f(T*) &&;
 
   template<typename T> void g(T*) &; // expected-note{{previous declaration}}
   template<typename T> void g(T*); // expected-error{{cannot overload a member function without a ref-qualifier with a member function with ref-qualifier '&'}}
 
   void k(); // expected-note{{previous declaration}}
   void k() &&; // expected-error{{cannot overload a member function with ref-qualifier '&&' with a member function without a ref-qualifier}}
 };
+
+
+#if __cplusplus >= 202002L
+namespace GH58962 {
+
+template<typename T>
+__add_rvalue_reference(T) declval();
+
+template<unsigned R>
+struct type
+{
+    void func() requires (R == 0);
+    void func() & requires (R == 1);
+    void func() && requires (R == 2);
+};
+
+template<typename T>
+concept test = requires { declval<T>().func(); };
+
+static_assert(test<type<0>&>);
+static_assert(test<type<0>&&>);
+static_assert(test<type<1>&>);
+static_assert(not test<type<1>&&>);
+static_assert(not test<type<2>&>);
+static_assert(test<type<2>&&>);
+
+}
+#endif