Make data-carrying enums emit #[derive(Debug)] on the Rust side#209
Make data-carrying enums emit #[derive(Debug)] on the Rust side#209amsam0 wants to merge 4 commits intochinedufn:masterfrom
#[derive(Debug)] on the Rust side#209Conversation
|
Hey, thanks for working on this. Mind updating your PR to more clearly explain the problem and how this PR solves the problem, as well as to include an example bridge module that illustrates what this PR enables? Thanks! |
|
Added! |
|
@chinedufn Sorry to bother you, but any chance of this being merged? |
|
Can we add some tests? |
|
What tests need to be added? |
|
Is there any progress on this? This change is quite a bit critical to one of our projects. |
|
@junepark678 You could add my branch as a dependency to use it early. I'll sync up the branch |
chinedufn
left a comment
There was a problem hiding this comment.
Here's how we can land this PR.
Update the PR body with an example of the "undefined symbol" error.
Add a #[derive(Debug)] section to the book that mentions that we currently only allow Swift to call the Rust Debug impl if the enum
is non-data-carrying, because we do not currently have a way to pass enums by reference (non-data-carrying enums are copied).
Can add the docs somewhere in here
Add a codegen test that covers the code that you've introduced.
Your codegen test can test that we do not emit C and Swift code for a data-carrying enum.
Can use this codegen test as inspiration:
Can use these to confirm that the Swift/C code does not contain any generated Debug code
swift-bridge/crates/swift-bridge-ir/src/codegen/codegen_tests.rs
Lines 107 to 108 in 636fa27
Add an integration test.
Can add an data-carrying enum here:
Something like:
#[swift_bridge::bridge]
mod ffi {
#[derive(Debug)]
enum DeriveDebugEnum {
Variant,
}
#[derive(Debug)]
enum DeriveDebugEnumDataCarrying {
DataCarryingVariant(String),
}
}
fn assert_implements_debug<T: Debug>() {}
/// Verify that the FFI enums implement `Debug`.
/// This confirms that `swift-bridge` is emitting the `#[derive(Debug)]` attribute on the final enum.
fn _test_implements_debug() {
assert_implements_debug::<ffi::DeriveDebugEnum>();
assert_implements_debug::<ffi::DeriveDebugEnumDataCarrying>();
}| // We don't want to confuse the developer if one of our variants has data and Debug isn't derived, | ||
| // so we still want to derive(Debug) on the Rust side. |
There was a problem hiding this comment.
You can remove this first part of the comment.
We don't need to explain why we always emit the Debug impl on the Rust side. This is desirable behavior.
Can keep the TODO. It's worth considering.
#[derive(Debug)] on the Rust side
|
Is there anyone interested in picking up this PR? I am not actively working on anything that depends on swift-bridge and finishing this up seems like a lot for something that was intended to be a quick fix for a bug. As for the error message, it's just |
|
May I take over this PR if no one addresses this by next week? |
|
Absolutely, feel free. Thanks. |
Previously, this would cause a "undefined symbol" build error and cause the error not to implement
Debugtrait from Rust:When generating C headers and Swift, there was no check to ensure that the enum has no variants with data, but this check was added when generated Rust tokens. This causes undefined symbol if a variant has data since C and Swift thinks the method exists, but it actually doesn't.
This PR fixes the undefined symbol error and also makes the enum implement
Debug, even if only from Rust.