Skip to content

[cxx-interop] Add support for SWIFT_RETURNS_(UN)RETAINED for ObjC APIs returning C++ FRT #78230

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.

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

Conversation

fahadnayyar
Copy link
Contributor

Extending #75897 so that the new annotations SWIFT_RETURNS_(UN)RETAINED can be used on ObjC APIs as well.

rdar://135360972

Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

The code changes look good to me. I only have some comments about testing inline.

Copy link
Contributor

@j-hui j-hui left a comment

Choose a reason for hiding this comment

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

This patch looks good to me, though as @Xazax-hun suggested you should split out the ObjC/Foundation test code into a separate test so that it only runs on supported platforms.

@j-hui
Copy link
Contributor

j-hui commented Dec 17, 2024

By the way, without your patch, what is the behavior of the Swift compiler when you try to compile ObjC code annotated with SWIFT_RETURNS_{,UN}RETAINED? Does it lead to memory leaks, due to the caller assuming it does not own the returned value?

If so, you could clarify your commit message to specify that this patch fixes support for SWIFT_RETURNS_{,UN}RETAINED in ObjC.

@fahadnayyar fahadnayyar force-pushed the returns-retained-objc-support branch from cbd4b3a to 426d471 Compare December 18, 2024 01:32
@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test

@fahadnayyar
Copy link
Contributor Author

By the way, without your patch, what is the behavior of the Swift compiler when you try to compile ObjC code annotated with SWIFT_RETURNS_{,UN}RETAINED? Does it lead to memory leaks, due to the caller assuming it does not own the returned value?

If so, you could clarify your commit message to specify that this patch fixes support for SWIFT_RETURNS_{,UN}RETAINED in ObjC.

SWIFT_RETURNS_{,UN}RETAINED is a new feature. It is implemented as a swift_attr so before this patch also, people could've used these annotations of ObjC APIs. But that would have been a no-op (same behaviour as without these annotations). We are not changing the default behavior (for unannotated case) in this patch which is returning the value as unowned (+0). It can lead to memory leaks if used incorrectly but that problem always existed when returning SWIFT_SHARED_REFERENCE types from ObjC APIs. With these new annotations developers could be more mindful about ownership semantics and also explicitly specify that to the swift compiler.

@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

LGTM!

@fahadnayyar fahadnayyar merged commit 0f2637c into swiftlang:main Dec 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants