[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/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

clang/lib/Sema/SemaOverload.cpp

public:
 
   BuiltinCandidateTypeSet(Sema &SemaRef)
     : HasNonRecordTypes(false),
       HasArithmeticOrEnumeralTypes(false),
       HasNullPtrType(false),
       SemaRef(SemaRef),
       Context(SemaRef.Context) { }
 
-  void AddTypesConvertedFrom(QualType Ty,
-                             SourceLocation Loc,
+  void AddTypesConvertedFrom(QualType Ty, SourceLocation Loc,
                              bool AllowUserConversions,
                              bool AllowExplicitConversions,
+                             bool DisallowNonRelationalComparableTypes,
                              const Qualifiers &VisibleTypeConversionsQuals);
 
   llvm::iterator_range<iterator> pointer_types() { return PointerTypes; }
   llvm::iterator_range<iterator> member_pointer_types() {
     return MemberPointerTypes;
   }
   llvm::iterator_range<iterator> enumeration_types() {
     return EnumerationTypes;
BuiltinCandidateTypeSet::AddMemberPointerWithMoreQualifiedTypeVariants(
   return true;
 }
 
 /// AddTypesConvertedFrom - Add each of the types to which the type @p
 /// Ty can be implicit converted to the given set of @p Types. We're
 /// primarily interested in pointer types and enumeration types. We also
 /// take member pointer types, for the conditional operator.
 /// AllowUserConversions is true if we should look at the conversion
-/// functions of a class type, and AllowExplicitConversions if we
+/// functions of a class type, AllowExplicitConversions if we
 /// should also include the explicit conversion functions of a class
-/// type.
-void
-BuiltinCandidateTypeSet::AddTypesConvertedFrom(QualType Ty,
-                                               SourceLocation Loc,
-                                               bool AllowUserConversions,
-                                               bool AllowExplicitConversions,
-                                               const Qualifiers &VisibleQuals) {
+/// type, and DisallowNonRelationalComparableTypes if we should exclude types
+/// which cannot be compared with relational operators.
+void BuiltinCandidateTypeSet::AddTypesConvertedFrom(
+    QualType Ty, SourceLocation Loc, bool AllowUserConversions,
+    bool AllowExplicitConversions, bool DisallowNonRelationalComparableTypes,
   // Only deal with canonical types.
   Ty = Context.getCanonicalType(Ty);
 
   // Look through reference types; they aren't part of the type of an
   // expression for the purposes of conversions.
   if (const ReferenceType *RefTy = Ty->getAs<ReferenceType>())
     Ty = RefTy->getPointeeType();
 
void BuiltinCandidateTypeSet::AddTypesConvertedFrom(
 
   // Flag if we encounter an arithmetic type.
   HasArithmeticOrEnumeralTypes =
     HasArithmeticOrEnumeralTypes || Ty->isArithmeticType();
 
   if (Ty->isObjCIdType() || Ty->isObjCClassType())
     PointerTypes.insert(Ty);
   else if (Ty->getAs<PointerType>() || Ty->getAs<ObjCObjectPointerType>()) {
+    // The builtin operator for relational comparisons on function pointers
+    // is the only known case which cannot be used for three-way comparisons.
+    if (DisallowNonRelationalComparableTypes && Ty->isFunctionPointerType())
+      return;
     // Insert our type, and its more-qualified variants, into the set
     // of types.
     if (!AddPointerWithMoreQualifiedTypeVariants(Ty, VisibleQuals))
       return;
   } else if (Ty->isMemberPointerType()) {
     // Member pointers are far easier, since the pointee can't be converted.
     if (!AddMemberPointerWithMoreQualifiedTypeVariants(Ty))
       return;
for (NamedDecl *D : ClassDecl->getVisibleConversionFunctions()) {
       // Skip conversion function templates; they don't tell us anything
       // about which builtin types we can convert to.
       if (isa<FunctionTemplateDecl>(D))
         continue;
 
       CXXConversionDecl *Conv = cast<CXXConversionDecl>(D);
       if (AllowExplicitConversions || !Conv->isExplicit()) {
         AddTypesConvertedFrom(Conv->getConversionType(), Loc, false, false,
+                              DisallowNonRelationalComparableTypes,
                               VisibleQuals);
       }
     }
   }
 }
 /// Helper function for adjusting address spaces for the pointer or reference
 /// operands of builtin operators depending on the argument.
 static QualType AdjustAddressSpaceForBuiltinOperandType(Sema &S, QualType T,
void Sema::AddBuiltinOperatorCandidates(OverloadedOperatorKind Op,
   for (unsigned ArgIdx = 0, N = Args.size(); ArgIdx != N; ++ArgIdx)
     VisibleTypeConversionsQuals += CollectVRQualifiers(Context, Args[ArgIdx]);
 
   bool HasNonRecordCandidateType = false;
   bool HasArithmeticOrEnumeralCandidateType = false;
   SmallVector<BuiltinCandidateTypeSet, 2> CandidateTypes;
   for (unsigned ArgIdx = 0, N = Args.size(); ArgIdx != N; ++ArgIdx) {
     CandidateTypes.emplace_back(*this);
-    CandidateTypes[ArgIdx].AddTypesConvertedFrom(Args[ArgIdx]->getType(),
-                                                 OpLoc,
-                                                 true,
-                                                 (Op == OO_Exclaim ||
-                                                  Op == OO_AmpAmp ||
-                                                  Op == OO_PipePipe),
-                                                 VisibleTypeConversionsQuals);
+    CandidateTypes[ArgIdx].AddTypesConvertedFrom(
+        Args[ArgIdx]->getType(), OpLoc, true,
+        (Op == OO_Exclaim || Op == OO_AmpAmp || Op == OO_PipePipe),
+        (Op == OO_Less || Op == OO_Spaceship), VisibleTypeConversionsQuals);
     HasNonRecordCandidateType = HasNonRecordCandidateType ||
         CandidateTypes[ArgIdx].hasNonRecordTypes();
     HasArithmeticOrEnumeralCandidateType =
         HasArithmeticOrEnumeralCandidateType ||
         CandidateTypes[ArgIdx].hasArithmeticOrEnumeralTypes();
   }
 
   // Exit early when no non-record types have been added to the candidate set

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

namespace PR50591 {
   };
   std::partial_ordering cmp_b2 = b2() <=> b2();
 
   struct a3 {
     using fp = void (*)();
     operator fp() const;
   };
   struct b3 {
-    auto operator<=>(b3 const &) const = default; // expected-error {{cannot be deduced because three-way comparison for member 'f' would compare as builtin type 'void (*)()' which is not currently supported}}
-    // expected-warning@-1 {{implicitly deleted}}
-    a3 f;
+    auto operator<=>(b3 const &) const = default; // expected-warning {{implicitly deleted}}
+    a3 f; // expected-note {{because there is no viable three-way comparison function for member 'f'}}
   };
 }