Skip to content

[cxx-interop] Introduce type-level annotations to specify default ownership convention for C++ foreign reference return values #81093

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fahadnayyar
Copy link
Contributor

@fahadnayyar fahadnayyar commented Apr 25, 2025

In Swift 6.1, we introduced SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED annotations for C++ APIs to explicitly specify the ownership convention of SWIFT_SHARED_REFERENCE type return values.

Currently the Swift compiler emits warnings for unannotated C++ APIs returning SWIFT_SHARED_REFERENCE types. We've received some feedback that people are finding these warnings useful to get a reminder to annotate their APIs. While this improves correctness , it also imposes a high annotation burden on adopters — especially in large C++ codebases.

This patch addresses that burden by introducing two new type-level annotations:

  • SWIFT_RETURNS_RETAINED_BY_DEFAULT
  • SWIFT_RETURNS_UNRETAINED_BY_DEFAULT

These annotations allow developers to specify a default ownership convention for all C++ APIs returning a given SWIFT_SHARED_REFERENCE-annotated type, unless explicitly overridden at the API by using SWIFT_RETURNS_RETAINED or SWIFT_RETURNS_UNRETAINED. If a C++ class inherits from a base class annotated with SWIFT_RETURNS_RETAINED_BY_DEFAULT or SWIFT_RETURNS_UNRETAINED_BY_DEFAULT, the derived class automatically inherits the default ownership convention unless it is explicitly overridden. This strikes a balance between safety/correctness and usability:

  • It avoids the need to annotate every API individually.
  • It retains the ability to opt out of the default at the API level when needed.
  • To verify correctness, the user can just remove the SWIFT_RETURNS_(UN)RETAINED_BY_DEFAULT annotation from that type and they will start seeing the warnings on all the unannotated C++ APIs returning that SWIFT_SHARED_REFERENCE type. They can add SWIFT_RETURNS_(UN)RETAINED annotation at each API in which they want a different behaviour than the default. Then they can reintroduce the SWIFT_RETURNS_(UN)RETAINED_BY_DEFAULT at the type level to suppress the warnings on remaining unannotated APIs.

A global default ownership convention (like always return unretained/unowned) was considered but it would weaken the diagnostic signal and remove valuable guardrails that help detect use-after-free bugs and memory leaks in absence of SWIFT_RETURNS_(UN)RETAINED annotations. In the absence of these annotations when Swift emits the unannotated API warning, the current fallback behavior (e.g. relying on heuristics based on API name such as "create", "copy", "get") is derived from Objective-C interop but is ill-suited for C++, which has no consistent naming patterns for ownership semantics.

Several codebases are expected to have project-specific conventions, such as defaulting to unretained except for factory methods and constructors. A type-level default seems like the most precise and scalable mechanism to support such patterns. It integrates cleanly with existing SWIFT_SHARED_REFERENCE usage and provides a per-type opt-in mechanism without global silencing of ownership diagnostics.

This addition improves ergonomics while preserving the safety benefits of explicit annotations and diagnostics.

rdar://145453509

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 tests are not passing at the moment.

Also this does not seem to address inheritance. I think it would be really unintuitive that the SHARED_REFERENCE annotations can be inherited but this does not. I suspect that it might not be too much work to support inheritance in this PR so I'd recommend including it in this one rather than doing follow-up work.

@fahadnayyar
Copy link
Contributor Author

Also this does not seem to address inheritance. I think it would be really unintuitive that the SHARED_REFERENCE annotations can be inherited but this does not. I suspect that it might not be too much work to support inheritance in this PR so I'd recommend including it in this one rather than doing follow-up work.

Thanks for bringing this up — I agree that inheritance behavior is an important aspect to consider. That said, I suspect supporting inheritance semantics here could introduce a non-trivial amount of complexity and might make this initial patch harder to review.

The practical downside of not supporting inheritance right away is relatively limited: diagnostics would still be emitted for unannotated APIs returning derived types. In those cases, users can continue to fall back on explicit SWIFT_RETURNS_(UN)RETAINED annotations as needed.

Also, I’m not yet convinced that default ownership annotations should be inherited. One of the core arguments for requiring type-level annotations is that users are already expected to explicitly mark types with SWIFT_SHARED_REFERENCE. They are meant to be a way to suppress diagnostics for a particular C++ foreign reference type as return values of C++ APIs referred from Swift. If we make these annotations also inherited then there might be risk of missing the warnings at some important API.

We've had some internal discussion where opinions have varied on whether ownership defaults should propagate to subclasses, so I think we’d benefit from a broader design conversation before baking inheritance behavior into the model.
Happy to revisit this shortly after landing the base support — especially if we agree on the direction in an offline discussion.

PS: We've added the implicit inheritance of SWIFT_SHARED_REFERENCE very recently and it will ship with Swift 6.2. So till now SWIFT_SHARED_REFERENCE was also not inherited implicitly.

@fahadnayyar fahadnayyar requested a review from DougGregor April 25, 2025 15:56
@Xazax-hun
Copy link
Contributor

Xazax-hun commented Apr 25, 2025

I suspect supporting inheritance semantics here could introduce a non-trivial amount of complexity

What complexity do you anticipate? We already support inheritance for SWIFT_SHARED_REFERENCE. I'd expect we could extend that code to also look for this annotation without any big structural changes. Is there a reason why that would not work?

downside of not supporting inheritance right away is relatively limited

The main goal of this change is to reduce the annotation burden. In case inheritance is not supported it might not be able to reduce the burden to an acceptable level. Moreover, it is confusing if this has different semantics than SWIFT_SHARED_REFERENCE.

We've had some internal discussion where opinions have varied on whether ownership defaults should propagate to subclasses

What discussion? If you refer to Wednesday's meeting I remember most people arguing for inheritance.

@Xazax-hun
Copy link
Contributor

broader design conversation

I thought this already happened. People said that if the conventions are different between the base class and the derived classes that would break the substitution principle.

Consider an API:

Base *getObject();

If the convention can change depending on whether it returns a pointer to Base* or Derived* all the callers would need to look at the dynamic type information and do the reference counting conditionally, and an API like this would be inherently incompatible with automatic reference counting.

@egorzhdan
Copy link
Contributor

I agree that this patch should ideally address inheritance if possible.

@fahadnayyar fahadnayyar force-pushed the cxx-frt-default-returns-unretained branch from ac02517 to 55cb7cd Compare April 29, 2025 04:57
@fahadnayyar fahadnayyar force-pushed the cxx-frt-default-returns-unretained branch 2 times, most recently from 51e0d54 to 42a2e0e Compare April 29, 2025 06:09
@fahadnayyar fahadnayyar force-pushed the cxx-frt-default-returns-unretained branch from 42a2e0e to f52ef8b Compare April 29, 2025 06:22
@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test

@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test linux

@fahadnayyar
Copy link
Contributor Author

@swift-ci please smoke test macos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants