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

Ensure bridging header function prototypes are valid #1512

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

ijc
Copy link
Contributor

@ijc ijc commented Apr 6, 2023

Compiling my swift project I am seeing:

Generated/mycrateFFI.h:75:45: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]

for the ffi_mycrate_uniffi_contract_version and
uniffi_mycrate_checksum_func_myfun declarations.

This is because they take no arguments which is emitted as (), for C (void) is required, C++ is happy with either.

Add a specific case to swift::arg_list_ffi_decl to handle this. The resulting diff is:

--- mycrateFFI.h	2023-04-06 10:38:25.645130502 +0100
+++ mycrateFFI.h	2023-04-06 11:01:12.415958817 +0100
@@ -73,9 +73,11 @@ RustBuffer ffi_mycrate_rustbuffer_reserv
 );

 uint32_t ffi_mycrate_uniffi_contract_version(
+    void

 );

 uint16_t uniffi_mycrate_checksum_func_myfun(
+    void

 );

@ijc ijc requested a review from a team as a code owner April 6, 2023 10:54
@ijc ijc requested review from badboy and removed request for a team April 6, 2023 10:54
@ijc
Copy link
Contributor Author

ijc commented Apr 6, 2023

I think these functions were newly added in #1469 (/cc @bendk) and uniffi didn't have any FfiFunction's which had an empty argument list before that point which is why this went unnoticed until now.

@ijc
Copy link
Contributor Author

ijc commented Apr 6, 2023

Compiling my swift project I am seeing:

Actually, that's inaccurate: it's a hybrid swift + objc project and i think the prototype error was coming from building a .m file which was including the bridging header and therefore the FFI.h.

@mhammond
Copy link
Member

mhammond commented Apr 6, 2023

Any chance you can demonstrate the problem in one of our fixtures?

@ijc
Copy link
Contributor Author

ijc commented Apr 6, 2023

Yes. I'm trying to figure that out now, there don't seem to be any existing fixtures which demonstrate running a C/obj-c compiler over the FFI.h.

Compiling my swift project I am seeing:

```
Generated/mycrateFFI.h:75:45: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
```

for the `ffi_mycrate_uniffi_contract_version` and
`uniffi_mycrate_checksum_func_myfun` declarations.

This is because they take no arguments which is emitted as `()`, for C `(void)`
is required, C++ is happy with either.

The included fixture attempts to validate that the generated header is valid
according to a compiler at a highly pendantic level, interestingly this doesn't
actually cover `-Wstrict-prototypes` so that is enabled separately.

Add a specific case to `swift::arg_list_ffi_decl` to handle this by adding the
`void` when there are no arguments. The resulting diff is:

```diff
--- swift_bridging_header_compileFFI.h.old	2023-04-06 14:30:29.994435300 +0000
+++ swift_bridging_header_compileFFI.h.new	2023-04-06 14:31:14.047300820 +0000
@@ -54,6 +54,7 @@

 void uniffi_swift_bridging_header_compile_fn_func_no_arguments(
     RustCallStatus *_Nonnull out_status
+
 );

 RustBuffer ffi_swift_bridging_header_compile_rustbuffer_alloc(
@@ -73,9 +74,11 @@
 );

 uint32_t ffi_swift_bridging_header_compile_uniffi_contract_version(
+    void

 );

 uint16_t uniffi_swift_bridging_header_compile_checksum_func_no_arguments(
+    void

 );
```
@ijc ijc force-pushed the swift-function-decl-is-not-a-prototype branch from b607131 to 12995eb Compare April 6, 2023 14:33
@ijc
Copy link
Contributor Author

ijc commented Apr 6, 2023

Any chance you can demonstrate the problem in one of our fixtures?

Done.

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, C is so weird. This seems correct to me based on https://en.wikipedia.org/wiki/Compatibility_of_C_and_C%2B%2B.

@badboy
Copy link
Member

badboy commented Apr 6, 2023

Wow, C is so weird. This seems correct to me based on en.wikipedia.org/wiki/Compatibility_of_C_and_C%2B%2B.

Yes. I can confirm void as a signal for "no argument" is correct and empty arguments technically allows any number of arguments. Bit annoying that we need the additional conditional in the template, but it is what it is.

@bendk bendk merged commit aa91307 into mozilla:main Apr 6, 2023
@ijc ijc deleted the swift-function-decl-is-not-a-prototype branch April 11, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants