[Clang] _default-movable_ should be based on the first declaration

[cor3ntin]

When the definition of a special member function was defaulted we would not consider it user-provided, even when the first declaration was not defaulted.

Fixes #143599

[llvmbot]

@llvm/pr-subscribers-clang

Author: Corentin Jabot (cor3ntin)

Changes

When the definition of a special member function was defaulted we would not consider it user-provided, even when the first declaration was not defaulted.

Fixes #143599

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaTypeTraits.cpp (+12-7)
• (modified) clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp (+21)
• (modified) clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp (+23)

diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index d663e5581093e..bba0099b6fcb5 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -121,8 +121,10 @@ static bool hasSuitableConstructorForRelocation(Sema &SemaRef,
 
   CXXMethodDecl *Decl =
       LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false);
-  return Decl && Decl->isUserProvided() == AllowUserDefined &&
-         !Decl->isDeleted();
+  if (!Decl)
+    return false;
+  Decl = Decl->getCanonicalDecl();
+  return Decl->isUserProvided() == AllowUserDefined && !Decl->isDeleted();
 }
 
 static bool hasSuitableMoveAssignmentOperatorForRelocation(
@@ -136,9 +138,8 @@ static bool hasSuitableMoveAssignmentOperatorForRelocation(
       LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/true);
   if (!Decl)
     return false;
-
-  return Decl && Decl->isUserProvided() == AllowUserDefined &&
-         !Decl->isDeleted();
+  Decl = Decl->getCanonicalDecl();
+  return Decl->isUserProvided() == AllowUserDefined && !Decl->isDeleted();
 }
 
 // [C++26][class.prop]
@@ -164,6 +165,8 @@ static bool IsDefaultMovable(Sema &SemaRef, const CXXRecordDecl *D) {
   if (!Dtr)
     return true;
 
+  Dtr = Dtr->getCanonicalDecl();
+
   if (Dtr->isUserProvided() && (!Dtr->isDefaulted() || Dtr->isDeleted()))
     return false;
 
@@ -2031,7 +2034,7 @@ static void DiagnoseNonDefaultMovable(Sema &SemaRef, SourceLocation Loc,
   if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) {
     const auto *Decl = cast_or_null<CXXConstructorDecl>(
         LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false));
-    if (Decl && Decl->isUserProvided())
+    if (Decl && Decl->getCanonicalDecl()->isUserProvided())
       SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
           << diag::TraitNotSatisfiedReason::UserProvidedCtr
           << Decl->isMoveConstructor() << Decl->getSourceRange();
@@ -2039,12 +2042,14 @@ static void DiagnoseNonDefaultMovable(Sema &SemaRef, SourceLocation Loc,
   if (!D->hasSimpleMoveAssignment() && !D->hasSimpleCopyAssignment()) {
     CXXMethodDecl *Decl =
         LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/true);
-    if (Decl && Decl->isUserProvided())
+    if (Decl && Decl->getCanonicalDecl()->isUserProvided())
       SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
           << diag::TraitNotSatisfiedReason::UserProvidedAssign
           << Decl->isMoveAssignmentOperator() << Decl->getSourceRange();
   }
   CXXDestructorDecl *Dtr = D->getDestructor();
+  if (Dtr)
+    Dtr = Dtr->getCanonicalDecl();
   if (Dtr && Dtr->isUserProvided() && !Dtr->isDefaulted())
     SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
         << diag::TraitNotSatisfiedReason::DeletedDtr << /*User Provided*/ 1
diff --git a/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp b/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
index aff172e0bc70a..9d43994ee7661 100644
--- a/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
+++ b/clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
@@ -388,3 +388,24 @@ void do_test__builtin_trivially_relocate() {
     // expected-note@-1 {{'test__builtin_trivially_relocate<S *, S *, int>' requested here}}
     // expected-error@#reloc1 {{first argument to '__builtin_trivially_relocate' must be relocatable}}
 }
+
+
+namespace GH143599 {
+struct A { ~A (); };
+A::~A () = default;
+
+static_assert (!__builtin_is_cpp_trivially_relocatable(A));
+static_assert (!__builtin_is_replaceable(A));
+
+struct B { B(const B&); };
+B::B (const B&) = default;
+
+static_assert (!__builtin_is_cpp_trivially_relocatable(B));
+static_assert (!__builtin_is_replaceable(B));
+
+struct C { C& operator=(const C&); };
+C& C::operator=(const C&) = default;
+
+static_assert (!__builtin_is_cpp_trivially_relocatable(C));
+static_assert (!__builtin_is_replaceable(C));
+}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
index 9e053034acda4..a8c78f6304ca9 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
@@ -320,6 +320,29 @@ static_assert(__builtin_is_cpp_trivially_relocatable(UnionOfPolymorphic));
 
 }
 
+struct GH143599 {  // expected-note 2 {{'GH143599' defined here}}
+    ~GH143599 ();
+     GH143599(const GH143599&);
+     GH143599& operator=(const GH143599&);
+};
+GH143599::~GH143599 () = default;
+GH143599::GH143599 (const GH143599&) = default;
+GH143599& GH143599::operator=(const GH143599&) = default;
+
+static_assert (__builtin_is_cpp_trivially_relocatable(GH143599));
+// expected-error@-1 {{static assertion failed due to requirement '__builtin_is_cpp_trivially_relocatable(GH143599)'}} \
+// expected-note@-1 {{'GH143599' is not trivially relocatable}} \
+// expected-note@-1 {{because it has a user provided copy constructor}} \
+// expected-note@-1 {{because it has a user provided copy assignment operator}} \
+// expected-note@-1 {{because it has a user-provided destructor}}
+
+static_assert (__builtin_is_replaceable(GH143599));
+// expected-error@-1 {{static assertion failed due to requirement '__builtin_is_replaceable(GH143599)'}} \
+// expected-note@-1 {{'GH143599' is not replaceable}} \
+// expected-note@-1 {{because it has a user provided copy constructor}} \
+// expected-note@-1 {{because it has a user provided copy assignment operator}} \
+// expected-note@-1 {{because it has a user-provided destructor}}
+
 namespace trivially_copyable {
 struct B {
  virtual ~B();

[ojhunt]

Is it reasonable to have LookupSpecialMemberFromXValue simply return the canonical declaration? Rather than every call site needing to remember that some Sema checks will require canonicalization?

It looks like you're having to perform canonicalization in 4 of the 6 calls to it, and I'm not sure why not in the remaining two? It seems like they're intended just for diagnostics, but it seems any diagnostic would need to be about the canonical decl as well?

[AaronBallman]

LGTM aside from a suggestion

[AaronBallman]

I think this is a bit clearer as:

if (const CXXDestructorDecl *Dtor = D->getDestructor()) {
  Dtor = Dtor->getCanonicalDecl();
  if (Dtr->isUserProvided() && !Dtr->isDefaulted()) {
    SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
        << diag::TraitNotSatisfiedReason::DeletedDtr << /*User Provided*/ 1
        << Dtr->getSourceRange();
  }
}

[erichkeane]

I'm disturbed at how much of these fixes are ending up being getCanonicalDecl sprinkled around. IMO we need to start being better about making the functions we use to 'check' things auto-canonicalizing.

[shafik]

It feels like having isDefaultedOnDeclaration() would be tremendously more readable and express the intent rather than doing "the dance" when necessary.