fzyzcjy / flutter_rust_bridge

Flutter/Dart <-> Rust binding generator, feature-rich, but seamless and simple.
https://fzyzcjy.github.io/flutter_rust_bridge/
MIT License
4.27k stars 300 forks source link

Use From and Into for External Types #1736

Closed furkan-guvenc closed 6 months ago

furkan-guvenc commented 9 months ago

Hi, thanks for the great library, it is really amazing. My issue is I am trying to avoid duplications of some functions.

For example, I have these functions to add and remove nodes.

pub fn add_node(label: String) -> NodeId;
pub fn remove_node(node_id: NodeId);

NodeId is type of KeyData from slotmap library which is defined as

pub struct KeyData {
    idx: u32,
    version: NonZeroU32,
}

impl KeyData {
  pub fn as_ffi(self) -> u64;
  pub fn from_ffi(value: 64) -> Self;
}

Currently I created another NodeId named NodeApiId for public api and implemented From and Into traits for convenience but still I have to write add_node and remove_node twice.

// core.rs
pub fn add_node(label: String) -> NodeId;
// api.rs
use crate::core::{NodeId};

pub fn add_node(label: String) -> NodeApiId{
    crate::core::add_node(label).into()
}

pub struct NodeApiId(pub u64);

impl From<NodeApiId> for NodeId {
    fn from(value: NodeApiId) -> Self {
        KeyData::from_ffi(value.0).into()
    }
}

impl From<NodeId> for NodeApiId {
    fn from(value: NodeId) -> Self {
        NodeApiId(value.data().as_ffi())
    }
}

My solution is adding a new macro like mirror

#[frb(convert(NodeId))]
pub struct NodeApiId(pub u64);

And for all generated functions that use NodeId, NodeApiId should be used with from and into.

welcome[bot] commented 9 months ago

Hi! Thanks for opening your first issue here! :smile:

fzyzcjy commented 9 months ago

Hi, you are welcome!

Firstly, I wonder whether the mirroring https://cjycode.com/flutter_rust_bridge/guides/types/translatable/external/diff-crate works for you?

furkan-guvenc commented 9 months ago

I tried that but I am not sure I correctly mirrored std::num::NonZeroU32

#[frb(mirror(NonZeroU32))]
pub struct _NonZeroU32(u32);

#[frb(mirror(InputId, OutputId))]
pub struct Id {
    idx: u32,
    version: NonZeroU32,
}

But I got lots of errors from build-web. The first and I think the most important one is

error[E0423]: cannot initialize a tuple struct which contains private fields
    --> rust/src/frb_generated.rs:183:16
     |
183  |         return crate::public_api::app::NonZeroU32(var_field0);
     |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    ::: rust/src/public_api/app.rs:13:1
     |
13   | #[frb(mirror(NonZeroU32))]
     | -------------------------- similarly named tuple struct `_NonZeroU32` defined here
     |
note: constructor is not visible here due to private fields
    --> /Users/furkanguvenc/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/num/nonzero.rs:1347:1
     |
1347 | / nonzero_integer! {
1348 | |     Self = NonZeroU32,
1349 | |     Primitive = unsigned u32,
1350 | | }
     | |_^ private field
     = note: this error originates in the macro `nonzero_integer` (in Nightly builds, run with -Z macro-backtrace for more info)
help: you might have meant to use the `new_unchecked` associated function
     |
183  |         return crate::public_api::app::NonZeroU32::new_unchecked(var_field0);
     |                                                  +++++++++++++++
help: a tuple struct with a similar name exists, consider changing it
    -->  rust/src/public_api/app.rs:13:1
     |
13   | NonZeroU32
     |
fzyzcjy commented 9 months ago

cannot initialize a tuple struct which contains private fields

Hmm, firstly maybe try to add a pub:

#[frb(mirror(NonZeroU32))]
pub struct _NonZeroU32(pub u32);
furkan-guvenc commented 9 months ago

Making it pub didn't work, it gave the exact same error. I think it is just std::num::NonZeroU32 is defined in this way. It is a 1 field tuple but the field is private.

It is possible to use it with new_unchecked but I don't know any way to say this to code generator.

fzyzcjy commented 9 months ago

Oh I see: So it seems that, you are having a struct (NonZeroU32) which does not expose all fields as public.

Then you suggestion LGTM (at first glance)! I will need to think about it a bit more later (hopefully within a few days) since tired now.

Btw, an alternative solution: You can also treat KeyData as an auto opaque type. In other words, just do not mirror it, and flutter_rust_bridge will wrap it automatically.

furkan-guvenc commented 9 months ago

You can also treat KeyData as an auto opaque type.

Makes sense, thank you.

fzyzcjy commented 9 months ago

You are welcome! Also feel free to PR if you like the convert feature (I may not have time to implement this by myself since it is a new feature that is mostly covered by existing features)

Btw, as for API, another possible way (parallel to the From/Into) for brainstorming is something like serde's custom serializer function. In other words, you annotate with something like convert = my_converter_function, with fn my_converter_function(a: NodeId) -> NodeApiId { ... } as well as the other direction. This may be slightly more flexible but more verbose though.

furkan-guvenc commented 9 months ago

Serde uses serde(serialize_with = "path"), serde(deserialize_with = "path") or serde(with = "module"). As I understand with this way you can use serde's serialization with a custom deserialization but I don't think this is possible for this library. Either a type is serializable and deserializable or it needs to be mirrored/converted. So only 1 attribute should be enough.

In my case I use a custom key but if I would use default key(KeyData) then it is directly convertible with u64. The issue is how we can say this to generator.

Here I am not sure how we will know it is between KeyData and u64:

#[frb(convert(from=slotmap::KeyData::from_ffi, into=slotmap::KeyData::as_ffi))]
pub struct _Id;

We can know now but it seems an ugly hack:

#[frb(convert(from=slotmap::KeyData::from_ffi, into=slotmap::KeyData::as_ffi))]
pub struct _KeyData(u64);

This is a duplication:

fn from_key_data(a: KeyData) -> u64 { ... }
fn into_key_data(a: u64) -> KeyData { ... }

#[frb(convert(from=from_key_data, into=into_key_data))]
pub struct _Id;

Intead of attribute we can use a comment maybe // !frb(convert(from=from_key_data, into=into_key_data))

Or we can have our own trait like standart libray's From and Into. Otherwise it will be an orphan issue:

impl From<KeyData> for u64 {
    fn from(value: KeyData) -> Self {
        todo!()
    }
}
furkan-guvenc commented 9 months ago

I can write the PR but I want to be sure it will be a good API.

fzyzcjy commented 9 months ago

Good point about the orphan issue for the std From/Into! So for summary, there seems like at least two categories of approaches:

Category 1: Function-based

Just like #[frb(convert(from=slotmap::KeyData::from_ffi, into=slotmap::KeyData::as_ffi))] or #[frb(convert(from=from_key_data, into=into_key_data))]

The former may be hard to know the ffi types, while the latter is OK.

Category 2: Trait-based

Write sth like this trait definition in user's frb_generated.rs (by using the boilerplate macro):

pub trait Serializer<T> {
  type Target; // e.g. u64 for your example
  fn serialize(self) -> Target;
  fn deserialize(target: Target) -> T;
}

Then we can generate some_object.serialize() and TheTypeOfYourObject::deserialize(..) freely.

This looks more encapsulated, but the boilerplate may be slightly longer.

furkan-guvenc commented 9 months ago

Can you explain boilerplate macro part, where it will be put and how the signature will be like? I was thinking that user can write the trait and we can have a static macro on top of the trait

fzyzcjy commented 9 months ago

Just write something near https://github.com/fzyzcjy/flutter_rust_bridge/blob/ed163209ffefff4b4c524ca17557bcc3c43f92b6/frb_rust/src/for_generated/boilerplate.rs#L5, and it will automatically be inside frb_geneated.rs.

stale[bot] commented 7 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 6 months ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new issue.