[cxx-interop] Remove SWIFT_RETURNED_AS_RETAINED_BY_DEFAULT annotation #81329
+39
−94
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
fahadnayyar
requested review from
zoecarver,
egorzhdan,
Xazax-hun,
j-hui,
susmonteiro,
hnrklssn,
beccadax and
ian-twilightcoder
as code owners
egorzhdan
requested changes
May 6, 2025
DougGregor
approved these changes
May 6, 2025
egorzhdan
approved these changes
May 7, 2025
| @@ -8809,8 +8809,7 @@ swift::importer::getCxxRefConventionWithAttrs(const clang::Decl *decl) { | |||
| if (const clang::RecordDecl *record = | |||
| ptrType->getPointeeType()->getAsRecordDecl()) { | |||
| return matchSwiftAttrConsideringInheritance<RC>( | |||
There was a problem hiding this comment.
Not specific to this patch, but could you please remove matchSwiftAttrConsideringInheritance, etc. from the header and move them into the implementation file, since they aren't used anywhere outside of ClangImporter?
fahadnayyar
added a commit
to fahadnayyar/swift
that referenced
this pull request
May 7, 2025
…swiftlang#81329) This patch removes the `SWIFT_RETURNED_AS_RETAINED_BY_DEFAULT` annotation while maintaining the support for `SWIFT_RETURNED_AS_UNRETAINED_BY_DEFAULT`. These type-level annotations were initially introduced in [PR-81093](swiftlang#81093) to reduce the annotation burden in large C++ codebases where many C++ APIs returning `SWIFT_SHARED_REFERENCE` types are exposed to Swift. ### Motivation The original goal was to make C++ interop more ergonomic by allowing type-level defaults for ownership conventions for`SWIFT_SHARED_REFERENCE` types . However, defaulting to retained return values (+1) seems to be problematic and poses memory safety risks. ### Why we’re removing `SWIFT_RETURNED_AS_RETAINED_BY_DEFAULT` - **Memory safety risks:** Defaulting to retained can potentially lead to use-after-free bugs when the API implementation actually returns `unowned` (`+0`). These errors are subtle and can be hard to debug or discover, particularly in the absence of explicit API-level `SWIFT_RETURNS_(UN)RETAINED` annotations. - **Risky transitive behavior:** If a `SWIFT_SHARED_REFERENCE` type is annotated with `SWIFT_RETURNED_AS_RETAINED_BY_DEFAULT`, any new C++ API returning this type will inherit the retained behavior by default—even if the API's actual return behavior is unretained. Unless explicitly overridden with `SWIFT_RETURNS_UNRETAINED`, this can introduce a silent mismatch in ownership expectations and lead to use-after-free bugs. This is especially risky in large or evolving codebases where such defaults may be overlooked. - **Simpler multiple inheritance semantics:** With only one type-level default (`SWIFT_RETURNED_AS_UNRETAINED_BY_DEFAULT`), we avoid complications that can arise when multiple base classes specify conflicting ownership defaults. This simplifies reasoning about behavior in class hierarchies and avoids ambiguity when Swift determines the ownership convention for inherited APIs. ### Why we’re keeping `SWIFT_RETURNED_AS_UNRETAINED_BY_DEFAULT` - It still enables projects to suppress warnings for unannotated C++ APIs returning `SWIFT_SHARED_REFERENCE` types, helping to reduce noise while maintaining clarity. - It encourages explicitness for retained behavior. Developers must annotate retained return values with `SWIFT_RETURNS_RETAINED`, making ownership intent clearer and safer. - The worst-case outcome of assuming unretained when the return is actually retained is a memory leak, which is more tolerable and easier to debug than a use-after-free. - Having a single default mechanism improves clarity for documentation, diagnostics, and long-term maintenance of Swift/C++ interop code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch removes the
SWIFT_RETURNED_AS_RETAINED_BY_DEFAULTannotation while maintaining the support forSWIFT_RETURNED_AS_UNRETAINED_BY_DEFAULT. These type-level annotations were initially introduced in PR-81093 to reduce the annotation burden in large C++ codebases where many C++ APIs returningSWIFT_SHARED_REFERENCEtypes are exposed to Swift.Motivation
The original goal was to make C++ interop more ergonomic by allowing type-level defaults for ownership conventions for
SWIFT_SHARED_REFERENCEtypes . However, defaulting to retained return values (+1) seems to be problematic and poses memory safety risks.Why we’re removing
SWIFT_RETURNED_AS_RETAINED_BY_DEFAULTunowned(+0). These errors are subtle and can be hard to debug or discover, particularly in the absence of explicit API-levelSWIFT_RETURNS_(UN)RETAINEDannotations.SWIFT_SHARED_REFERENCEtype is annotated withSWIFT_RETURNED_AS_RETAINED_BY_DEFAULT, any new C++ API returning this type will inherit the retained behavior by default—even if the API's actual return behavior is unretained. Unless explicitly overridden withSWIFT_RETURNS_UNRETAINED, this can introduce a silent mismatch in ownership expectations and lead to use-after-free bugs. This is especially risky in large or evolving codebases where such defaults may be overlooked.SWIFT_RETURNED_AS_UNRETAINED_BY_DEFAULT), we avoid complications that can arise when multiple base classes specify conflicting ownership defaults. This simplifies reasoning about behavior in class hierarchies and avoids ambiguity when Swift determines the ownership convention for inherited APIs.Why we’re keeping
SWIFT_RETURNED_AS_UNRETAINED_BY_DEFAULTSWIFT_SHARED_REFERENCEtypes, helping to reduce noise while maintaining clarity.SWIFT_RETURNS_RETAINED, making ownership intent clearer and safer.