[Clang] Diagnose defaulted assignment operator with incompatible object parameter #70176

cor3ntin · 2023-10-25T08:13:17Z

Per https://eel.is/c++draft/dcl.fct.def.default#2.2, the explicit object parameter of a defaulted special member function must be of the same type as the one of an equivalent implicitly defaulted function, ignoring references.

Fixes #69233

llvmbot · 2023-10-25T08:14:25Z

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

Per https://eel.is/c++draft/dcl.fct.def.default#2.2, the explicit object parameter of a defaulted special member function must be of the same type as the one of an equivalent implicitly defaulted function, ignoring references.

Fixes #69233

Full diff: https://github.com/llvm/llvm-project/pull/70176.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
(modified) clang/lib/Sema/SemaDeclCXX.cpp (+15)
(modified) clang/test/SemaCXX/cxx2b-deducing-this.cpp (+31)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index a673ce726d6c220..6e138683334c2c3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9481,6 +9481,9 @@ def err_defaulted_special_member_return_type : Error<
 def err_defaulted_special_member_quals : Error<
   "an explicitly-defaulted %select{copy|move}0 assignment operator may not "
   "have 'const'%select{, 'constexpr'|}1 or 'volatile' qualifiers">;
+def err_defaulted_special_member_explicit_object_mismatch : Error<
+  "the type of the explicit object parameter of an explicitly-defaulted "
+  "%select{copy|move}0 assignment operator should match the type of the class %1">;
 def err_defaulted_special_member_volatile_param : Error<
   "the parameter for an explicitly-defaulted %sub{select_special_member_kind}0 "
   "may not be volatile">;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index a3f68d4ffc0f6ec..35554fca5776f2c 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7748,6 +7748,21 @@ bool Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD,
         HadError = true;
       }
     }
+    // [C++23][dcl.fct.def.default]/p2.2
+    // if F2 has an implicit object parameter of type “reference to C”,
+    // F1 may be an explicit object member function whose explicit object
+    // parameter is of (possibly different) type “reference to C”,
+    // in which case the type of F1 would differ from the type of F2
+    // in that the type of F1 has an additional parameter;
+    if(!Context.hasSameType(ThisType.getNonReferenceType().getUnqualifiedType(), Context.getRecordType(RD))) {
+      if (DeleteOnTypeMismatch)
+        ShouldDeleteForTypeMismatch = true;
+      else {
+        Diag(MD->getLocation(), diag::err_defaulted_special_member_explicit_object_mismatch)
+            << (CSM == CXXMoveAssignment) << RD;
+        HadError = true;
+      }
+    }
   }
 
   // Check for parameter type matching.
diff --git a/clang/test/SemaCXX/cxx2b-deducing-this.cpp b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
index 535381e876da9c7..f9e73b41e2c330f 100644
--- a/clang/test/SemaCXX/cxx2b-deducing-this.cpp
+++ b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
@@ -585,3 +585,34 @@ class Server : public Thing {
     S name_;
 };
 }
+
+namespace GH69233 {
+struct Base {};
+struct S : Base {
+    int j;
+    S& operator=(this Base& self, const S&) = default;
+    // expected-warning@-1 {{explicitly defaulted copy assignment operator is implicitly deleted}}
+    // expected-note@-2 {{function is implicitly deleted because its declared type does not match the type of an implicit copy assignment operator}}
+    // expected-note@-3 {{explicitly defaulted function was implicitly deleted here}}
+};
+
+struct S2 {
+    S2& operator=(this int&& self, const S2&);
+    S2& operator=(this int&& self, S2&&);
+    operator int();
+};
+
+S2& S2::operator=(this int&& self, const S2&) = default;
+// expected-error@-1 {{the type of the explicit object parameter of an explicitly-defaulted copy assignment operator should match the type of the class 'S2'}}
+
+S2& S2::operator=(this int&& self, S2&&) = default;
+// expected-error@-1 {{the type of the explicit object parameter of an explicitly-defaulted move assignment operator should match the type of the class 'S2'}}
+
+void test() {
+    S s;
+    s = s; // expected-error {{object of type 'S' cannot be assigned because its copy assignment operator is implicitly deleted}}
+    S2 s2;
+    s2 = s2;
+}
+
+}

github-actions · 2023-10-25T08:23:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

tbaederr · 2023-10-25T09:13:55Z

clang/lib/Sema/SemaDeclCXX.cpp

+      else {
+        Diag(MD->getLocation(),
+             diag::err_defaulted_special_member_explicit_object_mismatch)
+            << (CSM == CXXMoveAssignment) << RD;


Is there a useful source range we can supply here?

What do you have in mind? the first parameter? I am not sure how useful it is though - we certainly don't do it for other warnings here.

Nothing specific. In line 7709 above, we pass MD->getSourceRange().

shafik · 2023-10-27T17:07:23Z

clang/test/SemaCXX/cxx2b-deducing-this.cpp

+
+void test() {
+    S s;
+    s = s; // expected-error {{object of type 'S' cannot be assigned because its copy assignment operator is implicitly deleted}}


Can we get an example where move assignment fails as well.

AaronBallman · 2023-10-30T12:04:47Z

Per https://eel.is/c++draft/dcl.fct.def.default#2.2, the explicit object parameter of a defaulted special member function must be of the same type as the one of an equivalent implicitly defaulted function, ignoring references.

I could use an explanation as to how they must be the same type when 2.2 says "...may be an explicit object member function whose explicit object parameter is of (possibly different) type “reference to C”, in which case..."

cor3ntin · 2023-10-30T12:11:48Z

Per https://eel.is/c++draft/dcl.fct.def.default#2.2, the explicit object parameter of a defaulted special member function must be of the same type as the one of an equivalent implicitly defaulted function, ignoring references.

I could use an explanation as to how they must be the same type when 2.2 says "...may be an explicit object member function whose explicit object parameter is of (possibly different) type “reference to C”, in which case..."

That wording is terrible, but it's hard to phrase it better.
The type may be different but it is always a reference to C where C is the same type.

Ie it can be either C& or C&&. this is consistent with the fact that we allow ref-qualified functions.
There is some context here https://lists.isocpp.org/core/2023/07/14608.php

I had a chat with core on mattermost and there is agreement that the wording could be made clearer editorially.

clang/lib/Sema/SemaDeclCXX.cpp

AaronBallman · 2023-10-30T12:13:44Z

Per https://eel.is/c++draft/dcl.fct.def.default#2.2, the explicit object parameter of a defaulted special member function must be of the same type as the one of an equivalent implicitly defaulted function, ignoring references.

I could use an explanation as to how they must be the same type when 2.2 says "...may be an explicit object member function whose explicit object parameter is of (possibly different) type “reference to C”, in which case..."

That wording is terrible, but it's hard to phrase it better. The type may be different but it is always a reference to C where C is the same type.

Ie it can be either C& or C&&. this is consistent with the fact that we allow ref-qualified functions. There is some context here https://lists.isocpp.org/core/2023/07/14608.php

I had a chat with core on mattermost and there is agreement that the wording could be made clearer editorially.

Thank you for the explanation!

AaronBallman

LGTM though something seems to be amiss with precommit CI and so it might be nice to get a clean run of that before landing (I don't insist).

…ct parameter. Per https://eel.is/c++draft/dcl.fct.def.default#2.2, the explicit object parameter of a defaulted special member function must be of the same type as the one of an equivalent implicitly defaulted function, ignoring references. Fixes llvm#69233

Note that if there is an invalid move constructor, overload resolution will try to pick a copy constructor instead.

cor3ntin requested a review from AaronBallman October 25, 2023 08:13

cor3ntin added the c++23 label Oct 25, 2023

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 25, 2023

cor3ntin force-pushed the corentin/gh69233 branch from 441f247 to 6fd05ad Compare October 25, 2023 08:26

tbaederr reviewed Oct 25, 2023

View reviewed changes

cor3ntin force-pushed the corentin/gh69233 branch 2 times, most recently from 9277312 to e5f6815 Compare October 26, 2023 16:48

shafik reviewed Oct 27, 2023

View reviewed changes

AaronBallman reviewed Oct 30, 2023

View reviewed changes

clang/lib/Sema/SemaDeclCXX.cpp Show resolved Hide resolved

AaronBallman approved these changes Oct 30, 2023

View reviewed changes

cor3ntin added 3 commits October 30, 2023 13:46

add source range

885bdd5

Add tests for move.

a4a51f2

Note that if there is an invalid move constructor, overload resolution will try to pick a copy constructor instead.

cor3ntin force-pushed the corentin/gh69233 branch from 77049d5 to a4a51f2 Compare October 30, 2023 12:47

cor3ntin merged commit 273ceb1 into llvm:main Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Diagnose defaulted assignment operator with incompatible object parameter #70176

[Clang] Diagnose defaulted assignment operator with incompatible object parameter #70176

Uh oh!

cor3ntin commented Oct 25, 2023

Uh oh!

llvmbot commented Oct 25, 2023

Uh oh!

github-actions bot commented Oct 25, 2023 •

edited

Loading

Uh oh!

tbaederr Oct 25, 2023

Uh oh!

cor3ntin Oct 26, 2023

Uh oh!

tbaederr Oct 26, 2023

Uh oh!

shafik Oct 27, 2023

Uh oh!

AaronBallman commented Oct 30, 2023

Uh oh!

cor3ntin commented Oct 30, 2023

Uh oh!

Uh oh!

AaronBallman commented Oct 30, 2023

Uh oh!

AaronBallman left a comment

Uh oh!

Uh oh!

[Clang] Diagnose defaulted assignment operator with incompatible object parameter #70176

[Clang] Diagnose defaulted assignment operator with incompatible object parameter #70176

Uh oh!

Conversation

cor3ntin commented Oct 25, 2023

Uh oh!

llvmbot commented Oct 25, 2023

Uh oh!

github-actions bot commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbaederr Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

cor3ntin Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

tbaederr Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

shafik Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Oct 30, 2023

Uh oh!

cor3ntin commented Oct 30, 2023

Uh oh!

Uh oh!

AaronBallman commented Oct 30, 2023

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 25, 2023 •

edited

Loading