Skip to content

Commit 1fc5a4e

Browse files
susmonteirosusmonteiro
authored
Merge pull request #79542 from swiftlang/susmonteiro/move-constructor-default-args
[cxx-interop] Prevent usage in Swift of C++ move constructor with default args
2 parents 077a01f + 51357a9 commit 1fc5a4e

11 files changed

+113
-31
lines changed

‎include/swift/AST/DiagnosticsClangImporter.def

+4
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

+3-1
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

+17-10
Original file line numberDiff line numberDiff line change
@@ -8011,14 +8011,6 @@ static bool isSufficientlyTrivial(const clang::CXXRecordDecl *decl) {
80118011
return true;
80128012
}
80138013

8014-
static bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) {
8015-
if (ctor->getNumParams() < 2)
8016-
return false;
8017-
8018-
auto lastParam = ctor->parameters().back();
8019-
return lastParam->hasDefaultArg();
8020-
}
8021-
80228014
/// Checks if a record provides the required value type lifetime operations
80238015
/// (copy and destroy).
80248016
static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
@@ -8035,7 +8027,8 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
80358027
// struct.
80368028
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
80378029
return ctor->isCopyConstructor() && !ctor->isDeleted() &&
8038-
!hasNonFirstDefaultArg(ctor) &&
8030+
// FIXME: Support default arguments (rdar://142414553)
8031+
ctor->getNumParams() == 1 &&
80398032
ctor->getAccess() == clang::AccessSpecifier::AS_public;
80408033
});
80418034
}
@@ -8051,7 +8044,9 @@ static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
80518044
return false;
80528045

80538046
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8054-
return ctor->isMoveConstructor();
8047+
return ctor->isMoveConstructor() &&
8048+
// FIXME: Support default arguments (rdar://142414553)
8049+
ctor->getNumParams() == 1;
80558050
});
80568051
}
80578052

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

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

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

‎lib/ClangImporter/ImportDecl.cpp

+11-3
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

+5-9
Original file line numberDiff line numberDiff line change
@@ -541,20 +541,14 @@ 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) &&
549+
if (ctor->isCopyConstructor() &&
550+
// FIXME: Support default arguments (rdar://142414553)
551+
ctor->getNumParams() == 1 &&
558552
ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
559553
return ctor;
560554
}
@@ -567,6 +561,8 @@ namespace {
567561
return nullptr;
568562
for (auto ctor : cxxRecordDecl->ctors()) {
569563
if (ctor->isMoveConstructor() &&
564+
// FIXME: Support default arguments (rdar://142414553)
565+
ctor->getNumParams() == 1 &&
570566
ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
571567
return ctor;
572568
}

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

+14
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

+22
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

+7
Original file line numberDiff line numberDiff line change
@@ -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

+3-3
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

+3-3
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

+24-2
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)