[clang] implicitly delete space ship operator with function pointers

[mizvekov]

Summary

See bug #48856

Definitions of classes with member function pointers and default
spaceship operator were getting accepted with no diagnostic on
release build, and triggering assert on builds with runtime checks
enabled. Diagnostics were only produced when actually comparing
instances of such classes.

This patch makes it so Spaceship and Less operators are not considered
as builtin operator candidates for function pointers, producing
equivalent diagnostics for the cases where pointers to member function
and pointers to data members are used instead.

[mizvekov]

added tests

adds test that will catch original bug
also adds some tests on default equality/relational operators

this last one would have caught the issue of not accepting equality
operator for function pointers, had I not remembered about it :)

[mizvekov]

Ping.

[mizvekov]

Ping.

By the way I realize I may not have put the new tests in the appropriate place. Any guidance there welcome.

[rsmith]

Thanks, this looks fine. I wish we had a better way to determine whether a builtin comparison is usable without actually building a use of it. :-(

In D95409#2565227, @mizvekov wrote:

By the way I realize I may not have put the new tests in the appropriate place. Any guidance there welcome.

I've suggested better locations.

clang/lib/Sema/SemaDeclCXX.cpp

7688–7690

Please can you extend the FIXME comment below to call out that relational comparisons on a function pointer is the one known case where there is a builtin candidate that can't actually be used?

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

1 ↗

(On Diff #319400)

Hm, I don't think this is the right place for this test. The A test is a test for [class.compare.default]p2 (which disallows reference members). The B, C, and D tests are for [class.eq]p2 (which requires xi == xi to be a usable expression). Can you move the tests to those places?

clang/test/CXX/class/class.compare/class.spaceship/p4.cpp

19 ↗

(On Diff #319400)

Similar to above, this is [class.compare.default]p2.

34 ↗

(On Diff #319400)

... and these are [class.spaceship]p2 ("The operator function is defined as deleted if that expression is not usable")

[mizvekov]

Updated comment and moved tests to correct places.

clang/lib/Sema/SemaDeclCXX.cpp

visitBinaryOperator(OverloadedOperatorKind OO, ArrayRef<Expr *> Args,
 
     /// C++2a [class.compare.default]p1 [P2002R0]:
     ///   [...] the defaulted function itself is never a candidate for overload
     ///   resolution [...]
     CandidateSet.exclude(FD);
 
     if (Args[0]->getType()->isOverloadableType())
       S.LookupOverloadedBinOp(CandidateSet, OO, Fns, Args);
-    else {
+    else if (OO == OO_EqualEqual ||
+             !Args[0]->getType()->isFunctionPointerType()) {
       // FIXME: We determine whether this is a valid expression by checking to
       // see if there's a viable builtin operator candidate for it. That isn't
       // really what the rules ask us to do, but should give the right results.
+      //
+      // Note that the builtin operator for relational comparisons on function
+      // pointers is the only known case which cannot be used.
       S.AddBuiltinOperatorCandidates(OO, FD->getLocation(), Args, CandidateSet);
     }
 
     Result R;
 
     OverloadCandidateSet::iterator Best;
     switch (CandidateSet.BestViableFunction(S, FD->getLocation(), Best)) {
     case OR_Success: {

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);
   void(a >= a);
 }
 
+struct A3 {
+  int &x; // expected-note {{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}}
   };
 
   bool operator==(const B1&) const = default; // expected-warning {{implicitly deleted}}
   bool operator<=>(const B1&) const = default; // expected-warning {{implicitly deleted}}

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

 };
 struct E {
   E(const E &) = delete; // expected-note {{deleted}}
   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)();
+};
+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)();
+};
+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;
+};
+
 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'}}
 };
 
 struct Mutable {

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

namespace BadDeducedType {
   };
 
   template<typename T> concept CmpCat = true;
   struct D {
     // expected-error@+1 {{deduced return type for defaulted three-way comparison operator must be 'auto', not 'CmpCat auto'}}
     friend CmpCat auto operator<=>(const D&, const D&) = default;
   };
 }
+
+namespace PR48856 {
+  struct A {
+    auto operator<=>(const A &) const = default; // expected-warning {{implicitly deleted}}
+    void (*x)();                                 // expected-note {{because there is no viable comparison function for member 'x'}}
+  };
+
+  struct B {
+    auto operator<=>(const B &) const = default; // expected-warning {{implicitly deleted}}
+    void (B::*x)();                              // expected-note {{because there is no viable comparison function for member 'x'}}
+  };
+
+  struct C {
+    auto operator<=>(const C &) const = default; // expected-warning {{implicitly deleted}}
+    int C::*x;                                   // expected-note {{because there is no viable comparison function for member 'x'}}
+  };
+}