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

Make data-carrying enums emit #[derive(Debug)] on the Rust side #209

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

amsam0
Copy link
Contributor

@amsam0 amsam0 commented Apr 7, 2023

  • fix: Add Debug to derived traits on Rust side even if one of the variants has data to hopefully avoid confusing the developer
  • fix: don't generate Debug functions in C header and Swift if any variants have data to avoid confusing the developer with undefined symbol build errors

Previously, this would cause a "undefined symbol" build error and cause the error not to implement Debug trait from Rust:

#[swift_bridge::bridge]
mod ffi {
    #[derive(Debug)]
    enum DeriveDebugEnum {
        Variant(String),
    }
}

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.

@chinedufn
Copy link
Owner

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!

@amsam0
Copy link
Contributor Author

amsam0 commented Apr 8, 2023

Added!

@amsam0
Copy link
Contributor Author

amsam0 commented Oct 7, 2023

@chinedufn Sorry to bother you, but any chance of this being merged?

@NiwakaDev
Copy link
Collaborator

Can we add some tests?

@amsam0
Copy link
Contributor Author

amsam0 commented Oct 9, 2023

What tests need to be added?

@junepark678
Copy link

Is there any progress on this? This change is quite a bit critical to one of our projects.

@amsam0
Copy link
Contributor Author

amsam0 commented Aug 12, 2024

@junepark678 You could add my branch as a dependency to use it early. I'll sync up the branch

Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

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

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).

#194 (comment)

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:

/// Verify that we generate debugDescription in Swift and Debug function in Rust when using #\[derive(Debug)]
mod derive_debug_enum {
use super::*;
fn bridge_module_tokens() -> TokenStream {
quote! {
#[swift_bridge::bridge]
mod ffi {
#[derive(Debug)]
enum SomeEnum {
Variant1
}
}
}
}
fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::ContainsMany(vec![
quote! {
#[derive(Copy, Clone, ::std::fmt::Debug)]
pub enum SomeEnum {
Variant1
}
},
quote! {
#[export_name = "__swift_bridge__$SomeEnum$Debug"]
pub extern "C" fn __swift_bridge__SomeEnum_Debug(this: __swift_bridge__SomeEnum) -> *mut swift_bridge::string::RustString {
swift_bridge::string::RustString(format!("{:?}", this.into_rust_repr())).box_into_raw()
}
},
])
}
fn expected_swift_code() -> ExpectedSwiftCode {
ExpectedSwiftCode::ContainsAfterTrim(
r#"
extension SomeEnum: CustomDebugStringConvertible {
public var debugDescription: String {
RustString(ptr: __swift_bridge__$SomeEnum$Debug(self.intoFfiRepr())).toString()
}
}
"#,
)
}
fn expected_c_header() -> ExpectedCHeader {
ExpectedCHeader::ContainsAfterTrim(
r#"
void* __swift_bridge__$SomeEnum$Debug(__swift_bridge__$SomeEnum this);
"#,
)
}
#[test]
fn generates_enum_to_and_from_ffi_conversions_no_data() {
CodegenTest {
bridge_module: bridge_module_tokens().into(),
expected_rust_tokens: expected_rust_tokens(),
expected_swift_code: expected_swift_code(),
expected_c_header: expected_c_header(),
}
.test();
}
}

Can use these to confirm that the Swift/C code does not contain any generated Debug code

DoesNotContainAfterTrim(&'static str),
DoesNotContainManyAfterTrim(Vec<&'static str>),


Add an integration test.

Can add an data-carrying enum here:

#[swift_bridge::bridge]
mod ffi {
#[derive(Debug)]
enum DeriveDebugEnum {
Variant,
}
}

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>();
}

Comment on lines +144 to +145
// 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.
Copy link
Owner

Choose a reason for hiding this comment

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

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.

@chinedufn chinedufn changed the title derive(Debug) enum fixes Make data-carrying enums emit #[derive(Debug)] on the Rust side Aug 15, 2024
@amsam0
Copy link
Contributor Author

amsam0 commented Aug 15, 2024

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 ld or the Swift compiler saying it can't find a symbol that some swift code needed.

@NiwakaDev
Copy link
Collaborator

@chinedufn

#209 (comment)

May I take over this PR if no one addresses this by next week?

@chinedufn
Copy link
Owner

Absolutely, feel free. Thanks.

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