[clang] Exclude function pointers on DefaultedComparisonAnalyzer

[mizvekov]

Summary

This implements a more comprehensive fix than was done at D95409.
Instead of excluding just function pointer subobjects, we also
exclude any user-defined function pointer conversion operators.

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

[rsmith]

I think we need to take this to the C++ committee. For example, consider a case like:

struct A {
  using T1 = void(*)();
  using T2 = int;
  operator T1();
  operator T2();
};
struct B {
  A a;
  friend auto operator<=>(B, B) = default;
};

Under the standard rules, the operator<=> is deleted due to ambiguity, but with this change we'd unambiguously pick the conversion to T2. The old behavior here better matches the current standard wording -- in particular, the special case is in [class.compare.default]/3.2 and only applies when neither operand is of overloadable type -- but perhaps the right approach here would be to fix the language rules so that operator<=> is only provided for object pointer types, not for all pointer types, in general.

[mizvekov]

I see. I can change this patch so it just implicitly deletes the operator in case we decide to pick function pointers, without the hard error introduced by the previous patch.
And if the comitee decides to stop providing relational operators for function pointers, we can revisit this.

[mizvekov]

Instead of excluding function pointers from the overload set, just implicitly delete
with new diagnostic in case we end up selecting it.

[rsmith]

Thanks, let's go with this for now.

Discussion in WG21 seems to be heading towards removing the builtin candidates for operator<=>, operator<, operator<=, operator>, and operator>= for function pointers, but we'll probably not have a firm decision on that for quite a while.

[mizvekov]

rebase.

[lhames]

Disregard 80f30a6b855b: I messed up a copy-paste of a commit message. That was for https://reviews.llvm.org/D104480.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_defaulted_comparison_cannot_deduce_undeduced_auto : Error<
   "return type of defaulted 'operator<=>' cannot be deduced because "
   "three-way comparison for %select{|member|base class}0 %1 "
   "has a deduced return type and is not yet defined">;
 def note_defaulted_comparison_cannot_deduce_undeduced_auto : Note<
   "%select{|member|base class}0 %1 declared here">;
 def note_defaulted_comparison_cannot_deduce_callee : Note<
   "selected 'operator<=>' for %select{|member|base class}0 %1 declared here">;
+def note_defaulted_comparison_selected_invalid : Note<
+  "would compare %select{|member|base class}0 %1 "
+  "as %2, which does not support relational comparisons">;
 def err_incorrect_defaulted_comparison_constexpr : Error<
   "defaulted definition of %select{%sub{select_defaulted_comparison_kind}1|"
   "three-way comparison operator}0 "
   "cannot be declared %select{constexpr|consteval}2 because "
   "%select{it|the corresponding implicit 'operator=='}0 "
   "invokes a non-constexpr comparison function">;
 def note_defaulted_comparison_not_constexpr : Note<
   "non-constexpr comparison function would be used to compare "

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 if (OO == OO_EqualEqual ||
-             !Args[0]->getType()->isFunctionPointerType()) {
+    else
       // 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: {
       // C++2a [class.compare.secondary]p2 [P2002R0]:
       //   The operator function [...] is defined as deleted if [...] the
case OR_Success: {
                 ArgClass, Best->FoundDecl, ObjectType, Subobj.Loc,
                 Diagnose == ExplainDeleted
                     ? S.PDiag(diag::note_defaulted_comparison_inaccessible)
                           << FD << Subobj.Kind << Subobj.Decl
                     : S.PDiag()))
           return Result::deleted();
       }
 
-      // C++2a [class.compare.default]p3 [P2002R0]:
-      //   A defaulted comparison function is constexpr-compatible if [...]
-      //   no overlod resolution performed [...] results in a non-constexpr
-      //   function.
+      bool NeedsDeducing =
+          OO == OO_Spaceship && FD->getReturnType()->isUndeducedAutoType();
+
       if (FunctionDecl *BestFD = Best->Function) {
+        // C++2a [class.compare.default]p3 [P2002R0]:
+        //   A defaulted comparison function is constexpr-compatible if
+        //   [...] no overlod resolution performed [...] results in a
+        //   non-constexpr function.
         assert(!BestFD->isDeleted() && "wrong overload resolution result");
         // If it's not constexpr, explain why not.
         if (Diagnose == ExplainConstexpr && !BestFD->isConstexpr()) {
           if (Subobj.Kind != Subobject::CompleteObject)
             S.Diag(Subobj.Loc, diag::note_defaulted_comparison_not_constexpr)
               << Subobj.Kind << Subobj.Decl;
           S.Diag(BestFD->getLocation(),
                  diag::note_defaulted_comparison_not_constexpr_here);
           // Bail out after explaining; we don't want any more notes.
           return Result::deleted();
         }
         R.Constexpr &= BestFD->isConstexpr();
-      }
 
-      if (OO == OO_Spaceship && FD->getReturnType()->isUndeducedAutoType()) {
-        if (auto *BestFD = Best->Function) {
+        if (NeedsDeducing) {
           // If any callee has an undeduced return type, deduce it now.
           // FIXME: It's not clear how a failure here should be handled. For
           // now, we produce an eager diagnostic, because that is forward
           // compatible with most (all?) other reasonable options.
           if (BestFD->getReturnType()->isUndeducedType() &&
               S.DeduceReturnType(BestFD, FD->getLocation(),
                                  /*Diagnose=*/false)) {
             // Don't produce a duplicate error when asked to explain why the
case OR_Success: {
                   diag::note_defaulted_comparison_cannot_deduce_undeduced_auto)
                   << Subobj.Kind << Subobj.Decl;
               S.Diag(BestFD->getLocation(),
                      diag::note_defaulted_comparison_cannot_deduce_callee)
                   << Subobj.Kind << Subobj.Decl;
             }
             return Result::deleted();
           }
-          if (auto *Info = S.Context.CompCategories.lookupInfoForType(
-              BestFD->getCallResultType())) {
-            R.Category = Info->Kind;
-          } else {
+          auto *Info = S.Context.CompCategories.lookupInfoForType(
+              BestFD->getCallResultType());
+          if (!Info) {
             if (Diagnose == ExplainDeleted) {
               S.Diag(Subobj.Loc, diag::note_defaulted_comparison_cannot_deduce)
                   << Subobj.Kind << Subobj.Decl
                   << BestFD->getCallResultType().withoutLocalFastQualifiers();
               S.Diag(BestFD->getLocation(),
                      diag::note_defaulted_comparison_cannot_deduce_callee)
                   << Subobj.Kind << Subobj.Decl;
             }
             return Result::deleted();
           }
-        } else {
-          QualType T = Best->BuiltinParamTypes[0];
-          assert(T == Best->BuiltinParamTypes[1] &&
-                 "builtin comparison for different types?");
-          assert(Best->BuiltinParamTypes[2].isNull() &&
-                 "invalid builtin comparison");
+          R.Category = Info->Kind;
+        }
+
+        // The builtin operator for relational comparisons on function
+        // pointers is the only known case which cannot be used.
+        if (OO != OO_EqualEqual && T->isFunctionPointerType()) {
+          if (Diagnose == ExplainDeleted)
+            S.Diag(Subobj.Loc, diag::note_defaulted_comparison_selected_invalid)
+                << Subobj.Kind << Subobj.Decl << T;
+          return Result::deleted();
+        }
+
+        if (NeedsDeducing) {
           Optional<ComparisonCategoryType> Cat =
               getComparisonCategoryForBuiltinCmp(T);
           assert(Cat && "no category for builtin comparison?");
           R.Category = *Cat;
         }
       }
 
       // Note that we might be rewriting to a different operator. That call is

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

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 three-way comparison function for member 'x'}}
+    void (*x)();                                 // expected-note {{does not support relational comparisons}}
   };
 
   struct B {
     auto operator<=>(const B &) const = default; // expected-warning {{implicitly deleted}}
     void (B::*x)();                              // expected-note {{because there is no viable three-way comparison function for member 'x'}}
   };
 
   struct C {
namespace PR50591 {
   struct a2 {
     operator float() const;
   };
   struct b2 {
     auto operator<=>(b2 const &) const = default;
     a2 f;
   };
   std::partial_ordering cmp_b2 = b2() <=> b2();
+
+  struct a3 {
+    using fp = void (*)();
+    operator fp() const;
+  };
+  struct b3 {
+    auto operator<=>(b3 const &) const = default; // expected-warning {{implicitly deleted}}
+    a3 f; // expected-note {{would compare member 'f' as 'void (*)()', which does not support relational comparisons}}
+  };
 }