[cxx-interop] Add attribute to hide Swift declarations from interop #82616
Conversation
Xazax-hun
requested review from
zoecarver,
egorzhdan,
j-hui,
fahadnayyar,
susmonteiro,
hnrklssn and
hborla
as code owners
Xazax-hun
requested review from
slavapestov,
xedin,
bnbarham,
CodaFi,
hamishknight and
rintaro
as code owners
include/swift/AST/DeclAttr.def
Outdated
| 133) | ||
|
|
||
| // Unused '134' | ||
| SIMPLE_DECL_ATTR(_swiftPrivate, SwiftPrivate, |
There was a problem hiding this comment.
We also have a swift_private attribute on the C++ side. Let me know if you think this would make this confusing (and potentially suggest an alternative name like doNotExpose, swiftOnly, disableInterop).
There was a problem hiding this comment.
I wonder if we could reuse the existing @_expose attribute here with a slightly extended syntax, e.g. @_expose(!Cxx). Let's have a discussion about this.
There was a problem hiding this comment.
First I disliked @_expose(!Cxx) as I was wondering if it could be interpreted as "this declaration is exposed to everything expect for C++", so as if we wrote @_expose(Wasm). I thought about something like @expose(None) but that is problematic as we might want to support exposing something to Webassembly but disable exposing to C++.
There was a problem hiding this comment.
@DougGregor Was wondering if you have some opinions here.
There was a problem hiding this comment.
An alternative idea is to have "Nil" instead of the function name as a way to say not to expose it.
There was a problem hiding this comment.
Looks like the @_expose(Wasm) has a slightly different meaning. It means that the function symbol should be exported from the generated shared library. This is required because in wasm everything is internal by default. So it is also possible that the Wasm and Cxx case should be divorced into two separate attributes and the Wasm one should be called something like export.
There was a problem hiding this comment.
Personally, I like @_expose(!Cxx) because it puts the "expose to another language" functionality under one attribute that we can document.
There was a problem hiding this comment.
Any opinion on !Cxx, NotCxx, or ~Cxx?
There was a problem hiding this comment.
I updated the PR to use !Cxx.
Xazax-hun
force-pushed
the
gaborh/attr-hide-swift-decls
branch
from
099b2d7 to
291a1f3
Compare
There was a problem hiding this comment.
This LGTM.
I do have a question about an edge case though: what happens if someone writes @_expose(!Cxx, "nonsense")? This name is nonsensical since we're saying to not expose the declaration, so we're not going to use it. It's probably harmless, but I'm wondering if we should throw a warning or error if we encounter this.
Xazax-hun
force-pushed
the
gaborh/attr-hide-swift-decls
branch
from
291a1f3 to
326580e
Compare
Good point, added a warning, thanks! |
Xazax-hun
force-pushed
the
gaborh/attr-hide-swift-decls
branch
from
326580e to
62a2a6a
Compare
|
@swift-ci please smoke test |
This can help work around problems when the names of a C++ declaration and a Swift declaration would collide, or to temporarily work around compiler bugs. rdar://152838988&140802127&158843666
Xazax-hun
force-pushed
the
gaborh/attr-hide-swift-decls
branch
from
62a2a6a to
7ac9a81
Compare
|
@swift-ci please smoke test |
Good point! Worked for me locally out of the box but started a CI test to see if that is universally the case: swiftlang/swift-syntax#3145 |
Explanation: We generate declarations in the C++ interop header with "unavailable" annotations when we cannot export something to C++. These declarations can collide with existing names. Previously, there were no ways to resolve these name collisions. This PR introduces a new attribute to hide declarations from the interop header. Issues: rdar://158843666 Original PRs: swiftlang#82616 Risk: Low, this adds a new, straightforward code path. Testing: Added a compiler test. Reviewers: @egorzhdan
swiftlang#82616 added a new exposure kind for the `@_expose` attribute, but did not update the serialization format to account for the new kind. This caused a crash when serializing a module that used `@_expose(wasm)`.
swiftlang#82616 added a new exposure kind for the `@_expose` attribute, but did not update the serialization format to account for the new kind. This caused a crash when serializing a module that used `@_expose(wasm)`.
swiftlang#82616 added a new exposure kind for the `@_expose` attribute, but did not update the serialization format to account for the new kind. This caused a crash when serializing a module that used `@_expose(wasm)`.
This can help work around problems when the names of a C++ declaration and a Swift declaration would collide, or to temporarily work around compiler bugs.
rdar://152838988&140802127&158843666