[Clang] Better handle overload of explicit object member functions

[cor3ntin]

struct A {
    constexpr int f() const { return 1; }  // #1
};

struct B : A {
    using A::f;
    constexpr int f(this A) { return 2; }
};

static_assert(B{}.f() == 1);

In the above scenario, we failed to realize the second f was an overload.
This was caused by a few compounding deficiences in how we handle reference qualified member objects (and non-qualified implicit object member functions).

We also failed to consider that #1 has a better conversion sequence.

Fixes #99902

[llvmbot]

@llvm/pr-subscribers-clang

Author: Corentin Jabot (cor3ntin)

Changes

struct A {
    constexpr int f() const { return 1; }  // #<!-- -->1
};

struct B : A {
    using A::f;
    constexpr int f(this A) { return 2; }
};

static_assert(B{}.f() == 1);

In the above scenario, we failed to realize the second f was an overload.
This was caused by a few compounding deficiences in how we handle reference qualified member objects (and non-qualified implicit object member functions).

We also failed to consider that #<!-- -->1 has a better conversion sequence.

Fixes #99902

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Sema/SemaOverload.cpp (+16-7)
• (modified) clang/test/SemaCXX/cxx2b-deducing-this.cpp (+43)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a6be59f1d6bd7..ba0c9cdcd11a6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -903,6 +903,7 @@ Bug Fixes to C++ Support
 - Fix a bug where private access specifier of overloaded function not respected. (#GH107629)
 - Correctly handle allocations in the condition of a ``if constexpr``.(#GH120197) (#GH134820)
 - Fixed a crash when handling invalid member using-declaration in C++20+ mode. (#GH63254)
+- Fix the handling of overload sets mixing explicit and implicit member functions. (#GH99902)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 34f512b13d6a8..c859268afa3b2 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1483,8 +1483,8 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
     if (BS.Ty != DS.Ty)
       return false;
 
-    if (Base->isLValueReferenceType())
-      return D->isLValueReferenceType();
+    if (Base->isLValueReferenceType() || D->isLValueReferenceType())
+      return Base->isLValueReferenceType() == D->isLValueReferenceType();
     return Base->isRValueReferenceType() == D->isRValueReferenceType();
   };
 
@@ -1536,10 +1536,19 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
                !F->isExplicitObjectMemberFunction();
       };
 
-      if (IsImplicitWithNoRefQual(Old) != IsImplicitWithNoRefQual(New) &&
-          CompareType(OldObjectType.getNonReferenceType(),
-                      NewObjectType.getNonReferenceType()))
-        return true;
+      if ((Old->getRefQualifier() != RQ_None ||
+           Old->isExplicitObjectMemberFunction()) &&
+          IsImplicitWithNoRefQual(New) &&
+          OldObjectType->isRValueReferenceType())
+        NewObjectType =
+            SemaRef.getASTContext().getRValueReferenceType(NewObjectType);
+      else if ((New->getRefQualifier() != RQ_None ||
+                New->isExplicitObjectMemberFunction()) &&
+               IsImplicitWithNoRefQual(Old) &&
+               NewObjectType->isRValueReferenceType())
+        OldObjectType =
+            SemaRef.getASTContext().getRValueReferenceType(OldObjectType);
+
       return CompareType(OldObjectType, NewObjectType);
     }(OldMethod, NewMethod);
 
@@ -5922,7 +5931,7 @@ static ImplicitConversionSequence TryObjectArgumentInitialization(
   if (Method->isExplicitObjectMemberFunction()) {
     if (ExplicitParameterType.isNull())
       ExplicitParameterType = Method->getFunctionObjectParameterReferenceType();
-    OpaqueValueExpr TmpExpr(Loc, FromType.getNonReferenceType(),
+    OpaqueValueExpr TmpExpr(Loc, FromType,
                             ValueKindFromClassification(FromClassification));
     ImplicitConversionSequence ICS = TryCopyInitialization(
         S, &TmpExpr, ExplicitParameterType, SuppressUserConversion,
diff --git a/clang/test/SemaCXX/cxx2b-deducing-this.cpp b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
index 2286da8d1c0e5..a4a5224782f5e 100644
--- a/clang/test/SemaCXX/cxx2b-deducing-this.cpp
+++ b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
@@ -1168,3 +1168,46 @@ struct S {
   bool g() { return f(); } // expected-error {{no viable conversion from returned value of type 'S' to function return type 'bool'}}
 };
 }
+
+namespace GH99902 {
+struct A {
+    constexpr int f() const { return 1; }
+    constexpr int g() const & { return 1; }
+    constexpr int h() const && { return 1; }
+
+    constexpr int i() { return 1; }
+    constexpr int j() & { return 1; }
+    constexpr int k() && { return 1; }
+
+};
+
+struct B : A {
+    using A::f;
+    using A::g;
+    using A::h;
+    using A::i;
+    using A::j;
+    using A::k;
+    constexpr int f(this A) { return 2; }
+    constexpr int g(this A) { return 2; }
+    constexpr int h(this A) { return 2; }
+    constexpr int i(this A) { return 2; }
+    constexpr int j(this A) { return 2; }
+    constexpr int k(this A) { return 2; }
+};
+
+constexpr B b;
+static_assert(B{}.f() == 1);
+static_assert(B{}.g() == 1);
+static_assert(B{}.h() == 1);
+static_assert(B{}.i() == 1);
+static_assert(B{}.j() == 2);
+static_assert(B{}.k() == 1);
+static_assert(b.f() == 1);
+static_assert(b.g() == 1);
+static_assert(b.h() == 2);
+static_assert(b.i() == 2);
+static_assert(b.j() == 2);
+static_assert(b.k() == 2);
+
+}

[erichkeane]

Perhaps too early in the morning, but this whole predicate (and the one below it) are both... rough :) Is there a good way to make them more readable by extracting named variables or something?