[Sema] Improve notes for value category mismatch in overloading

[aaronpuchert]

Summary

When an overloaded member function has a ref-qualifier, like:

class X {

void f() &&;
void f(int) &;

};

we would print strange notes when the ref-qualifier doesn't fit the value
category:

X x;
x.f();
X().f(0);

would both print a note "no known conversion from 'X' to 'X' for object
argument" on their relevant overload instead of pointing out the
mismatch in value category.

At first I thought the solution is easy: just use the FailureKind member
of the BadConversionSequence struct. But it turns out that we weren't
properly setting this for function arguments. So I went through
TryReferenceInit to make sure we're doing that right, and found a number
of notes in the existing tests that improved as well.

Fixes PR47791.

[aaronpuchert]

clang/lib/Sema/SemaOverload.cpp

4971–4972

No functional change here, I was just using the existing prefetched value.

4972

One might think that this should be rvalue_ref_to_lvalue, but we have a user-defined conversion here, so probably not.

10535–10543

We are in the middle of an if-else cascade that is only setting BaseToDerivedConversion, where I think this doesn't belong. So I moved it directly after the qualifier mismatch handling.

clang/test/SemaCXX/overload-member-call.cpp

86–87

The respective first note was "no known conversion from 'test1::A' to 'test1::A' for object argument", which is silly.

[rsmith]

Thanks!

clang/include/clang/Basic/DiagnosticSemaKinds.td

4292

Please change this to lvalue|rvalue (preferably in a separate commit, and likewise for the half-dozen or so other diagnostics that use this unconventional spelling). In both C and C++ contexts, these words are spelled without a hyphen.

clang/lib/Sema/SemaOverload.cpp

10480–10481

It's surprising to me that nothing else in this function is considering Conv.Bad.Kind. Do you know what's going on there? I see that PerformObjectArgumentInitialization has a switch on it, but it looks like that's the *only* place that uses it, and we mostly instead try to recompute what went wrong after the fact, which seems fragile. I wonder if more of the complexity of this function could be reduced by using the stored bad conversion kind. (But let's keep this patch targeted on just fixing the value category diagnostics!)

[aaronpuchert]

clang/include/clang/Basic/DiagnosticSemaKinds.td

4292

I was wondering about the inconsistent spelling as well. Yes, I'll do this in a follow-up.

clang/lib/Sema/SemaOverload.cpp

10480–10481

The previous if could perhaps be on Conv.Bad.Kind == BadConversionSequence::bad_qualifiers instead, but maybe we're not setting that correctly in some cases. I will try it out.

[aaronpuchert]

Fixed the spelling issue in rGdea31f135ceae6e860e6301f9bb66d3b3adb1357.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_ovl_candidate_bad_conv : Note<
     "; take the address of the argument with &|"
     "; remove *|"
     "; remove &}7">;
 def note_ovl_candidate_bad_arc_conv : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "cannot implicitly convert argument "
     "%diff{of type $ to $|type to parameter type}3,4 for "
     "%select{%ordinal6 argument|object argument}5 under ARC">;
-def note_ovl_candidate_bad_lvalue : Note<
+def note_ovl_candidate_bad_value_category : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
-    "expects an l-value for "
+    "expects an %select{l-value|r-value}5 for "
     "%select{%ordinal4 argument|object argument}3">;
 def note_ovl_candidate_bad_addrspace : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "cannot %select{pass pointer to|bind reference in}5 %3 "
     "%select{as a pointer to|to object in}5 %4 in %ordinal6 "
     "argument">;
 def note_ovl_candidate_bad_addrspace_this : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "

clang/lib/Sema/SemaOverload.cpp

if (!SuppressUserConversions && T2->isRecordType() &&
                                    AllowExplicit))
         return ICS;
     }
   }
 
   //     -- Otherwise, the reference shall be an lvalue reference to a
   //        non-volatile const type (i.e., cv1 shall be const), or the reference
   //        shall be an rvalue reference.
-  if (!isRValRef && (!T1.isConstQualified() || T1.isVolatileQualified()))
+  if (!isRValRef && (!T1.isConstQualified() || T1.isVolatileQualified())) {
+    if (InitCategory.isRValue() && RefRelationship != Sema::Ref_Incompatible)
+      ICS.setBad(BadConversionSequence::lvalue_ref_to_rvalue, Init, DeclType);
     return ICS;
+  }
 
   //       -- If the initializer expression
   //
   //            -- is an xvalue, class prvalue, array prvalue or function
   //               lvalue and "cv1 T1" is reference-compatible with "cv2 T2", or
   if (RefRelationship == Sema::Ref_Compatible &&
       (InitCategory.isXValue() ||
        (InitCategory.isPRValue() &&
TryReferenceInit(Sema &S, Expr *Init, QualType DeclType,
   // recursion, since TryImplicitConversion below will attempt to
   // create a temporary through the use of a copy constructor.
   if (SuppressUserConversions && RefRelationship == Sema::Ref_Incompatible &&
       (T1->isRecordType() || T2->isRecordType()))
     return ICS;
 
   // If T1 is reference-related to T2 and the reference is an rvalue
   // reference, the initializer expression shall not be an lvalue.
-  if (RefRelationship >= Sema::Ref_Related &&
-      isRValRef && Init->Classify(S.Context).isLValue())
+  if (RefRelationship >= Sema::Ref_Related && isRValRef &&
+      Init->Classify(S.Context).isLValue()) {
+    ICS.setBad(BadConversionSequence::rvalue_ref_to_lvalue, Init, DeclType);
     return ICS;
+  }
 
   // C++ [over.ics.ref]p2:
   //   When a parameter of reference type is not bound directly to
   //   an argument expression, the conversion sequence is the one
   //   required to convert the argument expression to the
   //   underlying type of the reference according to
   //   13.3.3.1. Conceptually, this conversion sequence corresponds
   //   to copy-initializing a temporary of the underlying type with
const ReferenceType *LValRefType =
             ->getAs<LValueReferenceType>();
 
     // C++ [over.ics.ref]p3:
     //   Except for an implicit object parameter, for which see 13.3.1, a
     //   standard conversion sequence cannot be formed if it requires [...]
     //   binding an rvalue reference to an lvalue other than a function
     //   lvalue.
     // Note that the function case is not possible here.
-    if (DeclType->isRValueReferenceType() && LValRefType) {
-      // FIXME: This is the wrong BadConversionSequence. The problem is binding
-      // an rvalue reference to a (non-function) lvalue, not binding an lvalue
-      // reference to an rvalue!
-      ICS.setBad(BadConversionSequence::lvalue_ref_to_rvalue, Init, DeclType);
+    if (isRValRef && LValRefType) {
+      ICS.setBad(BadConversionSequence::no_conversion, Init, DeclType);
       return ICS;
     }
 
     ICS.UserDefined.After.ReferenceBinding = true;
     ICS.UserDefined.After.IsLvalueReference = !isRValRef;
     ICS.UserDefined.After.BindsToFunctionLvalue = false;
     ICS.UserDefined.After.BindsToRvalue = !LValRefType;
     ICS.UserDefined.After.BindsImplicitObjectArgumentWithoutRefQualifier = false;
if (FromQs.hasUnaligned() != ToQs.hasUnaligned()) {
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
           << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
           << FromQs.hasUnaligned() << I + 1;
       MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
       return;
     }
 
     unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
-    assert(CVR && "unexpected qualifiers mismatch");
+    assert(CVR && "expected qualifiers mismatch");
 
     if (isObjectArgument) {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_cvr_this)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
           << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
           << (CVR - 1);
     } else {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_cvr)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
           << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
           << (CVR - 1) << I + 1;
     }
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
+  if (Conv.Bad.Kind == BadConversionSequence::lvalue_ref_to_rvalue ||
+      Conv.Bad.Kind == BadConversionSequence::rvalue_ref_to_lvalue) {
+    S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_value_category)
+        << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
+        << (unsigned)isObjectArgument << I + 1
+        << (Conv.Bad.Kind == BadConversionSequence::rvalue_ref_to_lvalue)
+        << (FromExpr ? FromExpr->getSourceRange() : SourceRange());
+    MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
+    return;
+  }
+
   // Special diagnostic for failure to convert an initializer list, since
   // telling the user that it has type void is not useful.
   if (FromExpr && isa<InitListExpr>(FromExpr)) {
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_list_argument)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
         << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
         << ToTy << (unsigned)isObjectArgument << I + 1;
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
if (const ObjCObjectPointerType *ToPtrTy
               FromIface->isSuperClassOf(ToIface))
             BaseToDerivedConversion = 2;
   } else if (const ReferenceType *ToRefTy = ToTy->getAs<ReferenceType>()) {
     if (ToRefTy->getPointeeType().isAtLeastAsQualifiedAs(FromTy) &&
         !FromTy->isIncompleteType() &&
         !ToRefTy->getPointeeType()->isIncompleteType() &&
         S.IsDerivedFrom(SourceLocation(), ToRefTy->getPointeeType(), FromTy)) {
       BaseToDerivedConversion = 3;
-    } else if (ToTy->isLValueReferenceType() && !FromExpr->isLValue() &&
-               ToTy.getNonReferenceType().getCanonicalType() ==
-               FromTy.getNonReferenceType().getCanonicalType()) {
-      S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_lvalue)
-          << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-          << (unsigned)isObjectArgument << I + 1
-          << (FromExpr ? FromExpr->getSourceRange() : SourceRange());
-      MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
-      return;
     }
   }
 
   if (BaseToDerivedConversion) {
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_base_to_derived_conv)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
         << (FromExpr ? FromExpr->getSourceRange() : SourceRange())
         << (BaseToDerivedConversion - 1) << FromTy << ToTy << I + 1;

clang/test/CXX/drs/dr14xx.cpp

namespace dr1467 {  // dr1467: 3.7 c++11
   }
   } // namespace NonAmbiguous
 
 #if __cplusplus >= 201103L
   namespace StringLiterals {
   // When the array size is 4 the call will attempt to bind an lvalue to an
   // rvalue and fail. Therefore #2 will be called. (rsmith will bring this
   // issue to CWG)
-  void f(const char(&&)[4]);              // expected-note 5 {{no known conversion}}
+  void f(const char(&&)[4]);              // expected-note 2 {{expects an r-value}} expected-note 3 {{no known conversion}}
   void f(const char(&&)[5]) = delete;     // expected-note 2 {{candidate function has been explicitly deleted}} expected-note 3 {{no known conversion}}
-  void f(const wchar_t(&&)[4]);           // expected-note 5 {{no known conversion}}
+  void f(const wchar_t(&&)[4]);           // expected-note {{expects an r-value}} expected-note 4 {{no known conversion}}
   void f(const wchar_t(&&)[5]) = delete;  // expected-note {{candidate function has been explicitly deleted}} expected-note 4 {{no known conversion}}
 #if __cplusplus >= 202002L
-  void f2(const char8_t(&&)[4]);          // expected-note {{no known conversion}}
+  void f2(const char8_t(&&)[4]);          // expected-note {{expects an r-value}}
   void f2(const char8_t(&&)[5]) = delete; // expected-note {{candidate function has been explicitly deleted}}
 #endif
-  void f(const char16_t(&&)[4]);          // expected-note 5 {{no known conversion}}
+  void f(const char16_t(&&)[4]);          // expected-note {{expects an r-value}} expected-note 4 {{no known conversion}}
   void f(const char16_t(&&)[5]) = delete; // expected-note {{candidate function has been explicitly deleted}} expected-note 4 {{no known conversion}}
-  void f(const char32_t(&&)[4]);          // expected-note 5 {{no known conversion}}
+  void f(const char32_t(&&)[4]);          // expected-note {{expects an r-value}} expected-note 4 {{no known conversion}}
   void f(const char32_t(&&)[5]) = delete; // expected-note {{candidate function has been explicitly deleted}} expected-note 4 {{no known conversion}}
   void g() {
     f({"abc"});       // expected-error {{call to deleted function 'f'}}
     f({((("abc")))}); // expected-error {{call to deleted function 'f'}}
     f({L"abc"});      // expected-error {{call to deleted function 'f'}}
 #if __cplusplus >= 202002L
     f2({u8"abc"});    // expected-error {{call to deleted function 'f2'}}
 #endif

clang/test/CXX/drs/dr1xx.cpp

struct A {
     A(const B &); // expected-note 0-1{{not viable: no known conversion from 'dr177::A' to}}
   };
   B b;
   A a = b;
 #if __cplusplus <= 201402L
   // expected-error@-2 {{no viable constructor copying variable}}
 #endif
 
-  struct C { C(C&); }; // expected-note {{not viable: no known conversion from 'dr177::D' to 'dr177::C &'}}
+  struct C { C(C&); }; // expected-note {{not viable: expects an l-value for 1st argument}}
   struct D : C {};
   struct E { operator D(); };
   E e;
   C c = e; // expected-error {{no viable constructor copying variable of type 'dr177::D'}}
 }
 
 namespace dr178 { // dr178: yes
   int check[int() == 0 ? 1 : -1];

clang/test/CXX/drs/dr6xx.cpp

 }
 
 // dr604: na
 // dr605 needs IRGen test
 
 namespace dr606 { // dr606: yes
 #if __cplusplus >= 201103L
   template<typename T> struct S {};
-  template<typename T> void f(S<T> &&); // expected-note {{no known conversion from 'S<int>' to 'S<int> &&'}}
+  template<typename T> void f(S<T> &&); // expected-note {{expects an r-value}}
   template<typename T> void g(T &&);
-  template<typename T> void h(const T &&); // expected-note {{no known conversion from 'S<int>' to 'const dr606::S<int> &&'}}
+  template<typename T> void h(const T &&); // expected-note {{expects an r-value}}
 
   void test(S<int> s) {
     f(s); // expected-error {{no match}}
     g(s);
     h(s); // expected-error {{no match}}
 
     g(test);
     h(test); // ok, an rvalue reference can bind to a function lvalue

clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -std=c++1z -fsyntax-only -verify %s
 
 // A forwarding reference is an rvalue reference to a cv-unqualified template
 // parameter that does not represent a template parameter of a class template.
 #if __cplusplus > 201402L
 namespace ClassTemplateParamNotForwardingRef {
   // This is not a forwarding reference.
   template<typename T> struct A { // expected-note {{candidate}}
-    A(T&&); // expected-note {{no known conversion from 'int' to 'int &&'}}
+    A(T&&); // expected-note {{expects an r-value}}
   };
   int n;
   A a = n; // expected-error {{no viable constructor or deduction guide}}
 
   A b = 0;
   A<int> *pb = &b;
 
   // This is a forwarding reference.
void test_f0() {
   X<int> xi0 = f0(prvalue<int>());
   X<int> xi1 = f0(xvalue<int>());
   X<int&> xi2 = f0(lvalue<int>());
   X<Y> xy0 = f0(prvalue<Y>());
   X<Y> xy1 = f0(xvalue<Y>());
   X<Y&> xy2 = f0(lvalue<Y>());
 }
 
-template<typename T> X<T> f1(const T&&); // expected-note{{candidate function [with T = int] not viable: no known conversion from 'int' to 'const int &&' for 1st argument}} \
-// expected-note{{candidate function [with T = Y] not viable: no known conversion from 'Y' to 'const Y &&' for 1st argument}}
+template<typename T> X<T> f1(const T&&); // expected-note{{candidate function [with T = int] not viable: expects an r-value for 1st argument}} \
+// expected-note{{candidate function [with T = Y] not viable: expects an r-value for 1st argument}}
 
 void test_f1() {
   X<int> xi0 = f1(prvalue<int>());
   X<int> xi1 = f1(xvalue<int>());
   f1(lvalue<int>()); // expected-error{{no matching function for call to 'f1'}}
   X<Y> xy0 = f1(prvalue<Y>());
   X<Y> xy1 = f1(xvalue<Y>());
   f1(lvalue<Y>()); // expected-error{{no matching function for call to 'f1'}}
 }
 
 namespace std_example {
   template <class T> int f(T&&); 
-  template <class T> int g(const T&&); // expected-note{{candidate function [with T = int] not viable: no known conversion from 'int' to 'const int &&' for 1st argument}}
+  template <class T> int g(const T&&); // expected-note{{candidate function [with T = int] not viable: expects an r-value for 1st argument}}
 
   int i;
   int n1 = f(i);
   int n2 = f(0);
   int n3 = g(i); // expected-error{{no matching function for call to 'g'}}
 
 #if __cplusplus > 201402L
   template<class T> struct A { // expected-note {{candidate}}
     template<class U>
-    A(T &&, U &&, int *); // expected-note {{[with T = int, U = int] not viable: no known conversion from 'int' to 'int &&'}}
+    A(T &&, U &&, int *); // expected-note {{[with T = int, U = int] not viable: expects an r-value}}
     A(T &&, int *);       // expected-note {{requires 2}}
   };
   template<class T> A(T &&, int *) -> A<T>; // expected-note {{requires 2}}
 
   int *ip;
   A a{i, 0, ip};  // expected-error {{no viable constructor or deduction guide}}
   A a0{0, 0, ip};
   A a2{i, ip};
 
   A<int> &a0r = a0;
   A<int&> &a2r = a2;
 #endif
 }

clang/test/SemaCXX/overload-member-call.cpp

class A {
     void foo(int n, const char *s, int t, int u = 0); // expected-note {{candidate function not viable: requires at least 3 arguments, but 2 were provided}}
 
     void bar(double d); //expected-note {{candidate function not viable: 'this' argument has type 'const test1::A', but method is not marked const}}
     void bar(int i); //expected-note {{candidate function not viable: 'this' argument has type 'const test1::A', but method is not marked const}}
 
     void baz(A &d); // expected-note {{candidate function not viable: 1st argument ('const test1::A') would lose const qualifier}}
     void baz(int i); // expected-note {{candidate function not viable: no known conversion from 'const test1::A' to 'int' for 1st argument}} 
 
+    void ref() &&;   // expected-note {{expects an r-value for object argument}} expected-note {{requires 0 arguments, but 1 was provided}}
+    void ref(int) &; // expected-note {{expects an l-value for object argument}} expected-note {{requires 1 argument, but 0 were provided}}
+
     // PR 11857
     void foo(int n); // expected-note {{candidate function not viable: requires single argument 'n', but 2 arguments were provided}}
     void foo(unsigned n = 10); // expected-note {{candidate function not viable: allows at most single argument 'n', but 2 arguments were provided}}
     void rab(double n, int u = 0); // expected-note {{candidate function not viable: requires at least argument 'n', but no arguments were provided}}
     void rab(int n, int u = 0); // expected-note {{candidate function not viable: requires at least argument 'n', but no arguments were provided}}
     void zab(double n = 0.0, int u = 0); // expected-note {{candidate function not viable: requires at most 2 arguments, but 3 were provided}}
     void zab(int n = 0, int u = 0); // expected-note {{candidate function not viable: requires at most 2 arguments, but 3 were provided}}
   };
 
   void test() {
     A a;
     a.foo(4, "hello"); //expected-error {{no matching member function for call to 'foo'}}
 
     const A b = A();
     b.bar(0); //expected-error {{no matching member function for call to 'bar'}}
 
     a.baz(b); //expected-error {{no matching member function for call to 'baz'}}
 
     a.rab(); //expected-error {{no matching member function for call to 'rab'}}
     a.zab(3, 4, 5); //expected-error {{no matching member function for call to 'zab'}}
+
+    a.ref();    // expected-error {{no matching member function for call to 'ref'}}
+    A().ref(1); // expected-error {{no matching member function for call to 'ref'}}
   }
 }
 
 namespace b7398190 {
   struct S {
     int f(); // expected-note {{'this' argument has type 'const b7398190::S', but method is not marked const}}
     void f(int); // expected-note {{requires 1 argument, but 0 were provided}}
   };

clang/test/SemaCXX/rval-references-examples.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
 
 template<typename T>
 class unique_ptr {
   T *ptr;
 
   unique_ptr(const unique_ptr&) = delete; // expected-note 3{{'unique_ptr' has been explicitly marked deleted here}}
   unique_ptr &operator=(const unique_ptr&) = delete; // expected-note{{candidate function has been explicitly deleted}}
 public:
   unique_ptr() : ptr(0) { }
   unique_ptr(unique_ptr &&other) : ptr(other.ptr) { other.ptr = 0; }
   explicit unique_ptr(T *ptr) : ptr(ptr) { }
 
   ~unique_ptr() { delete ptr; }
 
-  unique_ptr &operator=(unique_ptr &&other) { // expected-note{{candidate function not viable: no known conversion from 'unique_ptr<int>' to 'unique_ptr<int> &&' for 1st argument}}
+  unique_ptr &operator=(unique_ptr &&other) { // expected-note{{candidate function not viable: expects an r-value for 1st argument}}
     if (this == &other)
       return *this;
 
     delete ptr;
     ptr = other.ptr;
     other.ptr = 0;
     return *this;
   }