[cxx-interop] Reapply implicitly defined copy and move constructors

include/swift/ClangImporter/ClangImporter.h

@@ -737,6 +737,12 @@ ValueDecl *getImportedMemberOperator(const DeclBaseName &name,
 /// as permissive as the input C++ access.
 AccessLevel convertClangAccess(clang::AccessSpecifier access);
 
+/// Lookup and return the copy constructor of \a decl
+///
+/// Returns nullptr if \a decl doesn't have a valid copy constructor
+const clang::CXXConstructorDecl *
+findCopyConstructor(const clang::CXXRecordDecl *decl);
+
 /// Read file IDs from 'private_fileid' Swift attributes on a Clang decl.
 ///
 /// May return >1 fileID when a decl is annotated more than once, which should

lib/ClangImporter/ClangImporter.cpp

@@ -8342,17 +8342,16 @@ bool importer::isViewType(const clang::CXXRecordDecl *decl) {
   return !hasOwnedValueAttr(decl) && hasPointerInSubobjects(decl);
 }
 
-static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
-  if (decl->hasSimpleCopyConstructor())
-    return true;
-
-  return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
-    return ctor->isCopyConstructor() && !ctor->isDeleted() &&
-           !ctor->isIneligibleOrNotSelected() &&
-           // FIXME: Support default arguments (rdar://142414553)
-           ctor->getNumParams() == 1 &&
-           ctor->getAccess() == clang::AccessSpecifier::AS_public;
-  });
+const clang::CXXConstructorDecl *
+importer::findCopyConstructor(const clang::CXXRecordDecl *decl) {
+  for (auto ctor : decl->ctors()) {
+    if (ctor->isCopyConstructor() &&
+        // FIXME: Support default arguments (rdar://142414553)
+        ctor->getNumParams() == 1 && ctor->getAccess() == clang::AS_public &&
+        !ctor->isDeleted() && !ctor->isIneligibleOrNotSelected())
+      return ctor;
+  }
+  return nullptr;
 }
 
 static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
@@ -8520,8 +8519,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
   while (!stack.empty()) {
     const clang::RecordDecl *recordDecl = stack.back();
     stack.pop_back();
-
-    if (!hasNonCopyableAttr(recordDecl)) {
+    bool isExplicitlyNonCopyable = hasNonCopyableAttr(recordDecl);
+    if (!isExplicitlyNonCopyable) {
       auto injectedStlAnnotation =
           recordDecl->isInStdNamespace()
               ? STLConditionalParams.find(recordDecl->getName())
@@ -8548,13 +8547,28 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
 
     const auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);
     if (!cxxRecordDecl || !recordDecl->isCompleteDefinition()) {
-      if (hasNonCopyableAttr(recordDecl))
+      if (isExplicitlyNonCopyable)
         return CxxValueSemanticsKind::MoveOnly;
       continue;
     }
 
-    bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
-                      hasCopyTypeOperations(cxxRecordDecl);
+    bool isCopyable = !isExplicitlyNonCopyable;
+    if (!isExplicitlyNonCopyable) {
+      auto copyCtor = findCopyConstructor(cxxRecordDecl);
+      isCopyable = copyCtor || cxxRecordDecl->needsImplicitCopyConstructor();
+      if ((copyCtor && copyCtor->isDefaulted()) ||
+          cxxRecordDecl->needsImplicitCopyConstructor()) {
+        // If the copy constructor is implicit/defaulted, we ask Clang to
+        // generate its definition. The implicitly-defined copy constructor
+        // performs full member-wise copy. Thus, if any member of this type is
+        // ~Copyable, the type should also be ~Copyable.
+        for (auto field : cxxRecordDecl->fields())
+          maybePushToStack(field->getType()->getUnqualifiedDesugaredType());
+        for (auto base : cxxRecordDecl->bases())
+          maybePushToStack(base.getType()->getUnqualifiedDesugaredType());
+      }
+    }
+
     bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
 
     if (!hasDestroyTypeOperations(cxxRecordDecl) ||
@@ -8568,7 +8582,7 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
       continue;
     }
 
-    if (hasNonCopyableAttr(cxxRecordDecl) && isMovable)
+    if (isExplicitlyNonCopyable && isMovable)
       return CxxValueSemanticsKind::MoveOnly;
 
     if (isCopyable)

lib/ClangImporter/ImportDecl.cpp

@@ -3106,15 +3106,13 @@ namespace {
         // instantiate its copy constructor.
         bool isExplicitlyNonCopyable = hasNonCopyableAttr(decl);
 
-        clang::CXXConstructorDecl *copyCtor = nullptr;
-        clang::CXXConstructorDecl *moveCtor = nullptr;
         clang::CXXConstructorDecl *defaultCtor = nullptr;
         if (decl->needsImplicitCopyConstructor() && !isExplicitlyNonCopyable) {
-          copyCtor = clangSema.DeclareImplicitCopyConstructor(
+          clangSema.DeclareImplicitCopyConstructor(
               const_cast<clang::CXXRecordDecl *>(decl));
         }
         if (decl->needsImplicitMoveConstructor()) {
-          moveCtor = clangSema.DeclareImplicitMoveConstructor(
+          clangSema.DeclareImplicitMoveConstructor(
               const_cast<clang::CXXRecordDecl *>(decl));
         }
         if (decl->needsImplicitDefaultConstructor()) {
@@ -3131,28 +3129,13 @@ namespace {
                 // Note: we use "doesThisDeclarationHaveABody" here because
                 // that's what "DefineImplicitCopyConstructor" checks.
                 !declCtor->doesThisDeclarationHaveABody()) {
-              if (declCtor->isCopyConstructor()) {
-                if (!copyCtor && !isExplicitlyNonCopyable)
-                  copyCtor = declCtor;
-              } else if (declCtor->isMoveConstructor()) {
-                if (!moveCtor)
-                  moveCtor = declCtor;
-              } else if (declCtor->isDefaultConstructor()) {
+              if (declCtor->isDefaultConstructor()) {
                 if (!defaultCtor)
                   defaultCtor = declCtor;
               }
             }
           }
         }
-        if (copyCtor && !isExplicitlyNonCopyable &&
-            !decl->isAnonymousStructOrUnion()) {
-          clangSema.DefineImplicitCopyConstructor(clang::SourceLocation(),
-                                                  copyCtor);
-        }
-        if (moveCtor && !decl->isAnonymousStructOrUnion()) {
-          clangSema.DefineImplicitMoveConstructor(clang::SourceLocation(),
-                                                  moveCtor);
-        }
         if (defaultCtor) {
           clangSema.DefineImplicitDefaultConstructor(clang::SourceLocation(),
                                                      defaultCtor);
@@ -3161,7 +3144,8 @@ namespace {
         if (decl->needsImplicitDestructor()) {
           auto dtor = clangSema.DeclareImplicitDestructor(
               const_cast<clang::CXXRecordDecl *>(decl));
-          clangSema.DefineImplicitDestructor(clang::SourceLocation(), dtor);
+          if (!dtor->isDeleted() && !dtor->isIneligibleOrNotSelected())
+            clangSema.DefineImplicitDestructor(clang::SourceLocation(), dtor);
         }
       }
 

lib/IRGen/GenStruct.cpp

@@ -553,15 +553,7 @@ namespace {
       const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
       if (!cxxRecordDecl)
         return nullptr;
-      for (auto ctor : cxxRecordDecl->ctors()) {
-        if (ctor->isCopyConstructor() &&
-            // FIXME: Support default arguments (rdar://142414553)
-            ctor->getNumParams() == 1 &&
-            ctor->getAccess() == clang::AS_public && !ctor->isDeleted() &&
-            !ctor->isIneligibleOrNotSelected())
-          return ctor;
-      }
-      return nullptr;
+      return importer::findCopyConstructor(cxxRecordDecl);
     }
 
     const clang::CXXConstructorDecl *findMoveConstructor() const {
@@ -619,7 +611,7 @@ namespace {
           /*invocation subs*/ SubstitutionMap(), IGF.IGM.Context);
     }
 
-    void emitCopyWithCopyConstructor(
+    void emitCopyWithCopyOrMoveConstructor(
         IRGenFunction &IGF, SILType T,
         const clang::CXXConstructorDecl *copyConstructor, llvm::Value *src,
         llvm::Value *dest) const {
@@ -629,7 +621,27 @@ namespace {
 
       auto &ctx = IGF.IGM.Context;
       auto *importer = static_cast<ClangImporter *>(ctx.getClangModuleLoader());
-
+
+      if (copyConstructor->isDefaulted() &&
+          copyConstructor->getAccess() == clang::AS_public &&
+          !copyConstructor->isDeleted() &&
+          !copyConstructor->isIneligibleOrNotSelected() &&
+          // Note: we use "doesThisDeclarationHaveABody" here because
+          // that's what "DefineImplicitCopyConstructor" checks.
+          !copyConstructor->doesThisDeclarationHaveABody()) {
+        assert(!copyConstructor->getParent()->isAnonymousStructOrUnion() &&
+               "Cannot do codegen of special member functions of anonymous "
+               "structs/unions");
+        if (copyConstructor->isCopyConstructor())
+          importer->getClangSema().DefineImplicitCopyConstructor(
+              clang::SourceLocation(),
+              const_cast<clang::CXXConstructorDecl *>(copyConstructor));
+        else
+          importer->getClangSema().DefineImplicitMoveConstructor(
+              clang::SourceLocation(),
+              const_cast<clang::CXXConstructorDecl *>(copyConstructor));
+      }
+
       auto &diagEngine = importer->getClangSema().getDiagnostics();
       clang::DiagnosticErrorTrap trap(diagEngine);
       auto clangFnAddr =
@@ -809,9 +821,9 @@ namespace {
                             Address srcAddr, SILType T,
                             bool isOutlined) const override {
       if (auto copyConstructor = findCopyConstructor()) {
-        emitCopyWithCopyConstructor(IGF, T, copyConstructor,
-                                    srcAddr.getAddress(),
-                                    destAddr.getAddress());
+        emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
+                                          srcAddr.getAddress(),
+                                          destAddr.getAddress());
         return;
       }
       StructTypeInfoBase<AddressOnlyCXXClangRecordTypeInfo, FixedTypeInfo,
@@ -824,9 +836,9 @@ namespace {
                         SILType T, bool isOutlined) const override {
       if (auto copyConstructor = findCopyConstructor()) {
         destroy(IGF, destAddr, T, isOutlined);
-        emitCopyWithCopyConstructor(IGF, T, copyConstructor,
-                                    srcAddr.getAddress(),
-                                    destAddr.getAddress());
+        emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
+                                          srcAddr.getAddress(),
+                                          destAddr.getAddress());
         return;
       }
       StructTypeInfoBase<AddressOnlyCXXClangRecordTypeInfo, FixedTypeInfo,
@@ -838,17 +850,15 @@ namespace {
                             SILType T, bool isOutlined,
                             bool zeroizeIfSensitive) const override {
       if (auto moveConstructor = findMoveConstructor()) {
-        emitCopyWithCopyConstructor(IGF, T, moveConstructor,
-                                    src.getAddress(),
-                                    dest.getAddress());
+        emitCopyWithCopyOrMoveConstructor(IGF, T, moveConstructor,
+                                          src.getAddress(), dest.getAddress());
         destroy(IGF, src, T, isOutlined);
         return;
       }
 
       if (auto copyConstructor = findCopyConstructor()) {
-        emitCopyWithCopyConstructor(IGF, T, copyConstructor,
-                                    src.getAddress(),
-                                    dest.getAddress());
+        emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
+                                          src.getAddress(), dest.getAddress());
         destroy(IGF, src, T, isOutlined);
         return;
       }
@@ -862,18 +872,16 @@ namespace {
                         bool isOutlined) const override {
       if (auto moveConstructor = findMoveConstructor()) {
         destroy(IGF, dest, T, isOutlined);
-        emitCopyWithCopyConstructor(IGF, T, moveConstructor,
-                                    src.getAddress(),
-                                    dest.getAddress());
+        emitCopyWithCopyOrMoveConstructor(IGF, T, moveConstructor,
+                                          src.getAddress(), dest.getAddress());
         destroy(IGF, src, T, isOutlined);
         return;
       }
 
       if (auto copyConstructor = findCopyConstructor()) {
         destroy(IGF, dest, T, isOutlined);
-        emitCopyWithCopyConstructor(IGF, T, copyConstructor,
-                                    src.getAddress(),
-                                    dest.getAddress());
+        emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
+                                          src.getAddress(), dest.getAddress());
         destroy(IGF, src, T, isOutlined);
         return;
       }

test/Interop/Cxx/class/noncopyable-typechecker.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
-// RUN: %target-swift-frontend -cxx-interoperability-mode=default -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t%{fs-sep}Inputs %t%{fs-sep}test.swift -verify-additional-file %t%{fs-sep}Inputs%{fs-sep}noncopyable.h
-// RUN: %target-swift-frontend -cxx-interoperability-mode=default -Xcc -std=c++20 -verify-additional-prefix cpp20- -D CPP20 -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t%{fs-sep}Inputs %t%{fs-sep}test.swift -verify-additional-file %t%{fs-sep}Inputs%{fs-sep}noncopyable.h
+// RUN: %target-swift-frontend -cxx-interoperability-mode=default -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t%{fs-sep}Inputs %t%{fs-sep}test.swift -verify-additional-file %t%{fs-sep}Inputs%{fs-sep}noncopyable.h -verify-ignore-unrelated
+// RUN: %target-swift-frontend -cxx-interoperability-mode=default -Xcc -std=c++20 -verify-additional-prefix cpp20- -D CPP20 -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t%{fs-sep}Inputs %t%{fs-sep}test.swift -verify-additional-file %t%{fs-sep}Inputs%{fs-sep}noncopyable.h -verify-ignore-unrelated
 
 //--- Inputs/module.modulemap
 module Test {
@@ -12,10 +12,11 @@ module Test {
 //--- Inputs/noncopyable.h
 #include "swift/bridging"
 #include <string>
+#include <vector>
 
 struct NonCopyable {
     NonCopyable() = default;
-    NonCopyable(const NonCopyable& other) = delete;
+    NonCopyable(const NonCopyable& other) = delete; // expected-note {{'NonCopyable' has been explicitly marked deleted here}}
     NonCopyable(NonCopyable&& other) = default;
 };
 
@@ -79,10 +80,63 @@ struct SWIFT_NONCOPYABLE NonCopyableNonMovable { // expected-note {{record 'NonC
     NonCopyableNonMovable(NonCopyableNonMovable&& other) = delete;
 };
 
+struct ImplicitCopyConstructor {
+  NonCopyable element;
+};
+
+template <typename T, typename P, typename R>
+struct TemplatedImplicitCopyConstructor {
+  T element;
+  P *pointer;
+  R &reference;
+};
+
+using NonCopyableT = TemplatedImplicitCopyConstructor<NonCopyable, int, int>;
+using NonCopyableP = TemplatedImplicitCopyConstructor<int, NonCopyable, int>;
+using NonCopyableR = TemplatedImplicitCopyConstructor<int, int, NonCopyable>;
+
+struct DefaultedCopyConstructor {
+  NonCopyable element; // expected-note {{copy constructor of 'DefaultedCopyConstructor' is implicitly deleted because field 'element' has a deleted copy constructor}}
+  DefaultedCopyConstructor(const DefaultedCopyConstructor&) = default; 
+  // expected-warning@-1 {{explicitly defaulted copy constructor is implicitly deleted}}
+  // expected-note@-2 {{replace 'default' with 'delete'}}
+  DefaultedCopyConstructor(DefaultedCopyConstructor&&) = default;
+};
+
+template<typename T>
+struct TemplatedDefaultedCopyConstructor {
+  T element;
+  TemplatedDefaultedCopyConstructor(const TemplatedDefaultedCopyConstructor&) = default;
+  TemplatedDefaultedCopyConstructor(TemplatedDefaultedCopyConstructor&&) = default;
+};
+
+template<typename T>
+struct DerivedTemplatedDefaultedCopyConstructor : TemplatedDefaultedCopyConstructor<T> {};
+
+using CopyableDefaultedCopyConstructor = TemplatedDefaultedCopyConstructor<NonCopyableP>;
+using NonCopyableDefaultedCopyConstructor = TemplatedDefaultedCopyConstructor<NonCopyable>;
+using CopyableDerived = DerivedTemplatedDefaultedCopyConstructor<NonCopyableR>;
+using NonCopyableDerived = DerivedTemplatedDefaultedCopyConstructor<NonCopyable>;
+
 template<typename T> struct SWIFT_COPYABLE_IF(T) DisposableContainer {};
 struct POD { int x; float y; }; // special members are implicit, but should be copyable
 using DisposablePOD = DisposableContainer<POD>; // should also be copyable
 
+struct DerivesFromMe : MyPair<DisposableContainer<DerivesFromMe>, std::vector<NonCopyable>> {};
+struct DerivesFromMeToo : MyPair<std::vector<NonCopyable>, DisposableContainer<DerivesFromMe>> {};
+
+template <typename T>
+struct OneField {
+    T field;
+};
+
+template <typename F, typename S>
+struct SWIFT_COPYABLE_IF(F, S) NoFields {};
+
+struct FieldDependsOnMe { // used to trigger a request cycle
+  OneField<NoFields<FieldDependsOnMe, NonCopyable>> field;
+};
+
 //--- test.swift
 import Test
 import CxxStdlib
@@ -133,6 +187,34 @@ func missingLifetimeOperation() {
     takeCopyable(s)
 }
 
+func implicitCopyConstructor(i: borrowing ImplicitCopyConstructor, t: borrowing NonCopyableT, p: borrowing NonCopyableP, r: borrowing NonCopyableR) {
+  takeCopyable(i) // expected-error {{global function 'takeCopyable' requires that 'ImplicitCopyConstructor' conform to 'Copyable'}}
+  takeCopyable(t) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableT' (aka 'TemplatedImplicitCopyConstructor<NonCopyable, CInt, CInt>') conform to 'Copyable'}}
+
+  // References and pointers to non-copyable types are still copyable
+  takeCopyable(p)
+  takeCopyable(r)
+}
+
+func defaultCopyConstructor(d: borrowing DefaultedCopyConstructor, d1: borrowing CopyableDefaultedCopyConstructor, d2: borrowing NonCopyableDefaultedCopyConstructor, d3: borrowing CopyableDerived, d4: borrowing NonCopyableDerived) {
+  takeCopyable(d) // expected-error {{global function 'takeCopyable' requires that 'DefaultedCopyConstructor' conform to 'Copyable'}}
+  takeCopyable(d1)
+  takeCopyable(d2) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableDefaultedCopyConstructor' (aka 'TemplatedDefaultedCopyConstructor<NonCopyable>') conform to 'Copyable'}}
+  takeCopyable(d3)
+  takeCopyable(d4) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableDerived' (aka 'DerivedTemplatedDefaultedCopyConstructor<NonCopyable>') conform to 'Copyable'}}
+}
+
 func copyableDisposablePOD(p: DisposablePOD) {
   takeCopyable(p)
 }
+
+func couldCreateCycleOfCxxValueSemanticsRequests() {
+  let d1 = DerivesFromMe()
+  takeCopyable(d1) // expected-error {{global function 'takeCopyable' requires that 'DerivesFromMe' conform to 'Copyable'}}
+
+  let d2 = DerivesFromMeToo()
+  takeCopyable(d2) // expected-error {{global function 'takeCopyable' requires that 'DerivesFromMeToo' conform to 'Copyable'}}
+
+  let d3 = FieldDependsOnMe()
+  takeCopyable(d3) // expected-error {{global function 'takeCopyable' requires that 'FieldDependsOnMe' conform to 'Copyable'}}
+}