Skip to content

Commit 8a1e620

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

File tree

13 files changed

+116
-31
lines changed

13 files changed

+116
-31
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/ClangImporter.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,8 @@ class ClangImporter final : public ClangModuleLoader {
688688
llvm::cas::ObjectStore &CAS, llvm::cas::ObjectRef ChainedPCHIncludeTree);
689689

690690
SourceLoc importSourceLocation(clang::SourceLocation loc) override;
691+
692+
static bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor);
691693
};
692694

693695
ImportDecl *createImportDecl(ASTContext &Ctx, DeclContext *DC, ClangNode ClangN,

‎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 & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8010,12 +8010,9 @@ 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();
8013+
bool ClangImporter::hasNonFirstDefaultArg(
8014+
const clang::CXXConstructorDecl *ctor) {
8015+
return ctor->getNumParams() > 1;
80198016
}
80208017

80218018
/// Checks if a record provides the required value type lifetime operations
@@ -8034,7 +8031,7 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
80348031
// struct.
80358032
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
80368033
return ctor->isCopyConstructor() && !ctor->isDeleted() &&
8037-
!hasNonFirstDefaultArg(ctor) &&
8034+
!ClangImporter::hasNonFirstDefaultArg(ctor) &&
80388035
ctor->getAccess() == clang::AccessSpecifier::AS_public;
80398036
});
80408037
}
@@ -8050,7 +8047,8 @@ static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
80508047
return false;
80518048

80528049
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8053-
return ctor->isMoveConstructor();
8050+
return ctor->isMoveConstructor() &&
8051+
!ClangImporter::hasNonFirstDefaultArg(ctor);
80548052
});
80558053
}
80568054

@@ -8069,6 +8067,14 @@ static bool hasCustomCopyOrMoveConstructor(const clang::CXXRecordDecl *decl) {
80698067
decl->hasUserDeclaredMoveConstructor();
80708068
}
80718069

8070+
static bool
8071+
hasConstructorWithUnsupportedDefaultArgs(const clang::CXXRecordDecl *decl) {
8072+
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8073+
return (ctor->isCopyConstructor() || ctor->isMoveConstructor()) &&
8074+
ClangImporter::hasNonFirstDefaultArg(ctor);
8075+
});
8076+
}
8077+
80728078
static bool isSwiftClassType(const clang::CXXRecordDecl *decl) {
80738079
// Swift type must be annotated with external_source_symbol attribute.
80748080
auto essAttr = decl->getAttr<clang::ExternalSourceSymbolAttr>();
@@ -8124,6 +8130,9 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
81248130
"import_iterator", decl->getNameAsString());
81258131
}
81268132

8133+
if (hasConstructorWithUnsupportedDefaultArgs(cxxDecl))
8134+
return CxxRecordSemanticsKind::UnavailableConstructors;
8135+
81278136
return CxxRecordSemanticsKind::MissingLifetimeOperation;
81288137
}
81298138

‎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: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "swift/AST/SubstitutionMap.h"
2626
#include "swift/AST/Types.h"
2727
#include "swift/Basic/Assertions.h"
28+
#include "swift/ClangImporter/ClangImporter.h"
2829
#include "swift/IRGen/Linking.h"
2930
#include "swift/SIL/SILFunctionBuilder.h"
3031
#include "swift/SIL/SILModule.h"
@@ -541,20 +542,13 @@ namespace {
541542
FixedTypeInfo, ClangFieldInfo> {
542543
const clang::RecordDecl *ClangDecl;
543544

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-
552545
const clang::CXXConstructorDecl *findCopyConstructor() const {
553546
const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
554547
if (!cxxRecordDecl)
555548
return nullptr;
556549
for (auto ctor : cxxRecordDecl->ctors()) {
557-
if (ctor->isCopyConstructor() && !hasNonFirstDefaultArg(ctor) &&
550+
if (ctor->isCopyConstructor() &&
551+
!ClangImporter::hasNonFirstDefaultArg(ctor) &&
558552
ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
559553
return ctor;
560554
}
@@ -567,6 +561,7 @@ namespace {
567561
return nullptr;
568562
for (auto ctor : cxxRecordDecl->ctors()) {
569563
if (ctor->isMoveConstructor() &&
564+
!ClangImporter::hasNonFirstDefaultArg(ctor) &&
570565
ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
571566
return ctor;
572567
}

‎lib/SILOptimizer/PassManager/PassPipeline.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -681,7 +681,7 @@ static void addHighLevelFunctionPipeline(SILPassPipelinePlan &P) {
681681

682682
// Skip EagerSpecializer on embedded Swift, which already specializes
683683
// everything. Otherwise this would create metatype references for functions
684-
// with @_specialize attribute and those are incompatible with Emebdded Swift.
684+
// with @_specialize attribute and those are incompatible with Embedded Swift.
685685
if (!P.getOptions().EmbeddedSwift) {
686686
P.addEagerSpecializer();
687687
}

‎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)