[cxx-interop] Add SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED…

[fahadnayyar]

Adding SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED annotations to specify ownership of FRT returned by a C++ method or function. The default behaviour without these annotations remain unchanged.

rdar://135306908

[fahadnayyar]

@swift-ci please smoke test

[fahadnayyar]

@swift-ci please smoke test

[Xazax-hun]

High-level question: do we need both of these? If we have a default behavior, we might be able drop the one that matches that. That being said, I don't have strong feelings about this, since these annotations can also serve as documentation and users can also use it to hedge against the defaults changing.

[Xazax-hun]

Ah, never mind. So the default might be based on naming conventions, so the user needs to be able to override the defaults in both directions.

[egorzhdan]

The overall approach seems reasonable to me, I have a few comments.

[Xazax-hun]

This looks good to me as well, I only mentioned some small nits.

[fahadnayyar]

@swift-ci please smoke test

[fahadnayyar]

@swift-ci please smoke test

[fahadnayyar]

@rjmccall @DougGregor

In this patch I am adding two new macros SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED and implementing these as swift_attr attributes with strings "returns_retained" and "returns_unretained" respectively. Some of our clients use these attributes as a public interface because they are unable to use the bridging header defining these marcos. Swift compiler already has some support for clang attributes cf_returns_retained and cf_returns_unretained for C++ functions and methods. But their semantics would be different for C++ as compared to CF. In CF return types cf_returns_unretained means returning the value as autoreleased while for c++ SWIFT_SHARED_REFERENCE types it means returning as unowned. Also using CF_ attributes and macros on C++ types seemed a bit odd to me as we want these attributes to be used by everyone and not just internally. Another source of confusion is that cf_returns_retained has effect on c++ methods and functions but ns_returns_retained has no effect. And in the clang documentation it is not clear if these attributes can be used for C++ methods and functions as well.

I wanted to get your opinion on whether we should use the existing clang attributes cf_returns_retained and cf_returns_unretained as the public interface to specify the ownership of C++ foreign reference types return values or implement new swift_attr("returns_retained") and swift_attr("returns_unretained") as I am doing in this patch?

[egorzhdan]

One important detail there is that we think that the attributes would need to change in the future, regardless of whether we use the existing cf_returns_retained or add a new swift_attr("returns_retained") attribute now.

To provide Clang diagnostics and Clang static analyzer support for these attributes, similarly to the existing support for cf_returns_retained, we would need to make them first-class Clang attributes, since Clang isn't aware of the contents of a swift_attr, and the cf_ attributes have slightly different semantic meaning (they assume usage of CFRetain/CFRelease, which is not something we use for C++ reference types).

[ahatanaka]

Another source of confusion is that cf_returns_retained has effect on c++ methods and functions but ns_returns_retained has no effect.

ns_returns_retained has effect on C++ methods and functions too if the return type is an ObjC object.

@class C;

struct S {
  C *f() __attribute__((ns_returns_retained));
};

https://releases.llvm.org/3.5.0/tools/clang/docs/AutomaticReferenceCounting.html#retained-return-values

[fahadnayyar]

@rjmccall @DougGregor

In this patch I am adding two new macros SWIFT_RETURNS_RETAINED and SWIFT_RETURNS_UNRETAINED and implementing these as swift_attr attributes with strings "returns_retained" and "returns_unretained" respectively. Some of our clients use these attributes as a public interface because they are unable to use the bridging header defining these marcos. Swift compiler already has some support for clang attributes cf_returns_retained and cf_returns_unretained for C++ functions and methods. But their semantics would be different for C++ as compared to CF. In CF return types cf_returns_unretained means returning the value as autoreleased while for c++ SWIFT_SHARED_REFERENCE types it means returning as unowned. Also using CF_ attributes and macros on C++ types seemed a bit odd to me as we want these attributes to be used by everyone and not just internally. Another source of confusion is that cf_returns_retained has effect on c++ methods and functions but ns_returns_retained has no effect. And in the clang documentation it is not clear if these attributes can be used for C++ methods and functions as well.

I wanted to get your opinion on whether we should use the existing clang attributes cf_returns_retained and cf_returns_unretained as the public interface to specify the ownership of C++ foreign reference types return values or implement new swift_attr("returns_retained") and swift_attr("returns_unretained") as I am doing in this patch?

Another reason why CF_ attributes are confusing for C++ methods and functions is that, for CF types there is also a create/copy rule which applied even if the CF_ attributes are not present. CF functions having either create or copy in their name pass the returned value as owned. I feel we should in future deprecate the use of CF_ macros and cf_ attributes for C++ functions and methods to avoid such confusions. Currently the create/copy rule is also applied to C++ function and method which is a bug reported by some of our clients. We are planning to fix the default behavior (in the absence of any attributes) in a follow up patch.

Ref: https://releases.llvm.org/3.5.0/tools/clang/docs/AutomaticReferenceCounting.html#id18

[fahadnayyar]

@swift-ci please smoke test

[egorzhdan]

Overall this looks good, I only have one minor comment.

[fahadnayyar]

@swift-ci please smoke test

[fahadnayyar]

Another source of confusion is that cf_returns_retained has effect on c++ methods and functions but ns_returns_retained has no effect.

ns_returns_retained has effect on C++ methods and functions too if the return type is an ObjC object.

@class C;

struct S {
  C *f() __attribute__((ns_returns_retained));
};

https://releases.llvm.org/3.5.0/tools/clang/docs/AutomaticReferenceCounting.html#retained-return-values

@ahatanaka I feel we should use these classes of annotations CF_ NS_ SWIFT_ as per the return type of C++ functions and/or methods:
See: rdar://135362223

We have 3 class of annotations:
CF_RETURNS_{RETAINED/UNRETAINED} -> handles return type of CF objects
NS_RETURNS_{RETAINED/UNRETAINED} -> handles return type of ObjC objects
SWIFT_RETURNS_{RETAINED/UNRETAINED} -> handles return type of C++ foreign reference type objects

They should have effect on C++ method, functions, etc only if the return type is of their respective category. Otherwise we should warn or error out in the ClangImporter.

We can have additional testing to make sure that behaviour is changed only for C++ functions and methods and not for C, ObjC, CF, etc functions.

Let me know if you have thoughts.

[egorzhdan]

LGTM