Skip to content

[cxx-interop] Default ownership convention as 3rd optional parameter of SWIFT_SHARED_REFERENCE annotation #79574

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

Closed
wants to merge 1 commit into from
Closed

[cxx-interop] Default ownership convention as 3rd optional parameter of SWIFT_SHARED_REFERENCE annotation #79574

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
17 changes: 17 additions & 0 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3664,6 +3664,23 @@ namespace {
unannotatedAPIWarningNeeded = true;
}

if (const auto *returnPtrTy = retType->getAs<clang::PointerType>()) {
clang::QualType returnPointeeType = returnPtrTy->getPointeeType();
if (const auto *returnRecordType =
returnPointeeType->getAs<clang::RecordType>()) {
clang::RecordDecl *returnRecordDecl = returnRecordType->getDecl();
for (const auto *attr : returnRecordDecl->getAttrs()) {
if (const auto *swiftAttr =
dyn_cast<clang::SwiftAttrAttr>(attr)) {
if (swiftAttr->getAttribute() == "ownership:unretained" ||
swiftAttr->getAttribute() == "ownership:retained") {
unannotatedAPIWarningNeeded = false;
}
}
}
}
}

if (unannotatedAPIWarningNeeded) {
HeaderLoc loc(decl->getLocation());
Impl.diagnose(loc, diag::no_returns_retained_returns_unretained,
Expand Down
24 changes: 18 additions & 6 deletions lib/ClangImporter/SwiftBridging/swift/bridging
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,19 @@
#define _CXX_INTEROP_CONCAT(...) \
_CXX_INTEROP_CONCAT_(__VA_ARGS__,,,,,,,,,,,,,,,,,)

/// Specifies that a C++ `class` or `struct` is reference-counted using
#define SWIFT_SHARED_REFERENCE_2ARGS(_retain, _release) \
__attribute__((swift_attr("import_reference"))) \
__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(retain:_retain)))) \
__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(release:_release))))

#define SWIFT_SHARED_REFERENCE_3ARGS(_retain, _release, _ownership) \
__attribute__((swift_attr("import_reference"))) \
__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(retain:_retain)))) \
__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(release:_release)))) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: formatting, the backslashes are not in the same column.

__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(ownership:_ownership))))

// FN_TODO: modify the documentation
/// Specifies that a C++ `class` or `struct` is reference-counted using
/// the given `retain` and `release` functions. This annotation lets Swift import
/// such a type as reference counted type in Swift, taking advantage of Swift's
/// automatic reference counting.
Expand All @@ -71,10 +83,10 @@
/// object.doSomething()
/// // The Swift compiler will release object here.
/// ```
#define SWIFT_SHARED_REFERENCE(_retain, _release) \
__attribute__((swift_attr("import_reference"))) \
__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(retain:_retain)))) \
__attribute__((swift_attr(_CXX_INTEROP_STRINGIFY(release:_release))))
#define _CXX_INTEROP_GET_MACRO(_1, _2, _3, NAME, ...) NAME
#define SWIFT_SHARED_REFERENCE(...) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to define this as (_retain, _release, ...)? That would make it more readable for someone opening this header file.

_CXX_INTEROP_GET_MACRO(__VA_ARGS__, SWIFT_SHARED_REFERENCE_3ARGS, \
SWIFT_SHARED_REFERENCE_2ARGS)(__VA_ARGS__)

/// Specifies that a C++ `class` or `struct` is a reference type whose lifetime
/// is presumed to be immortal, i.e. the reference to such object is presumed to
Expand Down Expand Up @@ -232,7 +244,7 @@
// Empty defines for compilers that don't support `attribute(swift_attr)`.
#define SWIFT_SELF_CONTAINED
#define SWIFT_RETURNS_INDEPENDENT_VALUE
#define SWIFT_SHARED_REFERENCE(_retain, _release)
#define SWIFT_SHARED_REFERENCE(_retain, _release, ...)
#define SWIFT_IMMORTAL_REFERENCE
#define SWIFT_UNSAFE_REFERENCE
#define SWIFT_NAME(_name)
Expand Down
38 changes: 30 additions & 8 deletions lib/SIL/IR/SILFunctionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1346,17 +1346,39 @@ class Conventions {
}

// Determines owned/unowned ResultConvention of the returned value based on
// returns_retained/returns_unretained attribute.
// SWIFT_RETURNS_(UN)RETAINED annotation on the C++ API and then based on
// ownership:(un)retained as the 3rd optional parameter of
// SWIFT_SHARED_REFERENCE annotation on the returned type
std::optional<ResultConvention>
getCxxRefConventionWithAttrs(const TypeLowering &tl,
const clang::Decl *decl) const {
if (tl.getLoweredType().isForeignReferenceType() && decl->hasAttrs()) {
for (const auto *attr : decl->getAttrs()) {
if (const auto *swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr)) {
if (swiftAttr->getAttribute() == "returns_unretained") {
return ResultConvention::Unowned;
} else if (swiftAttr->getAttribute() == "returns_retained") {
return ResultConvention::Owned;
if (tl.getLoweredType().isForeignReferenceType()) {
if (decl->hasAttrs()) {
for (const auto *attr : decl->getAttrs()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this pattern in our codebase so many times, I am wondering if we should have a helper function at this point.

Something like:

template<typename T>
std::optional<T> matchSwiftAttrs(const clang::Decl *d, ArrayRef<StringRef, T> matches) {
 ...
   if (swiftAttr->getAttribute() == matches[i].first)
     return matches[i].second; 
 ...
}

So we could simplify all of the code snippets like this to:

if (auto match = matchSwiftAttrs(decl, {{"returns_unretained", ResultConvention::Unowned},
            {"returns_retained", ResultConvention::Owned}})
    return match;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in PR-81093

if (const auto *swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr)) {
if (swiftAttr->getAttribute() == "returns_unretained") {
return ResultConvention::Unowned;
} else if (swiftAttr->getAttribute() == "returns_retained") {
return ResultConvention::Owned;
}
}
}
}
if (auto *classDecl = tl.getLoweredType()
.getASTType()
.getPointer()
->lookThroughAllOptionalTypes()
->getClassOrBoundGenericClass()) {
if (auto clangRecordDecl =
dyn_cast<clang::RecordDecl>(classDecl->getClangDecl())) {
for (const auto *attr : clangRecordDecl->getAttrs()) {
if (const auto *swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr)) {
if (swiftAttr->getAttribute() == "ownership:unretained") {
return ResultConvention::Unowned;
} else if (swiftAttr->getAttribute() == "ownership:retained") {
return ResultConvention::Owned;
}
}
}
}
}
Expand Down
104 changes: 104 additions & 0 deletions test/Interop/Cxx/foreign-reference/Inputs/cxx-functions-and-methods-returning-frt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#pragma once
#include "visibility.h"
#include <functional>

// FRT or SWIFT_SHARED_REFERENCE type
Expand Down Expand Up @@ -326,3 +327,106 @@ struct Derived : Base {
FRTStruct *_Nonnull VirtualMethodReturningFRTUnowned() override
__attribute__((swift_attr("returns_unretained")));
};

SWIFT_BEGIN_NULLABILITY_ANNOTATIONS

namespace DefaultOwnershipConventionOnCXXForegnRefType {
struct __attribute__((swift_attr("import_reference")))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for this change it would be important to add tests that are testing the bridging header directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not be relevant now in PR-81093. But feel free to leave a comment on the new PR if you feel strongly about this.

__attribute__((swift_attr("retain:defRetain1")))
__attribute__((swift_attr("release:defRelease1")))
__attribute__((swift_attr("ownership:retained"))) RefTypeDefaultRetained {};

RefTypeDefaultRetained *returnRefTypeDefaultRetained() {
return new RefTypeDefaultRetained();
}

struct __attribute__((swift_attr("import_reference")))
__attribute__((swift_attr("retain:defRetain2")))
__attribute__((swift_attr("release:defRelease2")))
__attribute__((swift_attr("ownership:unretained"))) RefTypeDefaultUnretained {};

RefTypeDefaultUnretained *returnRefTypeDefaultUnretained() {
return new RefTypeDefaultUnretained();
}
} // namespace DefaultOwnershipConventionOnCXXForegnRefType
void defRetain1(
DefaultOwnershipConventionOnCXXForegnRefType::RefTypeDefaultRetained *v) {};
void defRelease1(
DefaultOwnershipConventionOnCXXForegnRefType::RefTypeDefaultRetained *v) {};

void defRetain2(
DefaultOwnershipConventionOnCXXForegnRefType::RefTypeDefaultUnretained *v) {
};
void defRelease2(
DefaultOwnershipConventionOnCXXForegnRefType::RefTypeDefaultUnretained *v) {
};

namespace FunctionAnnotationHasPrecedence {
struct __attribute__((swift_attr("import_reference")))
__attribute__((swift_attr("retain:defaultRetain1")))
__attribute__((swift_attr("release:defaultRelease1")))
__attribute__((swift_attr("ownership:unretained"))) RefTypeDefaultUnRetained {};

RefTypeDefaultUnRetained *returnRefTypeDefaultUnRetained() {
return new RefTypeDefaultUnRetained();
}
RefTypeDefaultUnRetained *returnRefTypeDefaultUnRetainedAnnotatedRetained()
__attribute__((swift_attr("returns_retained"))) {
return new RefTypeDefaultUnRetained();
}

struct __attribute__((swift_attr("import_reference")))
__attribute__((swift_attr("retain:defaultRetain2")))
__attribute__((swift_attr("release:defaultRelease2")))
__attribute__((swift_attr("ownership:retained"))) RefTypeDefaultRetained {};

RefTypeDefaultRetained *returnRefTypeDefaultRetained() {
return new RefTypeDefaultRetained();
}
RefTypeDefaultRetained *returnRefTypeDefaultRetainedAnnotatedUnRetained()
__attribute__((swift_attr("returns_unretained"))) {
return new RefTypeDefaultRetained();
}

} // namespace FunctionAnnotationHasPrecedence

void defaultRetain1(
FunctionAnnotationHasPrecedence::RefTypeDefaultUnRetained *v) {};
void defaultRelease1(
FunctionAnnotationHasPrecedence::RefTypeDefaultUnRetained *v) {};

void defaultRetain2(
FunctionAnnotationHasPrecedence::RefTypeDefaultRetained *v) {};
void defaultRelease2(
FunctionAnnotationHasPrecedence::RefTypeDefaultRetained *v) {};

namespace DefaultOwnershipSuppressUnannotatedAPIWarning {

struct __attribute__((swift_attr("import_reference")))
__attribute__((swift_attr("retain:rretain")))
__attribute__((swift_attr("release:rrelease"))) RefType {};

RefType *returnRefType() { return new RefType(); }

struct __attribute__((swift_attr("import_reference")))
__attribute__((swift_attr("retain:dretain")))
__attribute__((swift_attr("release:drelease")))
__attribute__((swift_attr("ownership:retained"))) RefTypeDefaultRetained {};

RefTypeDefaultRetained *returnRefTypeDefaultRetained() {
return new RefTypeDefaultRetained();
}

} // namespace DefaultOwnershipSuppressUnannotatedAPIWarning

void rretain(DefaultOwnershipSuppressUnannotatedAPIWarning::RefType *v) {};
void rrelease(DefaultOwnershipSuppressUnannotatedAPIWarning::RefType *v) {};

void dretain(
DefaultOwnershipSuppressUnannotatedAPIWarning::RefTypeDefaultRetained *v) {
};
void drelease(
DefaultOwnershipSuppressUnannotatedAPIWarning::RefTypeDefaultRetained *v) {
};

SWIFT_END_NULLABILITY_ANNOTATIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: missing new line at the end of the file.

2 changes: 2 additions & 0 deletions test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes-error.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,5 @@ let f = FunctionVoidToFRTStruct()
let frt = f()
let nonFrt = NonFRTStruct()
let nonFrtLocalVar1 = global_function_returning_templated_retrun_frt_owned(nonFrt)
let _ = DefaultOwnershipSuppressUnannotatedAPIWarning.returnRefType()
let _ = DefaultOwnershipSuppressUnannotatedAPIWarning.returnRefTypeDefaultRetained()
20 changes: 20 additions & 0 deletions test/Interop/Cxx/foreign-reference/frt-retained-unretained-attributes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,23 @@ func testVirtualMethods(base: Base, derived: Derived) {
var frt4 = mutableDerived.VirtualMethodReturningFRTOwned()
// CHECK: function_ref @{{.*}}VirtualMethodReturningFRTOwned{{.*}} : $@convention(cxx_method) (@inout Derived) -> @owned FRTStruct
}

func testDefaultOwnershipAnnotation() {
let _ = DefaultOwnershipConventionOnCXXForegnRefType.returnRefTypeDefaultRetained()
// CHECK: function_ref {{.*}}returnRefTypeDefaultRetained{{.*}} : $@convention(c) () -> @owned DefaultOwnershipConventionOnCXXForegnRefType.RefTypeDefaultRetained

let _ = DefaultOwnershipConventionOnCXXForegnRefType.returnRefTypeDefaultUnretained()
// CHECK: function_ref {{.*}}returnRefTypeDefaultUnretained{{.*}} : $@convention(c) () -> DefaultOwnershipConventionOnCXXForegnRefType.RefTypeDefaultUnretained

let _ = FunctionAnnotationHasPrecedence.returnRefTypeDefaultUnRetained()
// CHECK: function_ref {{.*}}returnRefTypeDefaultUnRetained{{.*}} : $@convention(c) () -> FunctionAnnotationHasPrecedence.RefTypeDefaultUnRetained

let _ = FunctionAnnotationHasPrecedence.returnRefTypeDefaultUnRetainedAnnotatedRetained()
// CHECK: function_ref {{.*}}returnRefTypeDefaultUnRetainedAnnotatedRetained{{.*}} : $@convention(c) () -> @owned FunctionAnnotationHasPrecedence.RefTypeDefaultUnRetained

let _ = FunctionAnnotationHasPrecedence.returnRefTypeDefaultRetained()
// CHECK: function_ref {{.*}}returnRefTypeDefaultRetained{{.*}} : $@convention(c) () -> @owned FunctionAnnotationHasPrecedence.RefTypeDefaultRetained

let _ = FunctionAnnotationHasPrecedence.returnRefTypeDefaultRetainedAnnotatedUnRetained()
// CHECK: function_ref {{.*}}returnRefTypeDefaultRetainedAnnotatedUnRetained{{.*}} : $@convention(c) () -> FunctionAnnotationHasPrecedence.RefTypeDefaultRetained
}