[clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.

[massberg]

Summary

This patch implemed the change proposed in [P2002R1] to 11.11.1 [class.compare.default] paragraph 1.

A defaulted compariosn operator function must be non-volatile and must either have no ref-qualifier or the ref-qualifier &.

[ilya-biryukov]

Thanks! I believe we should also recover in the similar manner we do for missing const, see corresponding comment.

Extra NITs:

• there is a typo in description: *compariosn* should be comparison,
• we probably want to add this change to release notes.

Also adding @clang-language-wg in case someone else wants to chime in.

clang/include/clang/Basic/DiagnosticSemaKinds.td

9466

NIT: for consistency with wording of notes above.

9468

NIT: for consistency with the wording of err_ref_qualifier_constructor

clang/lib/Sema/SemaDeclCXX.cpp

8596–8604

NIT: this version is simpler and more aligned with the code below.
Also, could you move the volatile handling after the check for const?

Additionally, we seem to recover in case of const by adding it to the type and suggesting a fix-it to add it in the code.
Could we do the same for volatile and ref-qualifier, i.e. suggest a fix-it to remove the ref-qualifier and volatile and change the corresponding type to make AST pretend they were never there?

[shafik]

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

18–19

Can we also have examples with out of line defaulting.

[aaron.ballman]

Precommit CI found a relevant failure that should be addressed.

clang/lib/Sema/SemaDeclCXX.cpp

8596

8596–8604

Note, you can shorten it further with:

return Diag(...);

because that will return true for you.

[massberg]

Thanks for the comments!
Updated error messages, code and tests.

[massberg]

clang/lib/Sema/SemaDeclCXX.cpp

8596

Thanks! Which the change suggested by Ilya this has become obsolete.

8596–8604

Thanks, this is much simpler!

I have moved the code and added a recovery. However, adding a fix-it is much more complex as there is no simple way to get location to remove the keywords. Thus I decided to not offering a fix-it in this patch.

[aaron.ballman]

Basically looking good to me now, but the changes need a release note and I does this mean we can change cxx_status.html to claim full support for P2002R1 now or is there more left to be done for that? (If there's more left to do, adding those details to the cxx_status page would be appropriate.)

clang/include/clang/Basic/DiagnosticSemaKinds.td

9468

Still need to drop the s from operators

clang/lib/Sema/SemaDeclCXX.cpp

8596–8604

Yeah, it seems we don't track the ref qualifier location on any of the CXXMethodDecl, FunctionProtoType, or FunctionProtoTypeLoc classes. A future refactoring can consider adding locations for those to FunctionProtoTypeLoc, but needs to be able to handle multiple qualifiers (e.g., void func() const &; should track both the const and the & separately).

[massberg]

Fixed typo, added release note and updated cxx_status.html.

[aaron.ballman]

Did you verify that the rest of P2002R1 was implemented correctly? There are a lot more changes in that paper than just this tiny bit, so if we're not certain this finishes things, we shouldn't mark the status as complete (but if we're figuring out what's missing, we should capture those details somewhere so we know what's left to do)

https://github.com/llvm/llvm-project/commit/b74a381296eef048911bb22dc4eb2d3598460470 is what marked the status as partial, but there's no explanation as to why it was partial. Perhaps @rsmith remembers what was left to be done there.

[massberg]

Fix test that broke after last change.

[massberg]

I have checked the paper P2002R1 and as far as I can tell it is fully implemented when this patch has landed.

[shafik]

In D148924#4318381, @massberg wrote:

I have checked the paper P2002R1 and as far as I can tell it is fully implemented when this patch has landed.

So I read the paper but since the paper does not have examples for each change nor does it have any links to DRs it is not obvious without spending some time to verify that we support each section. Changes like these really should come with examples in the paper to demonstrate behavior before and after.

I searched in the code and found that @rsmith labelled P2002R0 changes with comments and so a cursory look at those changes side by side with the paper seems to indicate that we probably covered most of the cases but I am not 100% sure. There is test coverage added with those changes as well but again relating each test to specific paper section is not trivial, at least for me.

The paper also uses the term constexpr compatible which has been replaced with constexpr suitable and not sure if that has any practical effect here.

[ilya-biryukov]

In D148924#4320694, @shafik wrote:

The paper also uses the term constexpr compatible which has been replaced with constexpr suitable and not sure if that has any practical effect here.

They seem to be different terms, constexpr-suitable does not actually replace constexpr-compatible. The former refers to more basic checks (i.e. "not a coroutine", "not a ctor or dtor for a class with virtual bases"), the latter only pops up in context of defaulted functions.
Constexpr-compatible has been removed completely in P2448r2, but I think it's only applicable in C++23 and this change aims to implement C++20.

[ilya-biryukov]

@massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this?

[massberg]

In D148924#4391190, @ilya-biryukov wrote:

@massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this?

The only thing I came across was https://reviews.llvm.org/D151200 (error out when parameters of defaulted comparison operator aren't as expected), but this is already submitted.
Related to the defaulted comparison operators is DR 2450 (braced-init-list as a template-argument), but this isn't part of P2002R1.

So I'm not aware of any blockers.

[ilya-biryukov]

Since there are no remaining blockers, LGTM from my side.
I suggest waiting for ~day to give chance to other reviewers.

@shafik, @aaron.ballman, @rsmith please let us know if you want to make another run through the paper to do extra checking.

[aaron.ballman]

LGTM aside from a minor issue with the status page (we use unreleased until the current version is released).

clang/www/cxx_status.html

535

[shafik]

LGTM

[massberg]

Fix status of paper in status page to unreleased.

[massberg]

Format code.

clang/docs/ReleaseNotes.rst

- Downgraded use of a reserved identifier in a module export declaration from
   <https://github.com/llvm/llvm-project/issues/61446>`_ and allows easier
   building of standard modules. This diagnostic may be strengthened into an
   error again in the future once there is a less fragile way to mark a module
   as being part of the implementation rather than a user module.
 - Clang now implements `[temp.deduct]p9`. Substitution failures inside lambdas from
   unevaluated contexts will be surfaced as errors. They were previously handled as
   SFINAE.
 - Clang now supports `requires cplusplus20` for module maps.
+- Implemented missing parts of `P2002R1: Consistent comparison operators <https://wg21.link/P2002R1>`_
 
 C++23 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Implemented `P2036R3: Change scope of lambda trailing-return-type <https://wg21.link/P2036R3>`_
   and `P2579R0 Mitigation strategies for P2036 <https://wg21.link/P2579R0>`_.
   These proposals modify how variables captured in lambdas can appear in trailing return type
   expressions and how their types are deduced therein, in all C++ language versions.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_defaulted_comparison_not_constexpr : Note<
   "non-constexpr comparison function would be used to compare "
   "%select{|member %1|base class %1}0">;
 def note_defaulted_comparison_not_constexpr_here : Note<
   "non-constexpr comparison function declared here">;
 def note_in_declaration_of_implicit_equality_comparison : Note<
   "while declaring the corresponding implicit 'operator==' "
   "for this defaulted 'operator<=>'">;
+def err_volatile_comparison_operator : Error<
+  "defaulted comparison function must not be volatile">;
+def err_ref_qualifier_comparison_operator : Error<
+  "ref-qualifier '&&' is not allowed on a defaulted comparison operator">;
 
 def ext_implicit_exception_spec_mismatch : ExtWarn<
   "function previously declared with an %select{explicit|implicit}0 exception "
   "specification redeclared with an %select{implicit|explicit}0 exception "
   "specification">, InGroup<DiagGroup<"implicit-exception-spec-mismatch">>;
 
 def warn_ptr_arith_precedes_bounds : Warning<
   "the pointer decremented by %0 refers before the beginning of the array">,

clang/lib/Sema/SemaDeclCXX.cpp

lookupOperatorsForDefaultedComparison(*this, S, Operators,
                                           FD->getOverloadedOperator());
     FD->setDefaultedFunctionInfo(FunctionDecl::DefaultedFunctionInfo::Create(
         Context, Operators.pairs()));
   }
 
   // C++2a [class.compare.default]p1:
   //   A defaulted comparison operator function for some class C shall be a
   //   non-template function declared in the member-specification of C that is
-  //    -- a non-static const member of C having one parameter of type
-  //       const C&, or
+  //    -- a non-static const non-volatile member of C having one parameter of
+  //       type const C& and either no ref-qualifier or the ref-qualifier &, or
   //    -- a friend of C having two parameters of type const C& or two
   //       parameters of type C.
 
   CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(FD->getLexicalDeclContext());
   bool IsMethod = isa<CXXMethodDecl>(FD);
   if (IsMethod) {
     auto *MD = cast<CXXMethodDecl>(FD);
     assert(!MD->isStatic() && "comparison function cannot be a static member");
 
+    if (MD->getRefQualifier() == RQ_RValue) {
+      Diag(MD->getLocation(), diag::err_ref_qualifier_comparison_operator);
+
+      // Remove the ref qualifier to recover.
+      const auto *FPT = MD->getType()->castAs<FunctionProtoType>();
+      FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
+      EPI.RefQualifier = RQ_None;
+      MD->setType(Context.getFunctionType(FPT->getReturnType(),
+                                          FPT->getParamTypes(), EPI));
+    }
+
     // If we're out-of-class, this is the class we're comparing.
     if (!RD)
       RD = MD->getParent();
 
     if (!MD->isConst()) {
       SourceLocation InsertLoc;
       if (FunctionTypeLoc Loc = MD->getFunctionTypeLoc())
         InsertLoc = getLocForEndOfToken(Loc.getRParenLoc());
       // Don't diagnose an implicit 'operator=='; we will have diagnosed the
       // corresponding defaulted 'operator<=>' already.
       if (!MD->isImplicit()) {
         Diag(MD->getLocation(), diag::err_defaulted_comparison_non_const)
             << (int)DCK << FixItHint::CreateInsertion(InsertLoc, " const");
       }
 
       // Add the 'const' to the type to recover.
       const auto *FPT = MD->getType()->castAs<FunctionProtoType>();
       FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
       EPI.TypeQuals.addConst();
       MD->setType(Context.getFunctionType(FPT->getReturnType(),
                                           FPT->getParamTypes(), EPI));
     }
+
+    if (MD->isVolatile()) {
+      Diag(MD->getLocation(), diag::err_volatile_comparison_operator);
+
+      // Remove the 'volatile' from the type to recover.
+      const auto *FPT = MD->getType()->castAs<FunctionProtoType>();
+      FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
+      EPI.TypeQuals.removeVolatile();
+      MD->setType(Context.getFunctionType(FPT->getReturnType(),
+                                          FPT->getParamTypes(), EPI));
+    }
   }
 
   if (FD->getNumParams() != (IsMethod ? 1 : 2)) {
     // Let's not worry about using a variadic template pack here -- who would do
     // such a thing?
     Diag(FD->getLocation(), diag::err_defaulted_comparison_num_args)
         << int(IsMethod) << int(DCK);
     return true;

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

struct A {
   friend bool operator!=(const A&, const B&) = default; // expected-error {{parameters for defaulted equality comparison operator must have the same type (found 'const A &' vs 'const B &')}}
   friend bool operator!=(const B&, const B&) = default; // expected-error {{invalid parameter type for defaulted equality comparison}}
   friend bool operator<(const A&, const A&);
   friend bool operator<(const B&, const B&) = default; // expected-error {{invalid parameter type for defaulted relational comparison}}
   friend bool operator>(A, A) = default; // expected-warning {{implicitly deleted}} expected-note{{replace 'default'}}
 
   bool operator<(const A&) const;
   bool operator<=(const A&) const = default;
-  bool operator==(const A&) const volatile && = default; // surprisingly, OK
+  bool operator==(const A&) const && = default; // expected-error {{ref-qualifier '&&' is not allowed on a defaulted comparison operator}}
+  bool operator>=(const A&) const volatile = default; // expected-error {{defaulted comparison function must not be volatile}}
   bool operator<=>(const A&) = default; // expected-error {{defaulted member three-way comparison operator must be const-qualified}}
   bool operator>=(const B&) const = default; // expected-error-re {{invalid parameter type for defaulted relational comparison operator; found 'const B &', expected 'const A &'{{$}}}}
   static bool operator>(const B&) = default; // expected-error {{overloaded 'operator>' cannot be a static member function}}
   friend bool operator>(A, const A&) = default; // expected-error {{must have the same type}} expected-note {{would be the best match}}
 
   template<typename T = void>
     friend bool operator==(const A&, const A&) = default; // expected-error {{comparison operator template cannot be defaulted}}
   template<typename T = void>

 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}}
 };
 
+struct S7 {
+  bool operator==(S7 const &) const &&;
+};
+bool S7::operator==(S7 const &) const && = default; // expected-error {{ref-qualifier '&&' is not allowed on a defaulted comparison operator}}
+
 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/CodeGenCXX/virtual-compare.cpp

 };
 struct A : X, Y {
   // CHECK-SAME: @_ZN1A1fEv
   virtual void f();
   // CHECK-SAME: @_ZNKR1AssERKS_
   virtual std::strong_ordering operator<=>(const A &) const & = default;
   // CHECK-SAME: @_ZN1A1gEv
   virtual void g();
-  // CHECK-SAME: @_ZNKO1AssERKS_
-  virtual std::strong_ordering operator<=>(const A &) const && = default;
   // CHECK-SAME: @_ZN1A1hEv
   virtual void h();
 
   // CHECK-SAME: @_ZN1AaSERKS_
   // implicit virtual A &operator=(const A&) = default;
 
   // CHECK-SAME: @_ZN1AD1Ev
   // CHECK-SAME: @_ZN1AD0Ev
   // implicit virtual ~A();
 
   // CHECK-SAME: @_ZNKR1AeqERKS_
   // implicit virtual A &operator==(const A&) const & = default;
-
-  // CHECK-SAME: @_ZNKO1AeqERKS_
-  // implicit virtual A &operator==(const A&) const && = default;
 };
 
 // For Y:
 // CHECK-SAME: @_ZTI1A
 
 // CHECK-SAME: @_ZThn{{[0-9]*}}_N1AD1Ev
 // CHECK-SAME: @_ZThn{{[0-9]*}}_N1AD0Ev
 // virtual ~Y();
 
 // CHECK-SAME: @_ZThn{{[0-9]*}}_N1AaSERKS_
 // virtual A &operator=(const A &);
 
 void A::f() {}

clang/www/cxx_status.html

</tr>
       <tr> <!-- from Belfast -->
         <td><a href="https://wg21.link/p1946r0">P1946R0</a></td>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p1959r0">P1959R0</a></td>
       </tr>
       <tr> <!-- from Prague -->
         <td><a href="https://wg21.link/p2002r1">P2002R1</a></td>
-        <td class="partial" align="center">Partial</td>
+        <td class="unreleased" align="center">Clang 17</td>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p2085r0">P2085R0</a></td>
         <td class="full" align="center">Clang 14</td>
       </tr>
     <tr>
       <td>Access checking on specializations</td>
       <td><a href="https://wg21.link/p0692r1">P0692R1</a></td>