[Clang] Do not treat Foo -> const Foo conversion sequences as perfect

[cor3ntin]

For implicit object arguments.
This fixes a regression introduced by the "perfect match" overload resolution mechanism introduced in 8c5a307.

Note that GCC allows the ambiguity between a const and non-const candidate to be resolved. But this patch focuses on restoring the Clang 20 behavior, and to fix the cases which we did resolve incorrectly.

Fixes #147374

[llvmbot]

@llvm/pr-subscribers-clang

Author: Corentin Jabot (cor3ntin)

Changes

For implicit object arguments.
This fixes a regression introduced by the "perfect match" overload resolusion mechanism introduced in #8c5a307.

Note that GCC allows the ambiguity between a const and non-const candidate to be resolved. But this patch focuses on restoring the Clang 20 behavior, and to fix the cases which we did resolve incorrectly.

Fixes #147374

---

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

3 Files Affected:

• (modified) clang/include/clang/Sema/Overload.h (+9-3)
• (modified) clang/lib/Sema/SemaOverload.cpp (+17-5)
• (modified) clang/test/SemaCXX/overload-resolution-deferred-templates.cpp (+28)

diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h
index a70335bef9dd4..bc42e1684ba99 100644
--- a/clang/include/clang/Sema/Overload.h
+++ b/clang/include/clang/Sema/Overload.h
@@ -350,6 +350,11 @@ class Sema;
     LLVM_PREFERRED_TYPE(bool)
     unsigned BindsToRvalue : 1;
 
+    /// Whether this was an identity conversion with qualification
+    /// conversion for the implicit object argument.
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned IsImplicitObjectArgumentQualificationConversion : 1;
+
     /// Whether this binds an implicit object argument to a
     /// non-static member function without a ref-qualifier.
     LLVM_PREFERRED_TYPE(bool)
@@ -448,11 +453,12 @@ class Sema;
 #endif
         return true;
       }
-      if (!C.hasSameType(getFromType(), getToType(2)))
-        return false;
       if (BindsToRvalue && IsLvalueReference)
         return false;
-      return true;
+      if (IsImplicitObjectArgumentQualificationConversion) {
+        return C.hasSameUnqualifiedType(getFromType(), getToType(2));
+      }
+      return C.hasSameType(getFromType(), getToType(2));
     }
 
     ImplicitConversionRank getRank() const;
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 7af3acacb5ba6..cf6e7ed15a46a 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -245,6 +245,7 @@ void StandardConversionSequence::setAsIdentityConversion() {
   IsLvalueReference = true;
   BindsToFunctionLvalue = false;
   BindsToRvalue = false;
+  IsImplicitObjectArgumentQualificationConversion = false;
   BindsImplicitObjectArgumentWithoutRefQualifier = false;
   ObjCLifetimeConversionBinding = false;
   FromBracedInitList = false;
@@ -5305,10 +5306,10 @@ TryReferenceInit(Sema &S, Expr *Init, QualType DeclType,
     // FIXME: As a speculative fix to a defect introduced by CWG2352, we rank
     // a reference binding that performs a non-top-level qualification
     // conversion as a qualification conversion, not as an identity conversion.
-    ICS.Standard.Third = (RefConv &
-                              Sema::ReferenceConversions::NestedQualification)
-                             ? ICK_Qualification
-                             : ICK_Identity;
+    ICS.Standard.Third =
+        (RefConv & Sema::ReferenceConversions::NestedQualification)
+            ? ICK_Qualification
+            : ICK_Identity;
     ICS.Standard.setFromType(T2);
     ICS.Standard.setToType(0, T2);
     ICS.Standard.setToType(1, T1);
@@ -5317,6 +5318,7 @@ TryReferenceInit(Sema &S, Expr *Init, QualType DeclType,
     ICS.Standard.DirectBinding = BindsDirectly;
     ICS.Standard.IsLvalueReference = !isRValRef;
     ICS.Standard.BindsToFunctionLvalue = T2->isFunctionType();
+    ICS.Standard.IsImplicitObjectArgumentQualificationConversion = false;
     ICS.Standard.BindsToRvalue = InitCategory.isRValue();
     ICS.Standard.BindsImplicitObjectArgumentWithoutRefQualifier = false;
     ICS.Standard.ObjCLifetimeConversionBinding =
@@ -5496,6 +5498,7 @@ TryReferenceInit(Sema &S, Expr *Init, QualType DeclType,
     ICS.Standard.IsLvalueReference = !isRValRef;
     ICS.Standard.BindsToFunctionLvalue = false;
     ICS.Standard.BindsToRvalue = true;
+    ICS.Standard.IsImplicitObjectArgumentQualificationConversion = false;
     ICS.Standard.BindsImplicitObjectArgumentWithoutRefQualifier = false;
     ICS.Standard.ObjCLifetimeConversionBinding = false;
   } else if (ICS.isUserDefined()) {
@@ -5518,6 +5521,8 @@ TryReferenceInit(Sema &S, Expr *Init, QualType DeclType,
     ICS.UserDefined.After.IsLvalueReference = !isRValRef;
     ICS.UserDefined.After.BindsToFunctionLvalue = false;
     ICS.UserDefined.After.BindsToRvalue = !LValRefType;
+    ICS.UserDefined.After.IsImplicitObjectArgumentQualificationConversion =
+        false;
     ICS.UserDefined.After.BindsImplicitObjectArgumentWithoutRefQualifier = false;
     ICS.UserDefined.After.ObjCLifetimeConversionBinding = false;
     ICS.UserDefined.After.FromBracedInitList = false;
@@ -5802,6 +5807,7 @@ TryListConversion(Sema &S, InitListExpr *From, QualType ToType,
       StandardConversionSequence &SCS = Result.isStandard() ? Result.Standard :
                                             Result.UserDefined.After;
       SCS.ReferenceBinding = true;
+      SCS.IsImplicitObjectArgumentQualificationConversion = false;
       SCS.IsLvalueReference = ToType->isLValueReferenceType();
       SCS.BindsToRvalue = true;
       SCS.BindsToFunctionLvalue = false;
@@ -5999,8 +6005,12 @@ static ImplicitConversionSequence TryObjectArgumentInitialization(
   // affects the conversion rank.
   QualType ClassTypeCanon = S.Context.getCanonicalType(ClassType);
   ImplicitConversionKind SecondKind;
-  if (ClassTypeCanon == FromTypeCanon.getLocalUnqualifiedType()) {
+  bool IsQualificationConversion = false;
+  if (ImplicitParamType.getCanonicalType() == FromTypeCanon) {
     SecondKind = ICK_Identity;
+  } else if (ClassTypeCanon == FromTypeCanon.getLocalUnqualifiedType()) {
+    SecondKind = ICK_Identity;
+    IsQualificationConversion = true;
   } else if (S.IsDerivedFrom(Loc, FromType, ClassType)) {
     SecondKind = ICK_Derived_To_Base;
   } else if (!Method->isExplicitObjectMemberFunction()) {
@@ -6041,6 +6051,8 @@ static ImplicitConversionSequence TryObjectArgumentInitialization(
   ICS.Standard.setFromType(FromType);
   ICS.Standard.setAllToTypes(ImplicitParamType);
   ICS.Standard.ReferenceBinding = true;
+  ICS.Standard.IsImplicitObjectArgumentQualificationConversion =
+      IsQualificationConversion;
   ICS.Standard.DirectBinding = true;
   ICS.Standard.IsLvalueReference = Method->getRefQualifier() != RQ_RValue;
   ICS.Standard.BindsToFunctionLvalue = false;
diff --git a/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp b/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp
index 46c3670848529..135865c8450f5 100644
--- a/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp
+++ b/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp
@@ -283,3 +283,31 @@ void f() {
 }
 
 #endif
+
+namespace GH147374 {
+
+struct String {};
+template <typename T> void operator+(T, String &&) = delete;
+
+struct Bar {
+    void operator+(String) const; // expected-note {{candidate function}}
+    friend void operator+(Bar, String) {};  // expected-note {{candidate function}}
+};
+
+struct Baz {
+    void operator+(String); // expected-note {{candidate function}}
+    friend void operator+(Baz, String) {}; // expected-note {{candidate function}}
+};
+
+void test() {
+    Bar a;
+    String b;
+    a + b;
+    //expected-error@-1 {{use of overloaded operator '+' is ambiguous (with operand types 'Bar' and 'String')}}
+
+    Baz z;
+    z + b;
+    //expected-error@-1 {{use of overloaded operator '+' is ambiguous (with operand types 'Baz' and 'String')}}
+}
+
+}

[erichkeane]

2 nits, else LGTM.

[erichkeane]

Nit: No curleys here.

[erichkeane]

Unrelated change?

[MatzeB]

FYI: This change breaks common thrift-generated code. Reduced example here shows the problem and produces "expression is not assignable" after this change but is accepted by all released gcc and clang versions as far as I can tell. No idea if the code is valid/invalid:

#include <exception>
struct TmsException : public std::exception {
  int _what;

  const char* what() const noexcept override { return ""; }

  template<typename..., typename T=int>
  T& what() & { return _what; }
};
int main() {
  TmsException E;
  E.what() = 42;
}

[alexfh]

Heads up: this commit causes a huge fallout in our codebase. We'll try to come up with some specific examples tomorrow.

[cor3ntin]

Thanks for the heads up!

I'm looking into it.

Reduced to

struct TmsException  {
  int _what;
  const char* what() const;

  template<typename T=int>
  int& what() & { return _what; }
};
int main() {
  TmsException E;
  E.what() = 42;
}

[bgra8]

Thanks for reverting @cor3ntin !

The fallout @alexfh refers too is caused by code that reduces to something similar to what @MatzeB reported.

[cor3ntin]

Sorry for the inconvenience. It was clearly not the right solution in hindsight. It may take me a while to figure out how to approach a fix to #147374