Skip to content

Commit fd6e6b3

Browse files
susmonteirosusmonteiro
authored
Merge pull request #85642 from susmonteiro/susmonteiro/reapply-implicit-constructors
[cxx-interop] Reapply implicitly defined copy and move constructors
2 parents ce082bc + fbfcd4d commit fd6e6b3

File tree

12 files changed

+238
-70
lines changed

12 files changed

+238
-70
lines changed

‎include/swift/ClangImporter/ClangImporter.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -737,6 +737,12 @@ ValueDecl *getImportedMemberOperator(const DeclBaseName &name,
737737
/// as permissive as the input C++ access.
738738
AccessLevel convertClangAccess(clang::AccessSpecifier access);
739739

740+
/// Lookup and return the copy constructor of \a decl
741+
///
742+
/// Returns nullptr if \a decl doesn't have a valid copy constructor
743+
const clang::CXXConstructorDecl *
744+
findCopyConstructor(const clang::CXXRecordDecl *decl);
745+
740746
/// Read file IDs from 'private_fileid' Swift attributes on a Clang decl.
741747
///
742748
/// May return >1 fileID when a decl is annotated more than once, which should

‎lib/ClangImporter/ClangImporter.cpp‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8342,17 +8342,16 @@ bool importer::isViewType(const clang::CXXRecordDecl *decl) {
83428342
return !hasOwnedValueAttr(decl) && hasPointerInSubobjects(decl);
83438343
}
83448344

8345-
static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
8346-
if (decl->hasSimpleCopyConstructor())
8347-
return true;
8348-
8349-
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8350-
return ctor->isCopyConstructor() && !ctor->isDeleted() &&
8351-
!ctor->isIneligibleOrNotSelected() &&
8352-
// FIXME: Support default arguments (rdar://142414553)
8353-
ctor->getNumParams() == 1 &&
8354-
ctor->getAccess() == clang::AccessSpecifier::AS_public;
8355-
});
8345+
const clang::CXXConstructorDecl *
8346+
importer::findCopyConstructor(const clang::CXXRecordDecl *decl) {
8347+
for (auto ctor : decl->ctors()) {
8348+
if (ctor->isCopyConstructor() &&
8349+
// FIXME: Support default arguments (rdar://142414553)
8350+
ctor->getNumParams() == 1 && ctor->getAccess() == clang::AS_public &&
8351+
!ctor->isDeleted() && !ctor->isIneligibleOrNotSelected())
8352+
return ctor;
8353+
}
8354+
return nullptr;
83568355
}
83578356

83588357
static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
@@ -8520,8 +8519,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
85208519
while (!stack.empty()) {
85218520
const clang::RecordDecl *recordDecl = stack.back();
85228521
stack.pop_back();
8523-
8524-
if (!hasNonCopyableAttr(recordDecl)) {
8522+
bool isExplicitlyNonCopyable = hasNonCopyableAttr(recordDecl);
8523+
if (!isExplicitlyNonCopyable) {
85258524
auto injectedStlAnnotation =
85268525
recordDecl->isInStdNamespace()
85278526
? STLConditionalParams.find(recordDecl->getName())
@@ -8548,13 +8547,28 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
85488547

85498548
const auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);
85508549
if (!cxxRecordDecl || !recordDecl->isCompleteDefinition()) {
8551-
if (hasNonCopyableAttr(recordDecl))
8550+
if (isExplicitlyNonCopyable)
85528551
return CxxValueSemanticsKind::MoveOnly;
85538552
continue;
85548553
}
85558554

8556-
bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
8557-
hasCopyTypeOperations(cxxRecordDecl);
8555+
bool isCopyable = !isExplicitlyNonCopyable;
8556+
if (!isExplicitlyNonCopyable) {
8557+
auto copyCtor = findCopyConstructor(cxxRecordDecl);
8558+
isCopyable = copyCtor || cxxRecordDecl->needsImplicitCopyConstructor();
8559+
if ((copyCtor && copyCtor->isDefaulted()) ||
8560+
cxxRecordDecl->needsImplicitCopyConstructor()) {
8561+
// If the copy constructor is implicit/defaulted, we ask Clang to
8562+
// generate its definition. The implicitly-defined copy constructor
8563+
// performs full member-wise copy. Thus, if any member of this type is
8564+
// ~Copyable, the type should also be ~Copyable.
8565+
for (auto field : cxxRecordDecl->fields())
8566+
maybePushToStack(field->getType()->getUnqualifiedDesugaredType());
8567+
for (auto base : cxxRecordDecl->bases())
8568+
maybePushToStack(base.getType()->getUnqualifiedDesugaredType());
8569+
}
8570+
}
8571+
85588572
bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
85598573

85608574
if (!hasDestroyTypeOperations(cxxRecordDecl) ||
@@ -8568,7 +8582,7 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
85688582
continue;
85698583
}
85708584

8571-
if (hasNonCopyableAttr(cxxRecordDecl) && isMovable)
8585+
if (isExplicitlyNonCopyable && isMovable)
85728586
return CxxValueSemanticsKind::MoveOnly;
85738587

85748588
if (isCopyable)

‎lib/ClangImporter/ImportDecl.cpp‎

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3106,15 +3106,13 @@ namespace {
31063106
// instantiate its copy constructor.
31073107
bool isExplicitlyNonCopyable = hasNonCopyableAttr(decl);
31083108

3109-
clang::CXXConstructorDecl *copyCtor = nullptr;
3110-
clang::CXXConstructorDecl *moveCtor = nullptr;
31113109
clang::CXXConstructorDecl *defaultCtor = nullptr;
31123110
if (decl->needsImplicitCopyConstructor() && !isExplicitlyNonCopyable) {
3113-
copyCtor = clangSema.DeclareImplicitCopyConstructor(
3111+
clangSema.DeclareImplicitCopyConstructor(
31143112
const_cast<clang::CXXRecordDecl *>(decl));
31153113
}
31163114
if (decl->needsImplicitMoveConstructor()) {
3117-
moveCtor = clangSema.DeclareImplicitMoveConstructor(
3115+
clangSema.DeclareImplicitMoveConstructor(
31183116
const_cast<clang::CXXRecordDecl *>(decl));
31193117
}
31203118
if (decl->needsImplicitDefaultConstructor()) {
@@ -3131,28 +3129,13 @@ namespace {
31313129
// Note: we use "doesThisDeclarationHaveABody" here because
31323130
// that's what "DefineImplicitCopyConstructor" checks.
31333131
!declCtor->doesThisDeclarationHaveABody()) {
3134-
if (declCtor->isCopyConstructor()) {
3135-
if (!copyCtor && !isExplicitlyNonCopyable)
3136-
copyCtor = declCtor;
3137-
} else if (declCtor->isMoveConstructor()) {
3138-
if (!moveCtor)
3139-
moveCtor = declCtor;
3140-
} else if (declCtor->isDefaultConstructor()) {
3132+
if (declCtor->isDefaultConstructor()) {
31413133
if (!defaultCtor)
31423134
defaultCtor = declCtor;
31433135
}
31443136
}
31453137
}
31463138
}
3147-
if (copyCtor && !isExplicitlyNonCopyable &&
3148-
!decl->isAnonymousStructOrUnion()) {
3149-
clangSema.DefineImplicitCopyConstructor(clang::SourceLocation(),
3150-
copyCtor);
3151-
}
3152-
if (moveCtor && !decl->isAnonymousStructOrUnion()) {
3153-
clangSema.DefineImplicitMoveConstructor(clang::SourceLocation(),
3154-
moveCtor);
3155-
}
31563139
if (defaultCtor) {
31573140
clangSema.DefineImplicitDefaultConstructor(clang::SourceLocation(),
31583141
defaultCtor);
@@ -3161,7 +3144,8 @@ namespace {
31613144
if (decl->needsImplicitDestructor()) {
31623145
auto dtor = clangSema.DeclareImplicitDestructor(
31633146
const_cast<clang::CXXRecordDecl *>(decl));
3164-
clangSema.DefineImplicitDestructor(clang::SourceLocation(), dtor);
3147+
if (!dtor->isDeleted() && !dtor->isIneligibleOrNotSelected())
3148+
clangSema.DefineImplicitDestructor(clang::SourceLocation(), dtor);
31653149
}
31663150
}
31673151

‎lib/IRGen/GenStruct.cpp‎

Lines changed: 37 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -553,15 +553,7 @@ namespace {
553553
const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
554554
if (!cxxRecordDecl)
555555
return nullptr;
556-
for (auto ctor : cxxRecordDecl->ctors()) {
557-
if (ctor->isCopyConstructor() &&
558-
// FIXME: Support default arguments (rdar://142414553)
559-
ctor->getNumParams() == 1 &&
560-
ctor->getAccess() == clang::AS_public && !ctor->isDeleted() &&
561-
!ctor->isIneligibleOrNotSelected())
562-
return ctor;
563-
}
564-
return nullptr;
556+
return importer::findCopyConstructor(cxxRecordDecl);
565557
}
566558

567559
const clang::CXXConstructorDecl *findMoveConstructor() const {
@@ -619,7 +611,7 @@ namespace {
619611
/*invocation subs*/ SubstitutionMap(), IGF.IGM.Context);
620612
}
621613

622-
void emitCopyWithCopyConstructor(
614+
void emitCopyWithCopyOrMoveConstructor(
623615
IRGenFunction &IGF, SILType T,
624616
const clang::CXXConstructorDecl *copyConstructor, llvm::Value *src,
625617
llvm::Value *dest) const {
@@ -629,7 +621,27 @@ namespace {
629621

630622
auto &ctx = IGF.IGM.Context;
631623
auto *importer = static_cast<ClangImporter *>(ctx.getClangModuleLoader());
632-
624+
625+
if (copyConstructor->isDefaulted() &&
626+
copyConstructor->getAccess() == clang::AS_public &&
627+
!copyConstructor->isDeleted() &&
628+
!copyConstructor->isIneligibleOrNotSelected() &&
629+
// Note: we use "doesThisDeclarationHaveABody" here because
630+
// that's what "DefineImplicitCopyConstructor" checks.
631+
!copyConstructor->doesThisDeclarationHaveABody()) {
632+
assert(!copyConstructor->getParent()->isAnonymousStructOrUnion() &&
633+
"Cannot do codegen of special member functions of anonymous "
634+
"structs/unions");
635+
if (copyConstructor->isCopyConstructor())
636+
importer->getClangSema().DefineImplicitCopyConstructor(
637+
clang::SourceLocation(),
638+
const_cast<clang::CXXConstructorDecl *>(copyConstructor));
639+
else
640+
importer->getClangSema().DefineImplicitMoveConstructor(
641+
clang::SourceLocation(),
642+
const_cast<clang::CXXConstructorDecl *>(copyConstructor));
643+
}
644+
633645
auto &diagEngine = importer->getClangSema().getDiagnostics();
634646
clang::DiagnosticErrorTrap trap(diagEngine);
635647
auto clangFnAddr =
@@ -809,9 +821,9 @@ namespace {
809821
Address srcAddr, SILType T,
810822
bool isOutlined) const override {
811823
if (auto copyConstructor = findCopyConstructor()) {
812-
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
813-
srcAddr.getAddress(),
814-
destAddr.getAddress());
824+
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
825+
srcAddr.getAddress(),
826+
destAddr.getAddress());
815827
return;
816828
}
817829
StructTypeInfoBase<AddressOnlyCXXClangRecordTypeInfo, FixedTypeInfo,
@@ -824,9 +836,9 @@ namespace {
824836
SILType T, bool isOutlined) const override {
825837
if (auto copyConstructor = findCopyConstructor()) {
826838
destroy(IGF, destAddr, T, isOutlined);
827-
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
828-
srcAddr.getAddress(),
829-
destAddr.getAddress());
839+
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
840+
srcAddr.getAddress(),
841+
destAddr.getAddress());
830842
return;
831843
}
832844
StructTypeInfoBase<AddressOnlyCXXClangRecordTypeInfo, FixedTypeInfo,
@@ -838,17 +850,15 @@ namespace {
838850
SILType T, bool isOutlined,
839851
bool zeroizeIfSensitive) const override {
840852
if (auto moveConstructor = findMoveConstructor()) {
841-
emitCopyWithCopyConstructor(IGF, T, moveConstructor,
842-
src.getAddress(),
843-
dest.getAddress());
853+
emitCopyWithCopyOrMoveConstructor(IGF, T, moveConstructor,
854+
src.getAddress(), dest.getAddress());
844855
destroy(IGF, src, T, isOutlined);
845856
return;
846857
}
847858

848859
if (auto copyConstructor = findCopyConstructor()) {
849-
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
850-
src.getAddress(),
851-
dest.getAddress());
860+
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
861+
src.getAddress(), dest.getAddress());
852862
destroy(IGF, src, T, isOutlined);
853863
return;
854864
}
@@ -862,18 +872,16 @@ namespace {
862872
bool isOutlined) const override {
863873
if (auto moveConstructor = findMoveConstructor()) {
864874
destroy(IGF, dest, T, isOutlined);
865-
emitCopyWithCopyConstructor(IGF, T, moveConstructor,
866-
src.getAddress(),
867-
dest.getAddress());
875+
emitCopyWithCopyOrMoveConstructor(IGF, T, moveConstructor,
876+
src.getAddress(), dest.getAddress());
868877
destroy(IGF, src, T, isOutlined);
869878
return;
870879
}
871880

872881
if (auto copyConstructor = findCopyConstructor()) {
873882
destroy(IGF, dest, T, isOutlined);
874-
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
875-
src.getAddress(),
876-
dest.getAddress());
883+
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
884+
src.getAddress(), dest.getAddress());
877885
destroy(IGF, src, T, isOutlined);
878886
return;
879887
}

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

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// RUN: %empty-directory(%t)
22
// RUN: split-file %s %t
3-
// 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
4-
// 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
3+
// 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
4+
// 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
55

66
//--- Inputs/module.modulemap
77
module Test {
@@ -12,10 +12,11 @@ module Test {
1212
//--- Inputs/noncopyable.h
1313
#include "swift/bridging"
1414
#include <string>
15+
#include <vector>
1516

1617
struct NonCopyable {
1718
NonCopyable() = default;
18-
NonCopyable(const NonCopyable& other) = delete;
19+
NonCopyable(const NonCopyable& other) = delete; // expected-note {{'NonCopyable' has been explicitly marked deleted here}}
1920
NonCopyable(NonCopyable&& other) = default;
2021
};
2122

@@ -79,10 +80,63 @@ struct SWIFT_NONCOPYABLE NonCopyableNonMovable { // expected-note {{record 'NonC
7980
NonCopyableNonMovable(NonCopyableNonMovable&& other) = delete;
8081
};
8182

83+
struct ImplicitCopyConstructor {
84+
NonCopyable element;
85+
};
86+
87+
template <typename T, typename P, typename R>
88+
struct TemplatedImplicitCopyConstructor {
89+
T element;
90+
P *pointer;
91+
R &reference;
92+
};
93+
94+
using NonCopyableT = TemplatedImplicitCopyConstructor<NonCopyable, int, int>;
95+
using NonCopyableP = TemplatedImplicitCopyConstructor<int, NonCopyable, int>;
96+
using NonCopyableR = TemplatedImplicitCopyConstructor<int, int, NonCopyable>;
97+
98+
struct DefaultedCopyConstructor {
99+
NonCopyable element; // expected-note {{copy constructor of 'DefaultedCopyConstructor' is implicitly deleted because field 'element' has a deleted copy constructor}}
100+
DefaultedCopyConstructor(const DefaultedCopyConstructor&) = default;
101+
// expected-warning@-1 {{explicitly defaulted copy constructor is implicitly deleted}}
102+
// expected-note@-2 {{replace 'default' with 'delete'}}
103+
DefaultedCopyConstructor(DefaultedCopyConstructor&&) = default;
104+
};
105+
106+
template<typename T>
107+
struct TemplatedDefaultedCopyConstructor {
108+
T element;
109+
TemplatedDefaultedCopyConstructor(const TemplatedDefaultedCopyConstructor&) = default;
110+
TemplatedDefaultedCopyConstructor(TemplatedDefaultedCopyConstructor&&) = default;
111+
};
112+
113+
template<typename T>
114+
struct DerivedTemplatedDefaultedCopyConstructor : TemplatedDefaultedCopyConstructor<T> {};
115+
116+
using CopyableDefaultedCopyConstructor = TemplatedDefaultedCopyConstructor<NonCopyableP>;
117+
using NonCopyableDefaultedCopyConstructor = TemplatedDefaultedCopyConstructor<NonCopyable>;
118+
using CopyableDerived = DerivedTemplatedDefaultedCopyConstructor<NonCopyableR>;
119+
using NonCopyableDerived = DerivedTemplatedDefaultedCopyConstructor<NonCopyable>;
120+
82121
template<typename T> struct SWIFT_COPYABLE_IF(T) DisposableContainer {};
83122
struct POD { int x; float y; }; // special members are implicit, but should be copyable
84123
using DisposablePOD = DisposableContainer<POD>; // should also be copyable
85124

125+
struct DerivesFromMe : MyPair<DisposableContainer<DerivesFromMe>, std::vector<NonCopyable>> {};
126+
struct DerivesFromMeToo : MyPair<std::vector<NonCopyable>, DisposableContainer<DerivesFromMe>> {};
127+
128+
template <typename T>
129+
struct OneField {
130+
T field;
131+
};
132+
133+
template <typename F, typename S>
134+
struct SWIFT_COPYABLE_IF(F, S) NoFields {};
135+
136+
struct FieldDependsOnMe { // used to trigger a request cycle
137+
OneField<NoFields<FieldDependsOnMe, NonCopyable>> field;
138+
};
139+
86140
//--- test.swift
87141
import Test
88142
import CxxStdlib
@@ -133,6 +187,34 @@ func missingLifetimeOperation() {
133187
takeCopyable(s)
134188
}
135189

190+
func implicitCopyConstructor(i: borrowing ImplicitCopyConstructor, t: borrowing NonCopyableT, p: borrowing NonCopyableP, r: borrowing NonCopyableR) {
191+
takeCopyable(i) // expected-error {{global function 'takeCopyable' requires that 'ImplicitCopyConstructor' conform to 'Copyable'}}
192+
takeCopyable(t) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableT' (aka 'TemplatedImplicitCopyConstructor<NonCopyable, CInt, CInt>') conform to 'Copyable'}}
193+
194+
// References and pointers to non-copyable types are still copyable
195+
takeCopyable(p)
196+
takeCopyable(r)
197+
}
198+
199+
func defaultCopyConstructor(d: borrowing DefaultedCopyConstructor, d1: borrowing CopyableDefaultedCopyConstructor, d2: borrowing NonCopyableDefaultedCopyConstructor, d3: borrowing CopyableDerived, d4: borrowing NonCopyableDerived) {
200+
takeCopyable(d) // expected-error {{global function 'takeCopyable' requires that 'DefaultedCopyConstructor' conform to 'Copyable'}}
201+
takeCopyable(d1)
202+
takeCopyable(d2) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableDefaultedCopyConstructor' (aka 'TemplatedDefaultedCopyConstructor<NonCopyable>') conform to 'Copyable'}}
203+
takeCopyable(d3)
204+
takeCopyable(d4) // expected-error {{global function 'takeCopyable' requires that 'NonCopyableDerived' (aka 'DerivedTemplatedDefaultedCopyConstructor<NonCopyable>') conform to 'Copyable'}}
205+
}
206+
136207
func copyableDisposablePOD(p: DisposablePOD) {
137208
takeCopyable(p)
138209
}
210+
211+
func couldCreateCycleOfCxxValueSemanticsRequests() {
212+
let d1 = DerivesFromMe()
213+
takeCopyable(d1) // expected-error {{global function 'takeCopyable' requires that 'DerivesFromMe' conform to 'Copyable'}}
214+
215+
let d2 = DerivesFromMeToo()
216+
takeCopyable(d2) // expected-error {{global function 'takeCopyable' requires that 'DerivesFromMeToo' conform to 'Copyable'}}
217+
218+
let d3 = FieldDependsOnMe()
219+
takeCopyable(d3) // expected-error {{global function 'takeCopyable' requires that 'FieldDependsOnMe' conform to 'Copyable'}}
220+
}

0 commit comments

Comments
 (0)