[cxx-interop] Implicitly defined copy constructors

Author: susmonteiro

include/swift/ClangImporter/ClangImporter.h

@@ -733,6 +733,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

@@ -8290,16 +8290,39 @@ bool importer::isViewType(const clang::CXXRecordDecl *decl) {
   return !hasOwnedValueAttr(decl) && hasPointerInSubobjects(decl);
 }
 
-static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
-  if (decl->hasSimpleCopyConstructor())
-    return true;
+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;
+}
 
-  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;
+static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl,
+                                  ClangImporter::Implementation *importerImpl) {
+  if (!decl->hasSimpleCopyConstructor()) {
+    auto *copyCtor = findCopyConstructor(decl);
+    if (!copyCtor)
+      return false;
+
+    if (!copyCtor->isDefaulted())
+      return true;
+  }
+
+  // If the copy constructor is defaulted or implicitly declared, we should only
+  // import the type as copyable if all its fields are also copyable
+  // FIXME: we should also look at the bases
+  return llvm::none_of(decl->fields(), [&](clang::FieldDecl *field) {
+    if (const auto *rd = field->getType()->getAsRecordDecl()) {
+      return (!field->getType()->isReferenceType() &&
+              !field->getType()->isPointerType() &&
+              recordHasMoveOnlySemantics(rd, importerImpl));
+    }
+    return false;
   });
 }
 
@@ -8498,8 +8521,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
     return CxxValueSemanticsKind::Copyable;
   }
 
-  bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) && 
-                    hasCopyTypeOperations(cxxRecordDecl);
+  bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
+                    hasCopyTypeOperations(cxxRecordDecl, importerImpl);
   bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
 
   if (!hasDestroyTypeOperations(cxxRecordDecl) || (!isCopyable && !isMovable)) {

lib/ClangImporter/ImportDecl.cpp

@@ -187,6 +187,15 @@ bool importer::recordHasReferenceSemantics(
   return semanticsKind == CxxRecordSemanticsKind::Reference;
 }
 
+bool importer::recordHasMoveOnlySemantics(
+    const clang::RecordDecl *decl,
+    ClangImporter::Implementation *importerImpl) {
+  auto semanticsKind = evaluateOrDefault(
+      importerImpl->SwiftContext.evaluator,
+      CxxValueSemantics({decl->getTypeForDecl(), importerImpl}), {});
+  return semanticsKind == CxxValueSemanticsKind::MoveOnly;
+}
+
 bool importer::hasImmortalAttrs(const clang::RecordDecl *decl) {
   return decl->hasAttrs() && llvm::any_of(decl->getAttrs(), [](auto *attr) {
            if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr))
@@ -2096,10 +2105,7 @@ namespace {
     }
 
     bool recordHasMoveOnlySemantics(const clang::RecordDecl *decl) {
-      auto semanticsKind = evaluateOrDefault(
-          Impl.SwiftContext.evaluator,
-          CxxValueSemantics({decl->getTypeForDecl(), &Impl}), {});
-      return semanticsKind == CxxValueSemanticsKind::MoveOnly;
+      return importer::recordHasMoveOnlySemantics(decl, &Impl);
     }
 
     void markReturnsUnsafeNonescapable(AbstractFunctionDecl *fd) {
@@ -2278,15 +2284,6 @@ namespace {
             loc, ArrayRef<InheritedEntry>(), nullptr, dc);
       Impl.ImportedDecls[{decl->getCanonicalDecl(), getVersion()}] = result;
 
-      if (recordHasMoveOnlySemantics(decl)) {
-        if (decl->isInStdNamespace() && decl->getName() == "promise") {
-          // Do not import std::promise.
-          return nullptr;
-        }
-        result->addAttribute(new (Impl.SwiftContext)
-                                 MoveOnlyAttr(/*Implicit=*/true));
-      }
-
       // FIXME: Figure out what to do with superclasses in C++. One possible
       // solution would be to turn them into members and add conversion
       // functions.
@@ -2503,6 +2500,15 @@ namespace {
                                  cxxRecordDecl->ctors().empty());
       }
 
+      if (recordHasMoveOnlySemantics(decl)) {
+        if (decl->isInStdNamespace() && decl->getName() == "promise") {
+          // Do not import std::promise.
+          return nullptr;
+        }
+        result->getAttrs().add(new (Impl.SwiftContext)
+                                   MoveOnlyAttr(/*Implicit=*/true));
+      }
+
       // TODO: builtin "zeroInitializer" does not work with non-escapable
       // types yet. Don't generate an initializer.
       if (hasZeroInitializableStorage && needsEmptyInitializer &&
@@ -3098,11 +3104,10 @@ 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()) {
@@ -3123,10 +3128,7 @@ 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 (declCtor->isMoveConstructor()) {
                 if (!moveCtor)
                   moveCtor = declCtor;
               } else if (declCtor->isDefaultConstructor()) {
@@ -3136,11 +3138,6 @@ namespace {
             }
           }
         }
-        if (copyCtor && !isExplicitlyNonCopyable &&
-            !decl->isAnonymousStructOrUnion()) {
-          clangSema.DefineImplicitCopyConstructor(clang::SourceLocation(),
-                                                  copyCtor);
-        }
         if (moveCtor && !decl->isAnonymousStructOrUnion()) {
           clangSema.DefineImplicitMoveConstructor(clang::SourceLocation(),
                                                   moveCtor);

lib/ClangImporter/ImporterImpl.h

@@ -1974,6 +1974,11 @@ namespace importer {
 bool recordHasReferenceSemantics(const clang::RecordDecl *decl,
                                  ClangImporter::Implementation *importerImpl);
 
+/// Returns true if the given C/C++ record should be imported as non-copyable into
+/// Swift.
+bool recordHasMoveOnlySemantics(const clang::RecordDecl *decl,
+                                ClangImporter::Implementation *importerImpl);
+
 /// Whether this is a forward declaration of a type. We ignore forward
 /// declarations in certain cases, and instead process the real declarations.
 bool isForwardDeclOfType(const clang::Decl *decl);

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 {
@@ -629,7 +621,18 @@ namespace {
 
       auto &ctx = IGF.IGM.Context;
       auto *importer = static_cast<ClangImporter *>(ctx.getClangModuleLoader());
-      
+
+      if (copyConstructor->isDefaulted() &&
+          copyConstructor->getAccess() == clang::AS_public &&
+          !copyConstructor->isDeleted() &&
+          // Note: we use "doesThisDeclarationHaveABody" here because
+          // that's what "DefineImplicitCopyConstructor" checks.
+          !copyConstructor->doesThisDeclarationHaveABody()) {
+        importer->getClangSema().DefineImplicitCopyConstructor(
+            clang::SourceLocation(),
+            const_cast<clang::CXXConstructorDecl *>(copyConstructor));
+      }
+
       auto &diagEngine = importer->getClangSema().getDiagnostics();
       clang::DiagnosticErrorTrap trap(diagEngine);
       auto clangFnAddr =

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 {
@@ -15,7 +15,7 @@ module Test {
 
 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,6 +79,44 @@ 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
@@ -133,6 +171,23 @@ 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)
 }

test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h

@@ -89,4 +89,9 @@ struct CountCopies {
 
 inline std::unique_ptr<CountCopies> getCopyCountedUniquePtr() { return std::make_unique<CountCopies>(); }
 
+struct HasUniqueIntVector {
+  HasUniqueIntVector() = default;
+  std::vector<std::unique_ptr<int>> x;
+};
+
 #endif // TEST_INTEROP_CXX_STDLIB_INPUTS_STD_UNIQUE_PTR_H

test/Interop/Cxx/stdlib/use-std-unique-ptr-typechecker.swift

@@ -10,3 +10,9 @@ func takeCopyable<T: Copyable>(_ x: T) {}
 let vecUniquePtr = getVectorNonCopyableUniquePtr()
 takeCopyable(vecUniquePtr)
 // CHECK: error: global function 'takeCopyable' requires that 'std{{.*}}vector{{.*}}unique_ptr{{.*}}NonCopyable{{.*}}' conform to 'Copyable'
+
+let uniqueIntVec = HasUniqueIntVector()
+takeCopyable(uniqueIntVec)
+// CHECK: error: global function 'takeCopyable' requires that 'HasUniqueIntVector' conform to 'Copyable'
+
+