From 1c5bbe0290da0704e8b1579a8052457c2c06944b Mon Sep 17 00:00:00 2001 From: susmonteiro Date: Fri, 3 Oct 2025 14:03:35 +0100 Subject: [PATCH] [cxx-interop] Implicitly defined copy constructors --- include/swift/ClangImporter/ClangImporter.h | 6 ++ lib/ClangImporter/ClangImporter.cpp | 45 ++++++++++---- lib/ClangImporter/ImportDecl.cpp | 42 ++++++------- lib/ClangImporter/ImporterImpl.h | 5 ++ lib/IRGen/GenStruct.cpp | 23 ++++--- .../Cxx/class/noncopyable-typechecker.swift | 61 ++++++++++++++++++- .../Cxx/stdlib/Inputs/std-unique-ptr.h | 5 ++ .../use-std-unique-ptr-typechecker.swift | 6 ++ 8 files changed, 147 insertions(+), 46 deletions(-) diff --git a/include/swift/ClangImporter/ClangImporter.h b/include/swift/ClangImporter/ClangImporter.h index 2c6dcceb6662a..5fe44dc8a7afe 100644 --- a/include/swift/ClangImporter/ClangImporter.h +++ b/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 diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp index cf450dffd59e0..4bdafa8284847 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -8315,16 +8315,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; }); } @@ -8523,8 +8546,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)) { diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp index 9176ce2caedba..343a799bd27f5 100644 --- a/lib/ClangImporter/ImportDecl.cpp +++ b/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(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,13 +2284,9 @@ namespace { loc, ArrayRef(), 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)); + if (decl->isInStdNamespace() && decl->getName() == "promise" && recordHasMoveOnlySemantics(decl)) { + // Do not import std::promise. + return nullptr; } // FIXME: Figure out what to do with superclasses in C++. One possible @@ -2503,6 +2505,11 @@ 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 && @@ -3098,11 +3105,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(decl)); } if (decl->needsImplicitMoveConstructor()) { @@ -3123,10 +3129,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 +3139,6 @@ namespace { } } } - if (copyCtor && !isExplicitlyNonCopyable && - !decl->isAnonymousStructOrUnion()) { - clangSema.DefineImplicitCopyConstructor(clang::SourceLocation(), - copyCtor); - } if (moveCtor && !decl->isAnonymousStructOrUnion()) { clangSema.DefineImplicitMoveConstructor(clang::SourceLocation(), moveCtor); diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h index e17f4d8d20d6e..82d24be75bbf3 100644 --- a/lib/ClangImporter/ImporterImpl.h +++ b/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); diff --git a/lib/IRGen/GenStruct.cpp b/lib/IRGen/GenStruct.cpp index d870143d153ea..ff94867085d89 100644 --- a/lib/IRGen/GenStruct.cpp +++ b/lib/IRGen/GenStruct.cpp @@ -553,15 +553,7 @@ namespace { const auto *cxxRecordDecl = dyn_cast(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(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(copyConstructor)); + } + auto &diagEngine = importer->getClangSema().getDiagnostics(); clang::DiagnosticErrorTrap trap(diagEngine); auto clangFnAddr = diff --git a/test/Interop/Cxx/class/noncopyable-typechecker.swift b/test/Interop/Cxx/class/noncopyable-typechecker.swift index 58b2f814865a8..175c86d124d95 100644 --- a/test/Interop/Cxx/class/noncopyable-typechecker.swift +++ b/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 +struct TemplatedImplicitCopyConstructor { + T element; + P *pointer; + R &reference; +}; + +using NonCopyableT = TemplatedImplicitCopyConstructor; +using NonCopyableP = TemplatedImplicitCopyConstructor; +using NonCopyableR = TemplatedImplicitCopyConstructor; + +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 +struct TemplatedDefaultedCopyConstructor { + T element; + TemplatedDefaultedCopyConstructor(const TemplatedDefaultedCopyConstructor&) = default; + TemplatedDefaultedCopyConstructor(TemplatedDefaultedCopyConstructor&&) = default; +}; + +template +struct DerivedTemplatedDefaultedCopyConstructor : TemplatedDefaultedCopyConstructor {}; + +using CopyableDefaultedCopyConstructor = TemplatedDefaultedCopyConstructor; +using NonCopyableDefaultedCopyConstructor = TemplatedDefaultedCopyConstructor; +using CopyableDerived = DerivedTemplatedDefaultedCopyConstructor; +using NonCopyableDerived = DerivedTemplatedDefaultedCopyConstructor; + template struct SWIFT_COPYABLE_IF(T) DisposableContainer {}; struct POD { int x; float y; }; // special members are implicit, but should be copyable using DisposablePOD = DisposableContainer; // 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') 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') conform to 'Copyable'}} + takeCopyable(d3) + takeCopyable(d4) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableDerived' (aka 'DerivedTemplatedDefaultedCopyConstructor') conform to 'Copyable'}} +} + func copyableDisposablePOD(p: DisposablePOD) { takeCopyable(p) } diff --git a/test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h b/test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h index e6cc6bf0d5ca0..a307b569674f4 100644 --- a/test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h +++ b/test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h @@ -89,4 +89,9 @@ struct CountCopies { inline std::unique_ptr getCopyCountedUniquePtr() { return std::make_unique(); } +struct HasUniqueIntVector { + HasUniqueIntVector() = default; + std::vector> x; +}; + #endif // TEST_INTEROP_CXX_STDLIB_INPUTS_STD_UNIQUE_PTR_H diff --git a/test/Interop/Cxx/stdlib/use-std-unique-ptr-typechecker.swift b/test/Interop/Cxx/stdlib/use-std-unique-ptr-typechecker.swift index 636ba05653689..51e3945c7d854 100644 --- a/test/Interop/Cxx/stdlib/use-std-unique-ptr-typechecker.swift +++ b/test/Interop/Cxx/stdlib/use-std-unique-ptr-typechecker.swift @@ -10,3 +10,9 @@ func takeCopyable(_ 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' + +