Revert "[cxx-interop] Implicitly defined copy constructors"

This reverts commit 1c5bbe0.

Author: susmonteiro

include/swift/ClangImporter/ClangImporter.h

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

@@ -8322,39 +8322,16 @@ bool importer::isViewType(const clang::CXXRecordDecl *decl) {
   return !hasOwnedValueAttr(decl) && hasPointerInSubobjects(decl);
 }
 
-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 hasCopyTypeOperations(const clang::CXXRecordDecl *decl,
-                                  ClangImporter::Implementation *importerImpl) {
-  if (!decl->hasSimpleCopyConstructor()) {
-    auto *copyCtor = findCopyConstructor(decl);
-    if (!copyCtor)
-      return false;
-
-    if (!copyCtor->isDefaulted())
-      return true;
-  }
+static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
+  if (decl->hasSimpleCopyConstructor())
+    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;
+  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;
   });
 }
 
@@ -8553,8 +8530,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
     return CxxValueSemanticsKind::Copyable;
   }
 
-  bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
-                    hasCopyTypeOperations(cxxRecordDecl, importerImpl);
+  bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) && 
+                    hasCopyTypeOperations(cxxRecordDecl);
   bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
 
   if (!hasDestroyTypeOperations(cxxRecordDecl) || (!isCopyable && !isMovable)) {

lib/ClangImporter/ImportDecl.cpp

@@ -186,15 +186,6 @@ 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))
@@ -2104,7 +2095,10 @@ namespace {
     }
 
     bool recordHasMoveOnlySemantics(const clang::RecordDecl *decl) {
-      return importer::recordHasMoveOnlySemantics(decl, &Impl);
+      auto semanticsKind = evaluateOrDefault(
+          Impl.SwiftContext.evaluator,
+          CxxValueSemantics({decl->getTypeForDecl(), &Impl}), {});
+      return semanticsKind == CxxValueSemanticsKind::MoveOnly;
     }
 
     void markReturnsUnsafeNonescapable(AbstractFunctionDecl *fd) {
@@ -2283,9 +2277,13 @@ namespace {
             loc, ArrayRef<InheritedEntry>(), nullptr, dc);
       Impl.ImportedDecls[{decl->getCanonicalDecl(), getVersion()}] = result;
 
-      if (decl->isInStdNamespace() && decl->getName() == "promise" && recordHasMoveOnlySemantics(decl)) {
-        // Do not import std::promise.
-        return nullptr;
+      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
@@ -2532,11 +2530,6 @@ namespace {
                                  cxxRecordDecl->ctors().empty());
       }
 
-      if (recordHasMoveOnlySemantics(decl)) {
-        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 &&
@@ -3132,10 +3125,11 @@ 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) {
-          clangSema.DeclareImplicitCopyConstructor(
+          copyCtor = clangSema.DeclareImplicitCopyConstructor(
               const_cast<clang::CXXRecordDecl *>(decl));
         }
         if (decl->needsImplicitMoveConstructor()) {
@@ -3156,7 +3150,10 @@ namespace {
                 // Note: we use "doesThisDeclarationHaveABody" here because
                 // that's what "DefineImplicitCopyConstructor" checks.
                 !declCtor->doesThisDeclarationHaveABody()) {
-              if (declCtor->isMoveConstructor()) {
+              if (declCtor->isCopyConstructor()) {
+                if (!copyCtor && !isExplicitlyNonCopyable)
+                  copyCtor = declCtor;
+              } else if (declCtor->isMoveConstructor()) {
                 if (!moveCtor)
                   moveCtor = declCtor;
               } else if (declCtor->isDefaultConstructor()) {
@@ -3166,6 +3163,11 @@ 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,11 +1974,6 @@ 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,7 +553,15 @@ namespace {
       const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
       if (!cxxRecordDecl)
         return nullptr;
-      return importer::findCopyConstructor(cxxRecordDecl);
+      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;
     }
 
     const clang::CXXConstructorDecl *findMoveConstructor() const {
@@ -621,18 +629,7 @@ 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 -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
+// 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
 
 //--- Inputs/module.modulemap
 module Test {
@@ -15,7 +15,7 @@ module Test {
 
 struct NonCopyable {
     NonCopyable() = default;
-    NonCopyable(const NonCopyable& other) = delete; // expected-note {{'NonCopyable' has been explicitly marked deleted here}}
+    NonCopyable(const NonCopyable& other) = delete;
     NonCopyable(NonCopyable&& other) = default;
 };
 
@@ -79,44 +79,6 @@ 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
@@ -171,23 +133,6 @@ 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,9 +89,4 @@ 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,9 +10,3 @@ 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'
-
-