[clang] Improve diagnostics on implicitly deleted defaulted comparisons

[mizvekov]

Summary

This patch just makes the error message clearer by reinforcing the cause
was a lack of viable three-way comparison function for the
complete object.

Signed-off-by: Matheus Izvekov <[e-mail]>

[rsmith]

I don't think this change is right: for

struct A {
  int &r;
  bool operator==(const A&) const = default;
};

we should warn about the reference member (as we do), but for

struct A {
  int &r;
  bool operator<(const A&) const = default;
};

... we shouldn't recurse to subobjects because they make no difference. The problem in that example is that there's no operator<=> for A; the fact that there's a reference member is irrelevant. I think it would be useful to change the diagnostic from

<stdin>:3:8: warning: explicitly defaulted relational comparison operator is implicitly deleted [-Wdefaulted-function-deleted]
  bool operator<(const A &) const = default;
       ^
<stdin>:3:8: note: defaulted 'operator<' is implicitly deleted because there is no viable comparison function

to something more verbose, such as

<stdin>:3:8: warning: explicitly defaulted relational comparison operator is implicitly deleted [-Wdefaulted-function-deleted]
  bool operator<(const A &) const = default;
       ^
<stdin>:3:8: note: defaulted 'operator<' is implicitly deleted because there is no viable three-way comparison function for 'A'

[mizvekov]

Now this a patch just about improving the diagnostic as rsmith suggested.

[mizvekov]

You are absolutely right, my bad!!

So we stealth fix this revision to improve the message :D

[mizvekov]

Change to simpler way to do it:
For CompleteObject Kind, just fill in the declaration for the complete object.

[rsmith]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8968–8970 ↗

(On Diff #329289)

Would it be useful to apply the same diagnostic improvement to this diagnostic too? (Genuine question: I *think* we'll attach a note pointing to the deleted function in this case, which would probably make the "for T" part just be noise, but I've not checked.)

[mizvekov]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8968–8970 ↗

(On Diff #329289)

Yeah I thought about checking other errors for similar improvements. It's on my list to check that.

[mizvekov]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8968–8970 ↗

(On Diff #329289)

For this one in particular, we really do attach another note that makes it obvious, pointing to the explicitly marked deleted function.
note_defaulted_comparison_not_constexpr at first glance looks like might suffer from the same problem, but in this case I don't think the error ever could apply to the complete object.
And then nothing else that I could find.

clang/lib/Sema/SemaDeclCXX.cpp

case OR_No_Viable_Function:
         // '==' and a usable '<'.
         if (!R.add(visitBinaryOperator(OO_EqualEqual, Args, Subobj,
                                        &CandidateSet)))
           R.add(visitBinaryOperator(OO_Less, Args, Subobj, &CandidateSet));
         break;
       }
 
       if (Diagnose == ExplainDeleted) {
+        if (Subobj.Kind == DefaultedComparisonSubobject::CompleteObject) {
+          QualType ParamLvalType =
+              FD->getParamDecl(0)->getType().getNonReferenceType();
+          getDerived().visitSubobjects(R, RD, ParamLvalType.getQualifiers());
+          R = Result::deleted();
+          break;
+        }
         S.Diag(Subobj.Loc, diag::note_defaulted_comparison_no_viable_function)
             << FD << Subobj.Kind << Subobj.Decl;
 
         // For a three-way comparison, list both the candidates for the
         // original operator and the candidates for the synthesized operator.
         if (SpaceshipCandidates) {
           SpaceshipCandidates->NoteCandidates(
               S, Args,

clang/test/CXX/class/class.compare/class.compare.default/p2.cpp

void f(A2 a) {
   void(a <=> a);
   void(a < a);
   void(a <= a);
   void(a > a);
   void(a >= a);
 }
 
 struct A3 {
-  int &x; // expected-note {{because class 'A3' has a reference member}}
+  int &x; // expected-note 2{{because class 'A3' has a reference member}}
 
   bool operator==(const A3 &) const = default; // expected-warning {{implicitly deleted}}
   bool operator<(const A3 &) const = default;  // expected-warning {{implicitly deleted}}
-  // expected-note@-1 {{because there is no viable comparison function}}
 };
 
 struct B1 {
   struct {
     int x;
     int &y; // expected-note 2{{because class 'B1' has a reference member}}
   };
 

clang/test/CXX/class/class.compare/class.eq/p2.cpp

struct E {
   int operator==(E) const; // expected-note {{passing}}
 };
 struct F { void operator==(F) const; };
 struct G { bool operator==(G) const = delete; }; // expected-note {{deleted here}}
 
 struct H1 {
   bool operator==(const H1 &) const = default;
   bool operator<(const H1 &) const = default; // expected-warning {{implicitly deleted}}
-  // expected-note@-1 {{because there is no viable comparison function}}
-  void (*x)();
+  void (*x)(); // expected-note {{because there is no viable comparison function for member 'x'}}
 };
 struct H2 {
   bool operator==(const H2 &) const = default;
   bool operator<(const H2 &) const = default; // expected-warning {{implicitly deleted}}
-  // expected-note@-1 {{because there is no viable comparison function}}
-  void (H2::*x)();
+  void (H2::*x)(); // expected-note {{because there is no viable comparison function for member 'x'}}
 };
 struct H3 {
   bool operator==(const H3 &) const = default;
   bool operator<(const H3 &) const = default; // expected-warning {{implicitly deleted}}
-  // expected-note@-1 {{because there is no viable comparison function}}
-  int H3::*x;
+  int H3::*x; // expected-note {{because there is no viable comparison function for member 'x'}}
 };
 
 template<typename T> struct X {
   X();
   bool operator==(const X&) const = default; // #x expected-note 4{{deleted here}}
   T t; // expected-note 3{{because there is no viable comparison function for member 't'}}
        // expected-note@-1 {{because it would invoke a deleted comparison function for member 't'}}
 };