Skip to content
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

No way to refine types/enums/functions for Swift #179

Open
litherum opened this issue May 13, 2023 · 14 comments
Open

No way to refine types/enums/functions for Swift #179

litherum opened this issue May 13, 2023 · 14 comments
Assignees
Labels
non-breaking Does not require a breaking change (that would block V1.0)

Comments

@litherum
Copy link

litherum commented May 13, 2023

Frameworks can expose C symbols (of course). However, when projected into Swift, those C symbols are usually not very ergonomic. One example is asynchronous functions: In C, they take a function pointer and a void* userdata. If the same function was to be exposed in Swift, it should be set up in such a way that it can actually be marked as async and use Swift's built in facilities for asynchronous programming.

To handle this, frameworks have a mechanism for saying "expose this symbol to C clients, but not to Swift clients" so that the functionality can be wrapped and exposed in a more ergonomic Swift wrapper. That mechanism looks like this:

typedef struct WGPUFooImpl* WGPUFoo NS_REFINED_FOR_SWIFT;
...
typedef enum WGPUBar {
    WGPUBar_Undefined = 0x00000000
} WGPUBar NS_REFINED_FOR_SWIFT;
...
WGPU_EXPORT void wgpuFooBaz() NS_REFINED_FOR_SWIFT;

However, the declarations in WebGPU.h are just:

typedef struct WGPUFooImpl* WGPUFoo;
...
typedef enum WGPUBar {
    WGPUBar_Undefined = 0x00000000
} WGPUBar;
...
WGPU_EXPORT void wgpuFooBaz();

There is no way for us to stick the NS_REFINED_FOR_SWIFT attribute on these functions/types.

It would be useful if WebGPU.h was modified, to do something like:

#if !defined(WGPU_TYPE_ATTRIBUTE)
#define WGPU_TYPE_ATTRIBUTE
#endif

#if !defined(WGPU_ENUM_ATTRIBUTE)
#define WGPU_ENUM_ATTRIBUTE
#endif

#if !defined(WGPU_FUNCTION_ATTRIBUTE)
#define WGPU_FUNCTION_ATTRIBUTE
#endif
...
typedef struct WGPUFooImpl* WGPUFoo WGPU_TYPE_ATTRIBUTE;
...
typedef enum WGPUBar {
    WGPUBar_Undefined = 0x00000000
} WGPUBar WGPU_ENUM_ATTRIBUTE;
...
WGPU_EXPORT void wgpuFooBaz() WGPU_FUNCTION_ATTRIBUTE;

That way, we could pass the necessary -D flag to our compiler to specify this swift refinement stuff for all the functions / types.

@litherum
Copy link
Author

This is blocking my PR at WebKit/WebKit#13849

@litherum litherum changed the title No way to refine types/functions for Swift No way to refine types/enums/functions for Swift May 13, 2023
@kainino0x
Copy link
Collaborator

@litherum Can you explain a bit more why this is needed? I don't know how this Swift mechanism works, maybe you can point to some documentation. If you're marking everything in webgpu.h as NS_REFINED_FOR_SWIFT, why can't you just not expose webgpu.h through the Swift bindings at all? For example in some hypothetical C++ bindings (this is different from Dawn's C++ bindings):

  • webgpu_cpp.h declares a C++ API, doesn't include webgpu.h
  • webgpu_cpp.cpp implements that C++ API, does include webgpu.h

This seems like a much more standard way to writing bindings over a C API, though to be fair only a few languages interoperate with C the way C++ and Objective-C/++ and I guess Swift do.

@litherum
Copy link
Author

litherum commented May 16, 2023

The docs are at https://developer.apple.com/documentation/swift/improving-objective-c-api-declarations-for-swift

The goal is:

  • It should be possible to make a single framework, that
  • has a C API / headers, visible to C clients, and
  • has a Swift API, visible to Swift clients, but
  • Swift clients shouldn't see the C API (and C clients shouldn't see the Swift API, ofc)

By default, a framework's C API is automatically "bridged" into Swift. So, if we do nothing, WebGPU.framework will expose a Swift function that's something like (please forgive the errors, written off the top of my head):

wgpuBufferMapAsync(buffer: WGPUBuffer, mode: WGPUMapModeFlags, offset: Int, size: Int, callback: @C_ABI ((status: WGPUBufferMapAsyncStatus, userdata: UnsafePointer<Void>) -> Void), userdata: UnsafePointer<Void>);

See all that junk at the end? That's just there because the C API doesn't translate well into Swift. In Swift, you would never write that by hand; instead, you'd write something like:

wgpuBufferMapAsync(buffer: WGPUBuffer, mode: WGPUMapModeFlags, offset: Int, size: Int) async -> WGPUBufferMapAsyncStatus

This is more expressive than C is capable of being. So, the only way to expose this kind of symbol from the framework is to actually write a Swift class/method that exposes this function signature. (The Swift class would just call the C function internally.)

But, now you've exposed 2 symbols which do the same thing!!

From a C client's perspective, there is no problem, since they can't see the Swift API in the first place. So they only see the C API, and they can call it no problem.

But, from a Swift client's perspective, both functions would exist, since the framework's C API automatically gets bridged.

NS_REFINED_FOR_SWIFT is the solution for this. Putting this on the C API tells the Swift importer "don't bridge this symbol." It doesn't have any effect on C clients; those still just call the C API like they always have. But, from a Swift client's perspective, if the C API is all marked as NS_REFINED_FOR_SWIFT, then they only see the nicer Swift API. This is the standard way to do this.

(Aside: The auto-bridging of C frameworks into Swift is a good thing in the general case; this is so that pure-C frameworks are all immediately accessible from Swift code, without the author of the framework having to do anything. We're only hitting the problem here because the author of WebGPU.framework (us) are trying to do better, and expose a more custom API that's more Swifty. An alternative is to make a second framework, WebGPUSwift.framework, and have that link WebGPU.framework, but it's generally an anti-pattern to do this, specifically because NS_REFINED_FOR_SWIFT exists.)

@litherum
Copy link
Author

Note that Swift doesn't have headers, so we can't just tell clients "please include the appropriate header for your language." Instead, Swift is based on modules, where you import a module as a whole, rather than textual includes.

Kangz added a commit to Kangz/webgpu-headers that referenced this issue May 16, 2023
@kainino0x
Copy link
Collaborator

kainino0x commented May 16, 2023

An alternative is to make a second framework, WebGPUSwift.framework, and have that link WebGPU.framework, but it's generally an anti-pattern to do this, specifically because NS_REFINED_FOR_SWIFT exists.

I disagree with the characterization as an anti-pattern in this case. This is not Apple software, it's an open source header. We can't have every new bindings layer requiring changes to the entire header, even if they're non-breaking. I absolutely see the reasoning you would want to expose both a C API and a Swift API from the same framework, while not exposing both at the same time, but I just don't think it's appropriate to impose Swift's peculiar, single-platform-centric design choices, at least on the public version of this header.

@JarWarren
Copy link

Unless I'm misunderstanding, all that's needed/asked for is an overridable attribute as seen in #182

@kainino0x
Copy link
Collaborator

kainino0x commented May 16, 2023

Yes, but it's still a 450 line patch. I would have no problem with it if it were generally useful in some way (like nullability attributes or C block support) or necessary (like the function pointer/proc table stuff). But it's an awful lot of overhead for one language binding when the language binding could just solve the problem in a less intrusive way.

@litherum
Copy link
Author

I disagree with the characterization as an anti-pattern in this case.

It's definitely an anti-pattern in the Swift world. I expect the reasoning for this is that, at the top of you Swift file, you want to have something like:

import Foo
import Bar
import Baz

and not

import Foo_Swift
import Bar_Swift
import Baz_Swift

it's an awful lot of overhead for one language binding

I dunno. I'm not a maintainer of this project, but it seems to me that if someone came along and said "adding some attribute allows for better interop with Rust" or "adding some attribute allows for better interop with C#" then those seem like totally legit use cases to me.

like nullability attributes or C block support

Would very much be interested in seeing these things added <3

@kainino0x
Copy link
Collaborator

kainino0x commented May 18, 2023

it's an awful lot of overhead for one language binding

I dunno. I'm not a maintainer of this project, but it seems to me that if someone came along and said "adding some attribute allows for better interop with Rust" or "adding some attribute allows for better interop with C#" then those seem like totally legit use cases to me.

I just have a hard time seeing this actually happen. Rust and most other languages in the world just deal with the fact that C is C without adding stuff to it. Swift seems fairly unique in this regard.

Rereading the documentation you linked, I realized NS_REFINED_FOR_SWIFT is an Objective-C thing. Of course, doing things in this header to make it work better in Objective-C would be fine, but you don't actually need to do that because Objective-C (like C++) (mostly) just deals with C being C.

I'm just very surprised if there isn't any better solution that you could apply in the case of an upstream project where you didn't have the opportunity to add these. Like, an option just say "here's a list of symbols, don't expose any of them to Swift"?

Anyway, I'm not the arbiter of what goes into webgpu.h. It's not really that big of a deal, we can add these if other maintainers are fine with it (seems @Kangz implicitly is).

@litherum
Copy link
Author

litherum commented May 19, 2023

You’re right that ideally there would be a way to make this modal. The way to make it modal would be to do this:

_Pragma("clang attribute push(__attribute__((swift_private)))")
...
_Pragma("clang attribute pop")

However, that doesn’t actually work, because the compiler reports:

Attribute 'swift_private' is not supported by '#pragma clang attribute'

I filed rdar://109555754 to request such a thing. Until then, the only way to do this appears to be to use Corentin’s patch.

@kainino0x
Copy link
Collaborator

OK, let's just go with it then. Thank you for looking into the alternatives, I appreciate it!

Kangz added a commit to Kangz/webgpu-headers that referenced this issue May 22, 2023
 - Also adds WGPU_NULLABLE instead of the /* nullable */ comments
 - Also adds WGPUFeatureName_Float32Filterable
 - Also adds struct forward declarations and move funtion pointers
   typedefs before the struct definitions, so that struct can contain
   function pointers.

Fixes webgpu-native#179
Kangz added a commit to Kangz/webgpu-headers that referenced this issue May 22, 2023
 - Also adds WGPU_NULLABLE instead of the /* nullable */ comments
 - Also adds WGPUFeatureName_Float32Filterable
 - Also adds struct forward declarations and move funtion pointers
   typedefs before the struct definitions, so that struct can contain
   function pointers.

Fixes webgpu-native#179
Fixes webgpu-native#119
kainino0x pushed a commit to Kangz/webgpu-headers that referenced this issue May 24, 2023
 - Also adds WGPU_NULLABLE instead of the /* nullable */ comments
 - Also adds WGPUFeatureName_Float32Filterable
 - Also adds struct forward declarations and move funtion pointers
   typedefs before the struct definitions, so that struct can contain
   function pointers.

Fixes webgpu-native#179
Fixes webgpu-native#119
blueboxd pushed a commit to blueboxd/dawn that referenced this issue May 27, 2023
See webgpu-native/webgpu-headers#179

Also changes WGPU_NULLABLE instead of /* nullable */ and make a couple
formatting fixes.

Bug: None
Change-Id: Ieb4315cddd3c806144892221fba11888171f072f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/133102
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
@litherum
Copy link
Author

Thank you very much!

@kainino0x
Copy link
Collaborator

@litherum Does

typedef enum WGPUBar {
    WGPUBar_Undefined = 0x00000000
} WGPUBar NS_REFINED_FOR_SWIFT;

Need to actually be

typedef enum WGPUBar {
    WGPUBar_Undefined = 0x00000000
} NS_REFINED_FOR_SWIFT WGPUBar NS_REFINED_FOR_SWIFT;

to avoid exporting the type enum WGPUBar in addition to WGPUBar?

@kainino0x
Copy link
Collaborator

And similarly

typedef struct WGPUQux {
} NS_REFINED_FOR_SWIFT WGPUQux NS_REFINED_FOR_SWIFT;

And are the implicit forward declarations like struct WGPUAdapterImpl in typedef struct WGPUAdapterImpl* WGPUAdapter WGPU_OBJECT_ATTRIBUTE; OK since they're not definitions?

@kainino0x kainino0x reopened this Jun 23, 2023
@kainino0x kainino0x added the non-breaking Does not require a breaking change (that would block V1.0) label Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking Does not require a breaking change (that would block V1.0)
Projects
None yet
Development

No branches or pull requests

4 participants