[Clang] Reject in-class defaulting of previously declared comparison operators

[royjacobson]

Summary

Comparison operators are not allowed to be defaulted if they were previously declared outside the class.
Pretty low-impact, but it's nice to reject this without a linking error.
Fixes https://github.com/llvm/llvm-project/issues/51227.

[royjacobson]

rebase on main

[ChuanqiXu]

LGTM.

[shafik]

Interesting it looks like neither gcc nor MSVC diagnose this either but it looks correct to me.

clang/docs/ReleaseNotes.rst

 - In C mode, when ``e1`` has ``__attribute__((noreturn))`` but ``e2`` doesn't,
   ``(c ? e1 : e2)`` is no longer considered noreturn.
   `Issue 59792 <https://github.com/llvm/llvm-project/issues/59792>`_
 - Fix an issue that makes Clang crash on lambda template parameters. This fixes
   `Issue 57960 <https://github.com/llvm/llvm-project/issues/57960>`_
 - Fix issue that the standard C++ modules importer will call global
   constructor/destructor for the global varaibles in the importing modules.
   This fixes `Issue 59765 <https://github.com/llvm/llvm-project/issues/59765>`_
+- Reject in-class defaulting of previosly declared comparison operators. Fixes
+  `Issue 51227 <https://github.com/llvm/llvm-project/issues/51227>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_defaulted_comparison_deduced_return_type_not_auto : Error<
   "must be 'auto', not %1">;
 def warn_defaulted_comparison_deleted : Warning<
   "explicitly defaulted %sub{select_defaulted_comparison_kind}0 is implicitly "
   "deleted">, InGroup<DefaultedFunctionDeleted>;
 def err_non_first_default_compare_deletes : Error<
   "defaulting %select{this %sub{select_defaulted_comparison_kind}1|"
   "the corresponding implicit 'operator==' for this defaulted 'operator<=>'}0 "
   "would delete it after its first declaration">;
+def err_non_first_default_compare_in_class : Error<
+  "defaulting this %sub{select_defaulted_comparison_kind}0 "
+  "is not allowed because it was already declared outside the class">;
 def note_defaulted_comparison_union : Note<
   "defaulted %0 is implicitly deleted because "
   "%2 is a %select{union-like class|union}1 with variant members">;
 def note_defaulted_comparison_reference_member : Note<
   "defaulted %0 is implicitly deleted because "
   "class %1 has a reference member">;
 def note_defaulted_comparison_ambiguous : Note<
   "defaulted %0 is implicitly deleted because implied %select{|'==' |'<' }1"

clang/lib/Sema/SemaDeclCXX.cpp

if (RD->isDependentType())
     return false;
 
   // Determine whether the function should be defined as deleted.
   DefaultedComparisonInfo Info =
       DefaultedComparisonAnalyzer(*this, RD, FD, DCK).visit();
 
   bool First = FD == FD->getCanonicalDecl();
 
-  // If we want to delete the function, then do so; there's nothing else to
-  // check in that case.
-  if (Info.Deleted) {
-    if (!First) {
+    if (Info.Deleted) {
       // C++11 [dcl.fct.def.default]p4:
       //   [For a] user-provided explicitly-defaulted function [...] if such a
       //   function is implicitly defined as deleted, the program is ill-formed.
       //
       // This is really just a consequence of the general rule that you can
       // only delete a function on its first declaration.
       Diag(FD->getLocation(), diag::err_non_first_default_compare_deletes)
           << FD->isImplicit() << (int)DCK;
       DefaultedComparisonAnalyzer(*this, RD, FD, DCK,
                                   DefaultedComparisonAnalyzer::ExplainDeleted)
           .visit();
       return true;
     }
+    if (isa<CXXRecordDecl>(FD->getLexicalDeclContext())) {
+      // C++20 [class.compare.default]p1:
+      //   [...] A definition of a comparison operator as defaulted that appears
+      //   in a class shall be the first declaration of that function.
+      Diag(FD->getLocation(), diag::err_non_first_default_compare_in_class)
+          << (int)DCK;
+      Diag(FD->getCanonicalDecl()->getLocation(),
+           diag::note_previous_declaration);
+      return true;
+    }
+  }
 
+  // If we want to delete the function, then do so; there's nothing else to
+  // check in that case.
+  if (Info.Deleted) {
     SetDeclDeleted(FD, FD->getLocation());
     if (!inTemplateInstantiation() && !FD->isImplicit()) {
       Diag(FD->getLocation(), diag::warn_defaulted_comparison_deleted)
           << (int)DCK;
       DefaultedComparisonAnalyzer(*this, RD, FD, DCK,
                                   DefaultedComparisonAnalyzer::ExplainDeleted)
           .visit();
       if (FD->getDefaultLoc().isValid())

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

 bool operator==(S3 const &, S3 const &) = default; // expected-error{{not a friend}}
 
 struct S4;                                         // expected-note{{forward declaration}}
 bool operator==(S4 const &, S4 const &) = default; // expected-error{{not a friend}}
 
 struct S5;                         // expected-note 3{{forward declaration}}
 bool operator==(S5, S5) = default; // expected-error{{not a friend}} expected-error 2{{has incomplete type}}
 
+struct S6;
+bool operator==(const S6&, const S6&); // expected-note {{previous declaration}}
+struct S6 {
+    friend bool operator==(const S6&, const S6&) = default; // expected-error {{because it was already declared outside}}
+};
+
 enum e {};
 bool operator==(e, int) = default; // expected-error{{expected class or reference to a constant class}}
 
 bool operator==(e *, int *) = default; // expected-error{{must have at least one}}
 } // namespace p2085
 
 namespace p2085_2 {
 template <class T> struct S6 {

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

union E2 {
   bool operator<(const E2&) const = default;
   bool operator<=(const E2&) const = default;
   bool operator>(const E2&) const = default;
   bool operator>=(const E2&) const = default;
 };
 
 struct F;
 bool operator==(const F&, const F&);
-bool operator!=(const F&, const F&);
+bool operator!=(const F&, const F&); // expected-note {{previous declaration}}
 bool operator<=>(const F&, const F&);
-bool operator<(const F&, const F&);
+bool operator<(const F&, const F&); // expected-note {{previous declaration}}
 struct F {
   union { int a; };
   friend bool operator==(const F&, const F&) = default; // expected-error {{defaulting this equality comparison operator would delete it after its first declaration}} expected-note {{implicitly deleted because 'F' is a union-like class}}
-  friend bool operator!=(const F&, const F&) = default;
+  friend bool operator!=(const F&, const F&) = default; // expected-error {{because it was already declared outside}}
   friend bool operator<=>(const F&, const F&) = default; // expected-error {{defaulting this three-way comparison operator would delete it after its first declaration}} expected-note {{implicitly deleted because 'F' is a union-like class}}
-  friend bool operator<(const F&, const F&) = default;
+  friend bool operator<(const F&, const F&) = default; // expected-error {{because it was already declared outside}}
 };

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

struct F {
   friend constexpr bool operator>(const F&, const F&) = default; // expected-error {{cannot be declared constexpr because it invokes a non-constexpr comparison function}}
   friend constexpr bool operator>=(const F&, const F&) = default; // expected-error {{cannot be declared constexpr because it invokes a non-constexpr comparison function}}
 };
 
 // No implicit 'constexpr' if it's not the first declaration.
 // FIXME: This rule creates problems for reordering of declarations; is this
 // really the right model?
 struct G;
-bool operator==(const G&, const G&);
-bool operator!=(const G&, const G&);
-std::strong_ordering operator<=>(const G&, const G&);
-bool operator<(const G&, const G&);
-bool operator<=(const G&, const G&);
-bool operator>(const G&, const G&);
-bool operator>=(const G&, const G&);
+bool operator==(const G&, const G&); // expected-note {{previous declaration}}
+bool operator!=(const G&, const G&); // expected-note {{previous declaration}}
+std::strong_ordering operator<=>(const G&, const G&); // expected-note {{previous declaration}}
+bool operator<(const G&, const G&); // expected-note {{previous declaration}}
+bool operator<=(const G&, const G&); // expected-note {{previous declaration}}
+bool operator>(const G&, const G&); // expected-note {{previous declaration}}
+bool operator>=(const G&, const G&); // expected-note {{previous declaration}}
 struct G {
-  friend bool operator==(const G&, const G&) = default;
-  friend bool operator!=(const G&, const G&) = default;
+  friend bool operator==(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
+  friend bool operator!=(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
 
-  friend std::strong_ordering operator<=>(const G&, const G&) = default;
-  friend bool operator<(const G&, const G&) = default;
-  friend bool operator<=(const G&, const G&) = default;
-  friend bool operator>(const G&, const G&) = default;
-  friend bool operator>=(const G&, const G&) = default;
+  friend std::strong_ordering operator<=>(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
+  friend bool operator<(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
+  friend bool operator<=(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
+  friend bool operator>(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
+  friend bool operator>=(const G&, const G&) = default; // expected-error {{because it was already declared outside}}
 };
 bool operator==(const G&, const G&);
 bool operator!=(const G&, const G&);
 
 std::strong_ordering operator<=>(const G&, const G&);
 bool operator<(const G&, const G&);
 bool operator<=(const G&, const G&);
 bool operator>(const G&, const G&);