Skip to content

[cxx-interop] Warning unannotated C++ APIs returning SWIFT_SHARED_REFERENCE types #76798

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

Jump to bottom
Merged
merged 1 commit into from
Nov 1, 2024
Merged

[cxx-interop] Warning unannotated C++ APIs returning SWIFT_SHARED_REFERENCE types #76798

merged 1 commit into from
Nov 1, 2024

Conversation

fahadnayyar
Copy link
Contributor

@fahadnayyar fahadnayyar commented Oct 1, 2024 •
edited
Loading

Adding a warning when C++ APIs (functions or methods) returning C++ foreign reference types or SWIFT_SHARED_REFERENCE types are not annotated with either of SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED.

rdar://139225276

@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

ravikandhadai
ravikandhadai reviewed Oct 1, 2024
View reviewed changes
egorzhdan
egorzhdan reviewed Oct 2, 2024
View reviewed changes
@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Oct 2, 2024
@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

@fahadnayyar fahadnayyar force-pushed the frt-return-unannotated-warning branch from 2bc6ed0 to ca1094c Compare October 17, 2024 04:30
@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

@fahadnayyar
Copy link
Contributor Author

@swift-ci Please Build Toolchain

@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

@fahadnayyar
Copy link
Contributor Author

@swift-ci Please Build Toolchain macOS

@fahadnayyar fahadnayyar mentioned this pull request Oct 17, 2024
Draft
fahadnayyar
fahadnayyar commented Oct 28, 2024
View reviewed changes
@fahadnayyar fahadnayyar force-pushed the frt-return-unannotated-warning branch from ca1094c to 92b7417 Compare October 28, 2024 22:38
@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

@fahadnayyar fahadnayyar force-pushed the frt-return-unannotated-warning branch from 92b7417 to ad4df73 Compare October 29, 2024 02:38
@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test

@fahadnayyar fahadnayyar marked this pull request as ready for review October 29, 2024 02:41
Xazax-hun
Xazax-hun reviewed Oct 29, 2024
View reviewed changes
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.

Mostly looks good to me, some nits inline, and I think some of the TODOs should be resolved in this PR (not in a follow-up).

egorzhdan
egorzhdan reviewed Oct 29, 2024
View reviewed changes
@fahadnayyar fahadnayyar force-pushed the frt-return-unannotated-warning branch from ad4df73 to e753bf6 Compare October 30, 2024 00:28
@fahadnayyar fahadnayyar force-pushed the frt-return-unannotated-warning branch from e753bf6 to e640ed6 Compare October 30, 2024 00:31
@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test

@fahadnayyar
Copy link
Contributor Author

@swift-ci please test

Xazax-hun
Xazax-hun approved these changes Oct 31, 2024
View reviewed changes
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.

Once Egor is happy about the placement of the new utility functions, I think this should be good to go.

egorzhdan
egorzhdan requested changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

I have one question regarding C++ pointers vs references. Otherwise this change would look good to me.

@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test windows

@fahadnayyar fahadnayyar enabled auto-merge October 31, 2024 17:11
j-hui
j-hui approved these changes Oct 31, 2024
View reviewed changes
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 PR looks good to me, with the disclaimer that I don't yet have much experience or confidence navigating this codebase yet. I left a couple of nits related to naming conventions.

I thought of an edge case that might be out of the scope of this PR: how would Swift/Clang treat multiple function declarations with different annotations? e.g.,:

T *_Nonnull f();
T *_Nonnull f() __attribute__((swift_attr("returns_retained")));
T *_Nonnull f() __attribute__((swift_attr("returns_unretained")));

Are the attributes coalesced across the different declarations?

@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test windows

@fahadnayyar fahadnayyar merged commit f430088 into swiftlang:main Nov 1, 2024
5 checks passed
This was referenced Nov 5, 2024
Open
Merged
@fahadnayyar fahadnayyar mentioned this pull request Dec 5, 2024
Merged
@fahadnayyar fahadnayyar changed the title [cxx-interop] Warning unannotated C++ APIs returning SWIFT_SHARED_REF… [cxx-interop] Warning unannotated C++ APIs returning SWIFT_SHARED_REFERENCE types Dec 9, 2024
@fahadnayyar fahadnayyar mentioned this pull request Feb 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ravikandhadai ravikandhadai ravikandhadai left review comments

@Xazax-hun Xazax-hun Xazax-hun approved these changes

@egorzhdan egorzhdan egorzhdan approved these changes

@j-hui j-hui j-hui approved these changes

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@hyp hyp Awaiting requested review from hyp

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

@ian-twilightcoder ian-twilightcoder Awaiting requested review from ian-twilightcoder ian-twilightcoder is a code owner

@hborla hborla Awaiting requested review from hborla

@slavapestov slavapestov Awaiting requested review from slavapestov

@xedin xedin Awaiting requested review from xedin

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fahadnayyar @Xazax-hun @egorzhdan @j-hui @ravikandhadai