Skip to content

[cxx-interop] SWIFT_NONCOPYABLE overlooked in some cases #84517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 30, 2025
+76 −53
Merged

[cxx-interop] SWIFT_NONCOPYABLE overlooked in some cases #84517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 46 additions & 45 deletions lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8237,17 +8237,15 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
}

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

return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
return ctor->isMoveConstructor() &&
return ctor->isMoveConstructor() && !ctor->isDeleted() &&
!ctor->isIneligibleOrNotSelected() &&
// FIXME: Support default arguments (rdar://142414553)
ctor->getNumParams() == 1;
ctor->getNumParams() == 1 &&
ctor->getAccess() == clang::AS_public;
});
}

Expand Down Expand Up @@ -8379,46 +8377,48 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
return CxxValueSemanticsKind::Copyable;
}

auto injectedStlAnnotation =
recordDecl->isInStdNamespace()
? STLConditionalParams.find(recordDecl->getName())
: STLConditionalParams.end();
auto STLParams = injectedStlAnnotation != STLConditionalParams.end()
? injectedStlAnnotation->second
: std::vector<int>();
auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);

if (!STLParams.empty() || !conditionalParams.empty()) {
HeaderLoc loc{recordDecl->getLocation()};
std::function checkArgValueSemantics =
[&](clang::TemplateArgument &arg,
StringRef argToCheck) -> std::optional<CxxValueSemanticsKind> {
if (arg.getKind() != clang::TemplateArgument::Type && importerImpl) {
importerImpl->diagnose(loc, diag::type_template_parameter_expected,
argToCheck);
return CxxValueSemanticsKind::Unknown;
}
if (!hasNonCopyableAttr(recordDecl)) {
auto injectedStlAnnotation =
recordDecl->isInStdNamespace()
? STLConditionalParams.find(recordDecl->getName())
: STLConditionalParams.end();
auto STLParams = injectedStlAnnotation != STLConditionalParams.end()
? injectedStlAnnotation->second
: std::vector<int>();
auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);

auto argValueSemantics = evaluateOrDefault(
evaluator,
CxxValueSemantics(
{arg.getAsType()->getUnqualifiedDesugaredType(), importerImpl}),
{});
if (argValueSemantics != CxxValueSemanticsKind::Copyable)
return argValueSemantics;
return std::nullopt;
};
if (!STLParams.empty() || !conditionalParams.empty()) {
HeaderLoc loc{recordDecl->getLocation()};
std::function checkArgValueSemantics =
[&](clang::TemplateArgument &arg,
StringRef argToCheck) -> std::optional<CxxValueSemanticsKind> {
if (arg.getKind() != clang::TemplateArgument::Type && importerImpl) {
importerImpl->diagnose(loc, diag::type_template_parameter_expected,
argToCheck);
return CxxValueSemanticsKind::Unknown;
}

auto result = checkConditionalParams<CxxValueSemanticsKind>(
recordDecl, STLParams, conditionalParams, checkArgValueSemantics);
if (result.has_value())
return result.value();
auto argValueSemantics = evaluateOrDefault(
evaluator,
CxxValueSemantics(
{arg.getAsType()->getUnqualifiedDesugaredType(), importerImpl}),
{});
if (argValueSemantics != CxxValueSemanticsKind::Copyable)
return argValueSemantics;
return std::nullopt;
};

if (importerImpl)
for (auto name : conditionalParams)
importerImpl->diagnose(loc, diag::unknown_template_parameter, name);
auto result = checkConditionalParams<CxxValueSemanticsKind>(
recordDecl, STLParams, conditionalParams, checkArgValueSemantics);
if (result.has_value())
return result.value();

return CxxValueSemanticsKind::Copyable;
if (importerImpl)
for (auto name : conditionalParams)
importerImpl->diagnose(loc, diag::unknown_template_parameter, name);

return CxxValueSemanticsKind::Copyable;
}
}

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

bool isCopyable = hasCopyTypeOperations(cxxRecordDecl);
bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
hasCopyTypeOperations(cxxRecordDecl);
bool isMovable = hasMoveTypeOperations(cxxRecordDecl);

if (!hasDestroyTypeOperations(cxxRecordDecl) || (!isCopyable && !isMovable)) {
Expand Down
9 changes: 5 additions & 4 deletions lib/ClangImporter/SwiftDeclSynthesizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3011,10 +3011,8 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
ctx.evaluator,
CxxValueSemantics({clangType->getTypeForDecl(), &ImporterImpl}), {});

if (valueSemanticsKind == CxxValueSemanticsKind::MoveOnly)
return destroyFunc;

if (valueSemanticsKind != CxxValueSemanticsKind::Copyable)
if (valueSemanticsKind != CxxValueSemanticsKind::Copyable &&
valueSemanticsKind != CxxValueSemanticsKind::MoveOnly)
return nullptr;

auto cxxRecordSemanticsKind = evaluateOrDefault(
Expand All @@ -3033,6 +3031,9 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
}
}

if (valueSemanticsKind == CxxValueSemanticsKind::MoveOnly)
return destroyFunc;

markDeprecated(
nominal,
"destroy operation '" +
Expand Down
2 changes: 1 addition & 1 deletion test/Interop/C/struct/Inputs/noncopyable-struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void badDestroy2(BadDestroyNonCopyableType2 *ptr);

struct __attribute__((swift_attr("~Copyable"))) __attribute__((swift_attr("destroy:extraDestroy"))) ExtraDestroy {
void *storage;

ExtraDestroy(ExtraDestroy&&) = default;
~ExtraDestroy() { }
};

Expand Down
1 change: 0 additions & 1 deletion test/Interop/C/struct/noncopyable_structs_nontrivial.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// RUN: %target-swift-frontend -emit-sil -I %S/Inputs/ -I %swift_src_root/lib/ClangImporter/SwiftBridging %s -verify -DERRORS -verify-additional-prefix conly-
// 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

// XFAIL: OS=windows-msvc
import NoncopyableStructs

#if CPLUSPLUS
Expand Down
26 changes: 24 additions & 2 deletions test/Interop/Cxx/class/noncopyable-typechecker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// RUN: %empty-directory(%t)
// RUN: split-file %s %t
// RUN: %target-swift-frontend -cxx-interoperability-mode=default -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t/Inputs %t/test.swift
// 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
// 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
// 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

//--- Inputs/module.modulemap
module Test {
Expand Down Expand Up @@ -68,6 +68,18 @@ MyPair<int, NonCopyableRequires> p7();

#endif

template<typename T>
struct SWIFT_COPYABLE_IF(T) SWIFT_NONCOPYABLE DoubleAnnotation {};
Copy link
Member

@hnrklssn hnrklssn Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to diagnose this? Or do we expect users to find themselves in a situation where this is useful?
Also could you add a test that checks that SWIFT_COPYABLE_IF(garbage) SWIFT_NONCOPYABLE DoubleAnnotation still diagnoses the invalid attribute?

Copy link
Contributor Author

@susmonteiro susmonteiro Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will be useful, but it's also not problematic that the user adds both. If we diagnose this, we should also diagnose the case where there's two or more SWIFT_COPYABLE_IF annotations in the same type (we currently merge all the parameters of these annotation) or when the user adds two or more SWIFT_NONCOPYABLE.

Copy link
Contributor Author

@susmonteiro susmonteiro Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the test you suggested, in the current implementation if there's a SWIFT_NONCOPYABLE we never even look at SWIFT_COPYABLE_IF, so we wouldn't diagnose the invalid attribute. I'm positive the same happens with escapable annotations: if a SWIFT_ESCAPABLE or SWIFT_NONESCAPABLE is present, we never parse the SWIFT_ESCAPABLE_IF

Copy link
Member

@hnrklssn hnrklssn Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should diagnose swift attributes that are obviously not used as intended.

Copy link
Contributor Author

@susmonteiro susmonteiro Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I opened an issue: #84559


using DoubleAnnotationInt = DoubleAnnotation<int>;

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?}}
NonCopyableNonMovable() {}
NonCopyableNonMovable(const NonCopyableNonMovable& other) {}
NonCopyableNonMovable(NonCopyableNonMovable&& other) = delete;
};


//--- test.swift
import Test
import CxxStdlib
Expand Down Expand Up @@ -107,3 +119,13 @@ func useOfRequires() {
takeCopyable(p7()) // expected-cpp20-error {{global function 'takeCopyable' requires that 'MyPair<CInt, RequiresCopyableT<NonCopyable>>' conform to 'Copyable'}}
}
#endif

func doubleAnnotation() {
let s = DoubleAnnotationInt()
takeCopyable(s) // expected-error {{global function 'takeCopyable' requires that 'DoubleAnnotationInt' (aka 'DoubleAnnotation<CInt>') conform to 'Copyable'}}
}

func missingLifetimeOperation() {
let s = NonCopyableNonMovable() // expected-error {{cannot find 'NonCopyableNonMovable' in scope}}
takeCopyable(s)
}