[Sema] Improve diagnostics for const- and ref-qualified member functions

[jtbandes]

Summary

Adjust wording for const-qualification mismatch to be a little more clear.

Also add another diagnostic for a ref qualifier mismatch, which previously produced a useless error (this error path is simply very old; see rL119336):

Before:

error: cannot initialize object parameter of type 'X0' with an expression of type 'X0'

After:

error: 'this' argument to member function 'rvalue' is an lvalue, but function has rvalue ref-qualifier

[aaron.ballman]

I think the new wording is a bit more clear, in general.

include/clang/Basic/DiagnosticSemaKinds.td

1595–1597

I don't think this diagnostic needs to be moved and renamed; the old name was more clear than "other" on the new name.

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

22–24

Can you add examples that cover the other diagnostic wordings as well (volatile, restrict, combinations, etc)?

[jtbandes]

include/clang/Basic/DiagnosticSemaKinds.td

1595–1597

How about _bad_type? I was hoping to give these all similar names since they happen during the same implicit conversion. Also, while "implicit object parameter" is technically a correct name, it seems to be not very commonly used and would confuse users. Though it's also valuable for the diagnostic id to be similar to the actual text...

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

1595–1597

I'm not too worried about the identifier used for the diagnostic; it doesn't seem likely to confuse many people working on Clang (or at least, not confuse them for long). However, I wouldn't be opposed to err_member_function_call_bad_type.

[jtbandes]

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

22–24

I've been working on this, but I actually can't trigger the restrict variants. Do you know whether this is something that's expected to work? The implicit object param doesn't seem to retain its restrict-ness (full disclosure, I have almost no prior experience with restrict...):

void c() const;
void v() volatile;
void r() __restrict__;
void cr() const __restrict__;
void cv() const volatile;
void vr() volatile __restrict__;
void cvr() const volatile __restrict__;

void test_diagnostics(const volatile X0 &__restrict__ cvr) {
  cvr.g(); // expected-error {{not marked const, volatile, or restrict}}  -- actually produces "not marked const or volatile"
  cvr.c(); // expected-error {{not marked volatile or restrict}}  -- actually produces "not marked volatile"
  cvr.v(); // expected-error {{not marked const or restrict}}  -- actually produces "not marked const"
  cvr.r(); // expected-error {{not marked const or volatile}}
  cvr.cr(); // expected-error {{not marked volatile}}
  cvr.cv(); // expected-error {{not marked restrict}}  -- actually produces no error
  cvr.vr(); // expected-error {{not marked const}}
  cvr.cvr();
}

[aaron.ballman]

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

22–24

Given that restrict is a Cism, it's entirely possible that it's not really supported as a member function qualifier. In that case, one more test for const volatile should be sufficient.

[lebedev.ri]

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

22–24

Note that you might want to test it regardless, to not miss if it starts to warn for some reason.

[jtbandes]

• feedback from review & more tests

[aaron.ballman]

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

89

You should spell out the entire diagnostic at least the first time it appears in the file.

[jtbandes]

• spell out full diagnostic the first time

[aaron.ballman]

This LGTM, but you should wait for a few days before committing in case @rsmith has comments.

[jtbandes]

Thanks, will do. Is there an automated system that can run all the tests before I merge rather than waiting for a potential build failure after the fact?

[aaron.ballman]

In D39937#929514, @jtbandes wrote:

Thanks, will do. Is there an automated system that can run all the tests before I merge rather than waiting for a potential build failure after the fact?

You can run the tests locally using llvm-lit, which will get you some coverage for your platform and architecture, but that's about the best you can do locally.

https://llvm.org/docs/TestingGuide.html#id1

[jtbandes]

Bump :)

[aaron.ballman]

You're set to commit (if there are concerns, @rsmith can raise them post-commit). Do you need me to commit on your behalf?

[jtbandes]

After merging the buildbots informed me some other tests were broken that I failed to notice. I reverted this commit and submitted D41646 which reintroduces the changes and fixes the other broken tests.

[aaronpuchert]

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

22–24

I was able to trigger the error with __restrict:

struct A { void f(); };
struct B
{
	void g() __restrict { return a.f(); }
	//                           ^
	// error: 'this' argument to member function 'f' has type
	//   '__restrict A', but function is not marked restrict
	A a;
};

Discovered this when I tried to optimize some code. Unfortunately this doesn't do what I wanted, since qualifiers on a function apply to the pointee of this, not this itself, which has const cv-qualifier. Converting the implicit this into an explicit argument we get:

void Bg(__restrict B *const); // error: restrict requires a pointer or reference ('B' is invalid)

so my original code should also be rejected by the parser. Indeed a type __restrict A can't exist. And if the parser does that, the restrict part is probably just dead: the pointee of this can't be restrict since it's a class type, and never a pointer type.

include/clang/Basic/DiagnosticSemaKinds.td

def err_not_integral_type_anon_bitfield : Error<
   "anonymous bit-field has non-integral type %0">;
 def err_member_function_initialization : Error<
   "initializer on function does not look like a pure-specifier">;
 def err_non_virtual_pure : Error<
   "%0 is not virtual and cannot be declared pure">;
 def ext_pure_function_definition : ExtWarn<
   "function definition with pure-specifier is a Microsoft extension">,
   InGroup<MicrosoftPureDefinition>;
-def err_implicit_object_parameter_init : Error<
-  "cannot initialize object parameter of type %0 with an expression "
-  "of type %1">;
 def err_qualified_member_of_unrelated : Error<
   "%q0 is not a member of class %1">;
 
+def err_member_function_call_bad_ref : Error<
+  "'this' argument to member function %0 is an %select{lvalue|rvalue}1, "
+  "but function has %select{non-const lvalue|rvalue}2 ref-qualifier">;
+def err_member_function_call_bad_cvr : Error<
+  "'this' argument to member function %0 has type %1, but function is not marked "
+  "%select{const|restrict|const or restrict|volatile|const or volatile|"
+  "volatile or restrict|const, volatile, or restrict}2">;
+def err_member_function_call_other : Error<
+  "cannot initialize object parameter of type %0 with an expression "
+  "of type %1">;
+
 def warn_call_to_pure_virtual_member_function_from_ctor_dtor : Warning<
   "call to pure virtual member function %0 has undefined behavior; "
   "overrides of %0 in subclasses are not available in the "
   "%select{constructor|destructor}1 of %2">;
 
 def note_member_declared_at : Note<"member is declared here">;
 def note_ivar_decl : Note<"instance variable is declared here">;
 def note_bitfield_decl : Note<"bit-field is declared here">;
def err_reference_bind_init_list : Error<
   "reference to type %0 cannot bind to an initializer list">;
 def warn_temporary_array_to_pointer_decay : Warning<
   "pointer is initialized by a temporary array, which will be destroyed at the "
   "end of the full-expression">,
   InGroup<DiagGroup<"address-of-array-temporary">>;
 def err_init_list_bad_dest_type : Error<
   "%select{|non-aggregate }0type %1 cannot be initialized with an initializer "
   "list">;
-def err_member_function_call_bad_cvr : Error<"member function %0 not viable: "
-    "'this' argument has type %1, but function is not marked "
-    "%select{const|restrict|const or restrict|volatile|const or volatile|"
-    "volatile or restrict|const, volatile, or restrict}2">;
 
 def err_reference_bind_to_bitfield : Error<
   "%select{non-const|volatile}0 reference cannot bind to "
   "bit-field%select{| %1}2">;
 def err_reference_bind_to_vector_element : Error<
   "%select{non-const|volatile}0 reference cannot bind to vector element">;
 def err_reference_var_requires_init : Error<
   "declaration of reference variable %0 requires an initializer">;

lib/Sema/SemaOverload.cpp

Sema::PerformObjectArgumentInitialization(Expr *From,
   }
 
   // Note that we always use the true parent context when performing
   // the actual argument initialization.
   ImplicitConversionSequence ICS = TryObjectArgumentInitialization(
       *this, From->getLocStart(), From->getType(), FromClassification, Method,
       Method->getParent());
   if (ICS.isBad()) {
-    if (ICS.Bad.Kind == BadConversionSequence::bad_qualifiers) {
+    switch (ICS.Bad.Kind) {
+    case BadConversionSequence::bad_qualifiers: {
       Qualifiers FromQs = FromRecordType.getQualifiers();
       Qualifiers ToQs = DestType.getQualifiers();
       unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
       if (CVR) {
         Diag(From->getLocStart(),
              diag::err_member_function_call_bad_cvr)
           << Method->getDeclName() << FromRecordType << (CVR - 1)
           << From->getSourceRange();
         Diag(Method->getLocation(), diag::note_previous_decl)
           << Method->getDeclName();
         return ExprError();
       }
+      break;
+    }
+
+    case BadConversionSequence::lvalue_ref_to_rvalue:
+    case BadConversionSequence::rvalue_ref_to_lvalue: {
+      bool IsRValueQualified =
+        Method->getRefQualifier() == RefQualifierKind::RQ_RValue;
+      Diag(From->getLocStart(), diag::err_member_function_call_bad_ref)
+        << Method->getDeclName() << FromClassification.isRValue()
+        << IsRValueQualified;
+      Diag(Method->getLocation(), diag::note_previous_decl)
+        << Method->getDeclName();
+      return ExprError();
+    }
+
+    default:
+      break;
     }
 
     return Diag(From->getLocStart(),
-                diag::err_implicit_object_parameter_init)
+                diag::err_member_function_call_other)
        << ImplicitParamRecordType << FromRecordType << From->getSourceRange();
   }
 
   if (ICS.Standard.Second == ICK_Derived_To_Base) {
     ExprResult FromRes =
       PerformObjectMemberConversion(From, Qualifier, FoundDecl, Method);
     if (FromRes.isInvalid())
       return ExprError();

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 template<typename T> T &lvalue();
 template<typename T> T &&xvalue();
 template<typename T> T prvalue();
 
 struct X0 {
   int &f() &;
   float &f() &&;
 
   template<typename T> int &ft(T) &;
   template<typename T> float &ft(T) &&;
 
   typedef int &(*func_int_ref)();
   typedef float &(*func_float_ref)();
 
   operator func_int_ref() &;
   operator func_float_ref() &&;
 
   void g();
 
+  void lvalue() &; // expected-note 2 {{'lvalue' declared here}}
+  void const_lvalue() const&;
+  void rvalue() &&; // expected-note {{'rvalue' declared here}}
+
   int &operator+(const X0&) &;
   float &operator+(const X0&) &&;
 
   template<typename T> int &operator+(const T&) &;
   template<typename T> float &operator+(const T&) &&;
 
   int &h() const&;
   float &h() &&;
   int &h2() const&;
   float &h2() const&&;
 };
 
 void X0::g() {
   int &ir1 = f();
   int &ir2 = X0::f();
 }
 
 void test_ref_qualifier_binding() {
   int &ir1 = lvalue<X0>().f();
   float &fr1 = xvalue<X0>().f();
   float &fr2 = prvalue<X0>().f();
   int &ir2 = lvalue<X0>().ft(1);
   float &fr3 = xvalue<X0>().ft(2);
   float &fr4 = prvalue<X0>().ft(3);
+
+  lvalue<X0>().lvalue();
+  lvalue<X0>().const_lvalue();
+  lvalue<X0>().rvalue(); // expected-error {{'this' argument to member function 'rvalue' is an lvalue, but function has rvalue ref-qualifier}}
+
+  xvalue<X0>().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  xvalue<X0>().const_lvalue();
+  xvalue<X0>().rvalue();
+
+  prvalue<X0>().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  prvalue<X0>().const_lvalue();
+  prvalue<X0>().rvalue();
 }
 
 void test_ref_qualifier_binding_with_surrogates() {
   int &ir1 = lvalue<X0>()();
   float &fr1 = xvalue<X0>()();
   float &fr2 = prvalue<X0>()();
 }
 
 void test_ref_qualifier_binding_operators() {
   int &ir1 = lvalue<X0>() + prvalue<X0>();
   float &fr1 = xvalue<X0>() + prvalue<X0>();
   float &fr2 = prvalue<X0>() + prvalue<X0>();
   int &ir2 = lvalue<X0>() + 1;
   float &fr3 = xvalue<X0>() + 2;
   float &fr4 = prvalue<X0>() + 3;
 }
 
 void test_ref_qualifier_overloading() {
   int &ir1 = lvalue<X0>().h();
   float &fr1 = xvalue<X0>().h();
   float &fr2 = prvalue<X0>().h();
   int &ir2 = lvalue<X0>().h2();
   float &fr3 = xvalue<X0>().h2();
   float &fr4 = prvalue<X0>().h2();
 }

test/SemaCXX/copy-initialization.cpp

void f(Y y, int *ip, float *fp) {
   X x4a(fp);
 }
 
 struct foo {
  void bar(); // expected-note{{declared here}}
 };
 
 // PR3600
-void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this' argument has type 'const foo', but function is not marked const}}
+void test(const foo *P) { P->bar(); } // expected-error{{'this' argument to member function 'bar' has type 'const foo', but function is not marked const}}
 
 namespace PR6757 {
   struct Foo {
     Foo(); // expected-note{{not viable}}
     Foo(Foo&); // expected-note{{candidate constructor not viable}}
   };
 
   struct Bar {