Skip to content

Commit 7e14a8e

Browse files
susmonteirosusmonteiro
committed
[cxx-interop] SWIFT_NONCOPYABLE overlooked in some cases
1 parent f4cf914 commit 7e14a8e

File tree

5 files changed

+76
-53
lines changed

5 files changed

+76
-53
lines changed

‎lib/ClangImporter/ClangImporter.cpp‎

Lines changed: 46 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -8237,17 +8237,15 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
82378237
}
82388238

82398239
static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
8240-
if (llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8241-
return ctor->isMoveConstructor() &&
8242-
(ctor->isDeleted() || ctor->isIneligibleOrNotSelected() ||
8243-
ctor->getAccess() != clang::AS_public);
8244-
}))
8245-
return false;
8240+
if (decl->hasSimpleMoveConstructor())
8241+
return true;
82468242

82478243
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8248-
return ctor->isMoveConstructor() &&
8244+
return ctor->isMoveConstructor() && !ctor->isDeleted() &&
8245+
!ctor->isIneligibleOrNotSelected() &&
82498246
// FIXME: Support default arguments (rdar://142414553)
8250-
ctor->getNumParams() == 1;
8247+
ctor->getNumParams() == 1 &&
8248+
ctor->getAccess() == clang::AS_public;
82518249
});
82528250
}
82538251

@@ -8379,46 +8377,48 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
83798377
return CxxValueSemanticsKind::Copyable;
83808378
}
83818379

8382-
auto injectedStlAnnotation =
8383-
recordDecl->isInStdNamespace()
8384-
? STLConditionalParams.find(recordDecl->getName())
8385-
: STLConditionalParams.end();
8386-
auto STLParams = injectedStlAnnotation != STLConditionalParams.end()
8387-
? injectedStlAnnotation->second
8388-
: std::vector<int>();
8389-
auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);
8390-
8391-
if (!STLParams.empty() || !conditionalParams.empty()) {
8392-
HeaderLoc loc{recordDecl->getLocation()};
8393-
std::function checkArgValueSemantics =
8394-
[&](clang::TemplateArgument &arg,
8395-
StringRef argToCheck) -> std::optional<CxxValueSemanticsKind> {
8396-
if (arg.getKind() != clang::TemplateArgument::Type && importerImpl) {
8397-
importerImpl->diagnose(loc, diag::type_template_parameter_expected,
8398-
argToCheck);
8399-
return CxxValueSemanticsKind::Unknown;
8400-
}
8380+
if (!hasNonCopyableAttr(recordDecl)) {
8381+
auto injectedStlAnnotation =
8382+
recordDecl->isInStdNamespace()
8383+
? STLConditionalParams.find(recordDecl->getName())
8384+
: STLConditionalParams.end();
8385+
auto STLParams = injectedStlAnnotation != STLConditionalParams.end()
8386+
? injectedStlAnnotation->second
8387+
: std::vector<int>();
8388+
auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);
84018389

8402-
auto argValueSemantics = evaluateOrDefault(
8403-
evaluator,
8404-
CxxValueSemantics(
8405-
{arg.getAsType()->getUnqualifiedDesugaredType(), importerImpl}),
8406-
{});
8407-
if (argValueSemantics != CxxValueSemanticsKind::Copyable)
8408-
return argValueSemantics;
8409-
return std::nullopt;
8410-
};
8390+
if (!STLParams.empty() || !conditionalParams.empty()) {
8391+
HeaderLoc loc{recordDecl->getLocation()};
8392+
std::function checkArgValueSemantics =
8393+
[&](clang::TemplateArgument &arg,
8394+
StringRef argToCheck) -> std::optional<CxxValueSemanticsKind> {
8395+
if (arg.getKind() != clang::TemplateArgument::Type && importerImpl) {
8396+
importerImpl->diagnose(loc, diag::type_template_parameter_expected,
8397+
argToCheck);
8398+
return CxxValueSemanticsKind::Unknown;
8399+
}
84118400

8412-
auto result = checkConditionalParams<CxxValueSemanticsKind>(
8413-
recordDecl, STLParams, conditionalParams, checkArgValueSemantics);
8414-
if (result.has_value())
8415-
return result.value();
8401+
auto argValueSemantics = evaluateOrDefault(
8402+
evaluator,
8403+
CxxValueSemantics(
8404+
{arg.getAsType()->getUnqualifiedDesugaredType(), importerImpl}),
8405+
{});
8406+
if (argValueSemantics != CxxValueSemanticsKind::Copyable)
8407+
return argValueSemantics;
8408+
return std::nullopt;
8409+
};
84168410

8417-
if (importerImpl)
8418-
for (auto name : conditionalParams)
8419-
importerImpl->diagnose(loc, diag::unknown_template_parameter, name);
8411+
auto result = checkConditionalParams<CxxValueSemanticsKind>(
8412+
recordDecl, STLParams, conditionalParams, checkArgValueSemantics);
8413+
if (result.has_value())
8414+
return result.value();
84208415

8421-
return CxxValueSemanticsKind::Copyable;
8416+
if (importerImpl)
8417+
for (auto name : conditionalParams)
8418+
importerImpl->diagnose(loc, diag::unknown_template_parameter, name);
8419+
8420+
return CxxValueSemanticsKind::Copyable;
8421+
}
84228422
}
84238423

84248424
const auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);
@@ -8428,7 +8428,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
84288428
return CxxValueSemanticsKind::Copyable;
84298429
}
84308430

8431-
bool isCopyable = hasCopyTypeOperations(cxxRecordDecl);
8431+
bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
8432+
hasCopyTypeOperations(cxxRecordDecl);
84328433
bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
84338434

84348435
if (!hasDestroyTypeOperations(cxxRecordDecl) || (!isCopyable && !isMovable)) {

‎lib/ClangImporter/SwiftDeclSynthesizer.cpp‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3011,10 +3011,8 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
30113011
ctx.evaluator,
30123012
CxxValueSemantics({clangType->getTypeForDecl(), &ImporterImpl}), {});
30133013

3014-
if (valueSemanticsKind == CxxValueSemanticsKind::MoveOnly)
3015-
return destroyFunc;
3016-
3017-
if (valueSemanticsKind != CxxValueSemanticsKind::Copyable)
3014+
if (valueSemanticsKind != CxxValueSemanticsKind::Copyable &&
3015+
valueSemanticsKind != CxxValueSemanticsKind::MoveOnly)
30183016
return nullptr;
30193017

30203018
auto cxxRecordSemanticsKind = evaluateOrDefault(
@@ -3033,6 +3031,9 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
30333031
}
30343032
}
30353033

3034+
if (valueSemanticsKind == CxxValueSemanticsKind::MoveOnly)
3035+
return destroyFunc;
3036+
30363037
markDeprecated(
30373038
nominal,
30383039
"destroy operation '" +

‎test/Interop/C/struct/Inputs/noncopyable-struct.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ void badDestroy2(BadDestroyNonCopyableType2 *ptr);
4747

4848
struct __attribute__((swift_attr("~Copyable"))) __attribute__((swift_attr("destroy:extraDestroy"))) ExtraDestroy {
4949
void *storage;
50-
50+
ExtraDestroy(ExtraDestroy&&) = default;
5151
~ExtraDestroy() { }
5252
};
5353

‎test/Interop/C/struct/noncopyable_structs_nontrivial.swift‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
// RUN: %target-swift-frontend -emit-sil -I %S/Inputs/ -I %swift_src_root/lib/ClangImporter/SwiftBridging %s -verify -DERRORS -verify-additional-prefix conly-
44
// RUN: %target-swift-frontend -emit-sil -I %S/Inputs/ -I %swift_src_root/lib/ClangImporter/SwiftBridging %s -verify -DERRORS -DCPLUSPLUS -verify-additional-prefix cplusplus- -cxx-interoperability-mode=default
55

6-
// XFAIL: OS=windows-msvc
76
import NoncopyableStructs
87

98
#if CPLUSPLUS

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

Lines changed: 24 additions & 2 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/Inputs %t/test.swift
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/Inputs %t/test.swift
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
55

66
//--- Inputs/module.modulemap
77
module Test {
@@ -68,6 +68,18 @@ MyPair<int, NonCopyableRequires> p7();
6868

6969
#endif
7070

71+
template<typename T>
72+
struct SWIFT_COPYABLE_IF(T) SWIFT_NONCOPYABLE DoubleAnnotation {};
73+
74+
using DoubleAnnotationInt = DoubleAnnotation<int>;
75+
76+
struct SWIFT_NONCOPYABLE NonCopyableNonMovable { // expected-note {{record 'NonCopyableNonMovable' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?}}
77+
NonCopyableNonMovable() {}
78+
NonCopyableNonMovable(const NonCopyableNonMovable& other) {}
79+
NonCopyableNonMovable(NonCopyableNonMovable&& other) = delete;
80+
};
81+
82+
7183
//--- test.swift
7284
import Test
7385
import CxxStdlib
@@ -107,3 +119,13 @@ func useOfRequires() {
107119
takeCopyable(p7()) // expected-cpp20-error {{global function 'takeCopyable' requires that 'MyPair<CInt, RequiresCopyableT<NonCopyable>>' conform to 'Copyable'}}
108120
}
109121
#endif
122+
123+
func doubleAnnotation() {
124+
let s = DoubleAnnotationInt()
125+
takeCopyable(s) // expected-error {{global function 'takeCopyable' requires that 'DoubleAnnotationInt' (aka 'DoubleAnnotation<CInt>') conform to 'Copyable'}}
126+
}
127+
128+
func missingLifetimeOperation() {
129+
let s = NonCopyableNonMovable() // expected-error {{cannot find 'NonCopyableNonMovable' in scope}}
130+
takeCopyable(s)
131+
}

0 commit comments

Comments
 (0)