[clang] diagnose instead of assert for defaulted comparison return type deduction.

[mizvekov]

Summary

When deducing the return type of defaulted three-way comparison, instead
of asserting in the case of unsupported builtin types, just diagnose it.

For now, we add a test case for function pointers which will produce
slightly incorrect diagnostics. In future work, we will stop adding
function pointers to the candidate set and get the correct diagnostics.

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

Depends on D103760

[mizvekov]

clang/lib/Sema/SemaDeclCXX.cpp

7876

I think this should be an error because otherwise we would be accepting and giving this program a different semantic just because of some not-implemented feature.
But I can see the downside that we could potentially reject a program which is not affected by the operator being implicitly deleted.

Thoughts?

[rsmith]

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

200

I think three-way comparison for function pointers is ill-formed, and that the problem in this case is that [over.built] lists candidates for which a three-way comparison is invalid. So our overload resolution succeeds, but results in a meaningless builtin candidate with an undefined return type.

I think the behavior here after this patch is fine -- we should treat the comparison as deleted -- but the diagnostic is wrong. This isn't a "not currently supported" case, this is a "not a valid expression" case, per [class.compare.default]/3.2 (except that the wording currently gets this wrong and doesn't actually say what happens here, because we're actually in [class.compare.default]/3.1). Though perhaps the better fix would be to not even include a builtin candidate for operator<=>(void (*)(), void (*)())?

[mizvekov]

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

200

Yeah this was part of my plan to give the correct diagnostic for function pointers in the next patch in the series:
https://reviews.llvm.org/D103855

The idea on this one is to produce an error, is the TU will not compile, for built-in types not supported.

After the next one, we just implicitly delete the comparison for function pointers, and leave the error for other unimplemented built-in types.

I think this makes more sense, even if we manage to implement this deduction for all built-in types currently available, it would still be too easy for another patch to introduce a new built-in types and forget to support it here.

And it does make sense to error on these cases than to simply pick an arbitrary answer and churn along, I think

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 err_defaulted_comparison_cannot_deduce_unsupported_builtin_type : Error<
+  "return type of defaulted 'operator<=>' cannot be deduced because "
+  "three-way comparison for %select{|member|base class}0 %1 "
+  "would compare as builtin type %2 which is not currently supported by clang">;
 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

case OR_Success: {
         } else {
           QualType T = Best->BuiltinParamTypes[0];
           assert(T == Best->BuiltinParamTypes[1] &&
                  "builtin comparison for different types?");
           assert(Best->BuiltinParamTypes[2].isNull() &&
                  "invalid builtin comparison");
           Optional<ComparisonCategoryType> Cat =
               getComparisonCategoryForBuiltinCmp(T);
-          assert(Cat && "no category for builtin comparison?");
+          if (!Cat) {
+            if (Diagnose == NoDiagnostics)
+              S.Diag(
+                  FD->getLocation(),
+                  diag::
+                      err_defaulted_comparison_cannot_deduce_unsupported_builtin_type)
+                  << Subobj.Kind << Subobj.Decl << T;
+            return Result::deleted();
+          }
           R.Category = *Cat;
         }
       }
 
       // Note that we might be rewriting to a different operator. That call is
       // not considered until we come to actually build the comparison function.
       break;
     }

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

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-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;
+  };
 }