Skip to content

Commit 0b83c84

Browse files
susmonteirosusmonteiro
committed
[cxx-interop] Prevent usage in Swift of C++ move constructor with default args
1 parent 6f132fd commit 0b83c84

11 files changed

+116
-34
lines changed

‎include/swift/AST/DiagnosticsClangImporter.def

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,10 @@ NOTE(record_not_automatically_importable, none, "record '%0' is not "
174174
"does this type have reference "
175175
"semantics?",
176176
(StringRef, StringRef))
177+
NOTE(record_unsupported_default_args, none,
178+
"copy constructors and move constructors with more than one parameter are "
179+
"unavailable in Swift",
180+
())
177181

178182
NOTE(projection_value_not_imported, none, "C++ method '%0' that returns a value "
179183
"of type '%1' is unavailable",

‎include/swift/ClangImporter/ClangImporterRequests.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,8 +347,10 @@ enum class CxxRecordSemanticsKind {
347347
MoveOnly,
348348
Reference,
349349
Iterator,
350-
// A record that is either not copyable or not destructible.
350+
// A record that is either not copyable/movable or not destructible.
351351
MissingLifetimeOperation,
352+
// A record that has no copy and no move operations
353+
UnavailableConstructors,
352354
// A C++ record that represents a Swift class type exposed to C++ from Swift.
353355
SwiftClassType
354356
};

‎lib/ClangImporter/ClangImporter.cpp

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8010,14 +8010,6 @@ static bool isSufficientlyTrivial(const clang::CXXRecordDecl *decl) {
80108010
return true;
80118011
}
80128012

8013-
static bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) {
8014-
if (ctor->getNumParams() < 2)
8015-
return false;
8016-
8017-
auto lastParam = ctor->parameters().back();
8018-
return lastParam->hasDefaultArg();
8019-
}
8020-
80218013
/// Checks if a record provides the required value type lifetime operations
80228014
/// (copy and destroy).
80238015
static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
@@ -8034,7 +8026,8 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
80348026
// struct.
80358027
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
80368028
return ctor->isCopyConstructor() && !ctor->isDeleted() &&
8037-
!hasNonFirstDefaultArg(ctor) &&
8029+
// FIXME: Support default arguments (rdar://142414553)
8030+
ctor->getNumParams() < 2 &&
80388031
ctor->getAccess() == clang::AccessSpecifier::AS_public;
80398032
});
80408033
}
@@ -8050,7 +8043,9 @@ static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
80508043
return false;
80518044

80528045
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8053-
return ctor->isMoveConstructor();
8046+
return ctor->isMoveConstructor() &&
8047+
// FIXME: Support default arguments (rdar://142414553)
8048+
ctor->getNumParams() < 2;
80548049
});
80558050
}
80568051

@@ -8069,6 +8064,15 @@ static bool hasCustomCopyOrMoveConstructor(const clang::CXXRecordDecl *decl) {
80698064
decl->hasUserDeclaredMoveConstructor();
80708065
}
80718066

8067+
static bool
8068+
hasConstructorWithUnsupportedDefaultArgs(const clang::CXXRecordDecl *decl) {
8069+
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8070+
return (ctor->isCopyConstructor() || ctor->isMoveConstructor()) &&
8071+
// FIXME: Support default arguments (rdar://142414553)
8072+
ctor->getNumParams() < 2;
8073+
});
8074+
}
8075+
80728076
static bool isSwiftClassType(const clang::CXXRecordDecl *decl) {
80738077
// Swift type must be annotated with external_source_symbol attribute.
80748078
auto essAttr = decl->getAttr<clang::ExternalSourceSymbolAttr>();
@@ -8124,6 +8128,9 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
81248128
"import_iterator", decl->getNameAsString());
81258129
}
81268130

8131+
if (hasConstructorWithUnsupportedDefaultArgs(cxxDecl))
8132+
return CxxRecordSemanticsKind::UnavailableConstructors;
8133+
81278134
return CxxRecordSemanticsKind::MissingLifetimeOperation;
81288135
}
81298136

‎lib/ClangImporter/ImportDecl.cpp

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2966,15 +2966,23 @@ namespace {
29662966
auto semanticsKind = evaluateOrDefault(
29672967
Impl.SwiftContext.evaluator,
29682968
CxxRecordSemantics({decl, Impl.SwiftContext, &Impl}), {});
2969-
if (semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation &&
2969+
if ((semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation ||
2970+
semanticsKind == CxxRecordSemanticsKind::UnavailableConstructors) &&
29702971
// Let un-specialized class templates through. We'll sort out their
2971-
// members once they're instranciated.
2972+
// members once they're instantiated.
29722973
!Impl.importSymbolicCXXDecls) {
2974+
2975+
if (semanticsKind == CxxRecordSemanticsKind::UnavailableConstructors) {
2976+
Impl.addImportDiagnostic(
2977+
decl, Diagnostic(diag::record_unsupported_default_args),
2978+
decl->getLocation());
2979+
}
2980+
29732981
Impl.addImportDiagnostic(
29742982
decl,
29752983
Diagnostic(diag::record_not_automatically_importable,
29762984
Impl.SwiftContext.AllocateCopy(decl->getNameAsString()),
2977-
"does not have a copy constructor or destructor"),
2985+
"it must have a copy/move constructor and a destructor"),
29782986
decl->getLocation());
29792987
return nullptr;
29802988
}

‎lib/IRGen/GenStruct.cpp

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -541,21 +541,15 @@ namespace {
541541
FixedTypeInfo, ClangFieldInfo> {
542542
const clang::RecordDecl *ClangDecl;
543543

544-
bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) const {
545-
if (ctor->getNumParams() < 2)
546-
return false;
547-
548-
auto lastParam = ctor->parameters().back();
549-
return lastParam->hasDefaultArg();
550-
}
551-
552544
const clang::CXXConstructorDecl *findCopyConstructor() const {
553545
const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
554546
if (!cxxRecordDecl)
555547
return nullptr;
556548
for (auto ctor : cxxRecordDecl->ctors()) {
557-
if (ctor->isCopyConstructor() && !hasNonFirstDefaultArg(ctor) &&
558-
ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
549+
if (ctor->isCopyConstructor() &&
550+
// FIXME: Support default arguments (rdar://142414553)
551+
ctor->getNumParams() < 2 && ctor->getAccess() == clang::AS_public &&
552+
!ctor->isDeleted())
559553
return ctor;
560554
}
561555
return nullptr;
@@ -567,7 +561,9 @@ namespace {
567561
return nullptr;
568562
for (auto ctor : cxxRecordDecl->ctors()) {
569563
if (ctor->isMoveConstructor() &&
570-
ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
564+
// FIXME: Support default arguments (rdar://142414553)
565+
ctor->getNumParams() < 2 && ctor->getAccess() == clang::AS_public &&
566+
!ctor->isDeleted())
571567
return ctor;
572568
}
573569
return nullptr;

‎test/Interop/Cxx/class/Inputs/constructors.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,4 +108,18 @@ struct HasPtrAuthMember {
108108
};
109109
#endif
110110

111+
struct MoveConstructorWithOneParameterWithDefaultArg {
112+
int value;
113+
114+
MoveConstructorWithOneParameterWithDefaultArg(int value) : value(value) {}
115+
116+
MoveConstructorWithOneParameterWithDefaultArg(
117+
const MoveConstructorWithOneParameterWithDefaultArg &) = delete;
118+
119+
MoveConstructorWithOneParameterWithDefaultArg(
120+
MoveConstructorWithOneParameterWithDefaultArg &&other =
121+
MoveConstructorWithOneParameterWithDefaultArg{0})
122+
: value(other.value + 1) {}
123+
};
124+
111125
#endif // TEST_INTEROP_CXX_CLASS_INPUTS_CONSTRUCTORS_H

‎test/Interop/Cxx/class/Inputs/type-classification.h

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,4 +274,26 @@ struct HasCopyConstructorWithDefaultArgs {
274274
default;
275275
};
276276

277+
struct HasMoveConstructorWithDefaultArgs {
278+
int value;
279+
HasMoveConstructorWithDefaultArgs(int value) : value(value) {}
280+
281+
HasMoveConstructorWithDefaultArgs(HasMoveConstructorWithDefaultArgs &&other,
282+
int value = 1)
283+
: value(other.value + value) {}
284+
};
285+
286+
struct HasCopyAndMoveConstructorWithDefaultArgs {
287+
int value;
288+
HasCopyAndMoveConstructorWithDefaultArgs(int value) : value(value) {}
289+
290+
HasCopyAndMoveConstructorWithDefaultArgs(
291+
const HasCopyAndMoveConstructorWithDefaultArgs &other, int value = 1)
292+
: value(other.value + value) {}
293+
294+
HasCopyAndMoveConstructorWithDefaultArgs(
295+
HasCopyAndMoveConstructorWithDefaultArgs &&other, int value = 1)
296+
: value(other.value + value) {}
297+
};
298+
277299
#endif // TEST_INTEROP_CXX_CLASS_INPUTS_TYPE_CLASSIFICATION_H

‎test/Interop/Cxx/class/constructors-executable.swift

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-experimental-cxx-interop)
1+
// RUN: %target-run-simple-swift(-I %S/Inputs/ -O -Xfrontend -enable-experimental-cxx-interop)
22
//
33
// REQUIRES: executable_test
44

@@ -63,4 +63,11 @@ CxxConstructorTestSuite.test("implicit default ctor") {
6363
expectNil(instance3.ptr)
6464
}
6565

66+
CxxConstructorTestSuite.test("MoveConstructorWithOneParamWithDefaultArg") {
67+
let instance1 = MoveConstructorWithOneParameterWithDefaultArg(5)
68+
let instance2 = instance1
69+
let instance3 = MoveConstructorWithOneParameterWithDefaultArg(5)
70+
expectTrue(instance2.value + instance3.value >= 10)
71+
}
72+
6673
runAllTests()

‎test/Interop/Cxx/class/copy-move-assignment-executable.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ CxxCopyMoveAssignTestSuite.test("NonTrivialCopyAssign") {
2525
expectEqual(0, instance.copyAssignCounter)
2626
takeValue(instance2)
2727
}
28-
// The number of construcors and destructors called for `NonTrivialCopyAssign` must be balanced.
28+
// The number of constructors and destructors called for `NonTrivialCopyAssign` must be balanced.
2929
expectEqual(0, InstanceBalanceCounter.getCounterValue())
3030
}
3131

@@ -37,7 +37,7 @@ CxxCopyMoveAssignTestSuite.test("NonTrivialMoveAssign") {
3737
// `operator=` isn't called.
3838
expectEqual(0, instance.moveAssignCounter)
3939
}
40-
// The number of construcors and destructors called for `NonTrivialCopyAssign` must be balanced.
40+
// The number of constructors and destructors called for `NonTrivialCopyAssign` must be balanced.
4141
expectEqual(0, InstanceBalanceCounter.getCounterValue())
4242
}
4343

@@ -51,7 +51,7 @@ CxxCopyMoveAssignTestSuite.test("NonTrivialCopyAndCopyMoveAssign") {
5151
expectEqual(0, instance.assignCounter)
5252
takeValue(instance2)
5353
}
54-
// The number of construcors and destructors called for `NonTrivialCopyAndCopyMoveAssign` must be balanced.
54+
// The number of constructors and destructors called for `NonTrivialCopyAndCopyMoveAssign` must be balanced.
5555
expectEqual(0, InstanceBalanceCounter.getCounterValue())
5656
}
5757

‎test/Interop/Cxx/class/invalid-class-errors.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,17 +27,17 @@ struct Nested {
2727

2828
import Test
2929

30-
// CHECK: note: record 'A' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
30+
// CHECK: note: record 'A' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?
3131
// CHECK: struct A {
3232
// CHECK: ^
3333
// CHECK: SWIFT_SHARED_REFERENCE(<#retain#>, <#release#>)
3434
public func test(x: A) { }
35-
// CHECK: note: record 'B' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
35+
// CHECK: note: record 'B' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?
3636
// CHECK: struct {{.*}}B {
3737
// CHECK: ^
3838
// CHECK: SWIFT_SHARED_REFERENCE(<#retain#>, <#release#>)
3939
public func test(x: B) { }
40-
// CHECK: note: record 'Nested' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
40+
// CHECK: note: record 'Nested' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?
4141
// CHECK: struct Nested {
4242
// CHECK: ^
4343
// CHECK: SWIFT_SHARED_REFERENCE(<#retain#>, <#release#>)

‎test/Interop/Cxx/class/type-classification-typechecker.swift

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,35 @@ func testAnnotated() {
2222
_ = v
2323
}
2424

25-
func testNonCopyable() {
25+
func testHasCopyTypeOperations() {
2626
let x = HasCopyConstructorWithDefaultArgs(5)
2727
let v = copy x // expected-error {{'copy' cannot be applied to noncopyable types}}
2828
_ = v
2929
}
3030

31+
func testFuncCallNoncopyable() {
32+
func aux(input: HasCopyConstructorWithDefaultArgs) {}
33+
let x = HasCopyConstructorWithDefaultArgs(5)
34+
aux(input: x)
35+
// expected-error@-3 {{parameter of noncopyable type 'HasCopyConstructorWithDefaultArgs' must specify ownership}}
36+
// expected-note@-4 {{add 'borrowing' for an immutable reference}}
37+
// expected-note@-5 {{add 'consuming' to take the value from the caller}}
38+
// expected-note@-6 {{add 'inout' for a mutable reference}}
39+
}
40+
41+
func testHasMoveTypeOperations() {
42+
let x = HasMoveConstructorWithDefaultArgs(5) // expected-error {{cannot find 'HasMoveConstructorWithDefaultArgs' in scope}}
43+
}
44+
45+
func testHasCopyOrMoveTypeOperations() {
46+
let x = HasCopyAndMoveConstructorWithDefaultArgs(5)
47+
// expected-error@-1 {{cannot find 'HasCopyAndMoveConstructorWithDefaultArgs' in scope}}
48+
}
49+
3150
test()
3251
testField()
3352
testAnnotated()
34-
testNonCopyable()
53+
testHasCopyTypeOperations()
54+
testFuncCallNoncopyable()
55+
testHasMoveTypeOperations()
56+
testHasCopyOrMoveTypeOperations()

0 commit comments

Comments
 (0)