Supporting methods

[lattice0]

ULTRA draft, just a request for how to go in this. Also I don't promise it will be done, I just want to see how hard would it be.

@fzyzcjy please look at frb_codegen/src/parser/mod.rs

here I parse methods. What do you think of transforming a method to a function which receives a reference of the Self type?

For example

pub struct StructWithMethod{
    something: String
}

impl StructWithMethod {
    pub fn do_something(&self, _u: u32) {
        todo!()
    }
}

would turn into

pub fn struct_with_method_do_something(self_object: &StructWithMethod, _u: u32) {
//...
}

if done like this, then I'd do something like this

if let Item::Impl(ref item_impl) = item {
            for item in &item_impl.items {
                if let ImplItem::Method(item_method) = item {
                    if let Visibility::Public(_) = &item_method.vis {
                        let item_fn = modify_method_to_function(item_method);
                        src_fns.push(item_fn);
                    }
                }
            }
        }

where modify_method_to_function would just create a function out of the method.

Close #539 Close #67

[fzyzcjy]

What do you think of transforming a method to a function which receives a reference of the Self type?

Sure. By doing that, we can reuse most of the existing code.

[lattice0]

Is it okay to change

fn extract_fns_from_file(file: &File) -> Vec<&ItemFn> {

to

fn extract_fns_from_file(file: &File) -> Vec<ItemFn> {

Cause we're going to have to create some ItemFn inside this function

[lattice0]

Please don't mind approving workflows for now. Sorry, gonna publish my sketches on another branch

[lattice0]

The only way I found to transform a method implementation into a function in a way that we know later that it should be a method, is by marking the function name as ending with is_a_method: https://github.com/fzyzcjy/flutter_rust_bridge/blob/e7cbec7dd980ab638d337a71cdd58d6532f604bc/frb_codegen/src/parser/mod.rs#L236

What do you think of this approach? I prefer to ask first then proceed with the code. Currently it generated the rust part correctly. The other option would be to reengineer a lot of stuff, and the goal is to reuse the code, transforming a method into a function, and later transforming this to a method call.

This:

pub struct StructWithMethod{
    pub something: String
}

impl StructWithMethod {
    pub fn do_something(&self, _u: u32, _x: String) {
        todo!()
    }
}

turned into

#[no_mangle]
pub extern "C" fn wire_StructWithMethod_do_something_is_a_method(
    port_: i64,
    StructWithMethod_is_a_method: *mut wire_StructWithMethod,
    _u: u32,
    _x: *mut wire_uint_8_list,
) {
    FLUTTER_RUST_BRIDGE_HANDLER.wrap(
        WrapInfo {
            debug_name: "StructWithMethod_do_something_is_a_method",
            port: Some(port_),
            mode: FfiCallMode::Normal,
        },
        move || {
            let api_StructWithMethod_is_a_method = StructWithMethod_is_a_method.wire2api();
            let api__u = _u.wire2api();
            let api__x = _x.wire2api();
            move |task_callback| {
                Ok(StructWithMethod::do_something(
                    &api_StructWithMethod_is_a_method,
                    api__u,
                    api__x,
                ))
            }
        },
    )
}

of course there's the dart part as well and mutability or not, etc, and testing for different types, args, etc

[fzyzcjy]

Is it okay to change fn extract_fns_from_file(file: &File) -> Vec<&ItemFn> { to fn extract_fns_from_file(file: &File) -> Vec {

Sure. If you care about performance, maybe Cow<ItemFn> is better. But since codegen is not very slow currently you may just use ItemFn

[fzyzcjy]

Please don't mind approving workflows for now. Sorry, gonna publish my sketches on another branch

542 is merged and now I do not need to approve :)

[fzyzcjy]

is by marking the function name as ending with is_a_method:

Generally looks ok to me.

Nit: What about ending with __method. Because (1) it is shorter (2) use double underscore to avoid conflict with normal functions, just like how Python and c++ uses double underscore

[fzyzcjy]

of course there's the dart part as well and mutability or not, etc, and testing for different types, args, etc

Yes, we should test for these details - great to see you have good testing in mind

[fzyzcjy]

Btw, the main high-level idea is: Just transform a method into a function (and vise versa), and let the original flutter_rust_bridge to the heavy lifting.

So yes, should not have to rewrite too much code otherwise it is the wrong direction :)

[lattice0]

The problem I had was that I transformed the &self into struct_with_method: &StructWithMethod, but then still there's no way to know that this is a method, so marking the name with struct_with_method__method solves this.

There's still the problem of not knowing if it's a mutable self or not. I could do __method_mutable versus __method for non mutable, or create a https://github.com/lattice0/flutter_rust_bridge/blob/e7cbec7dd980ab638d337a71cdd58d6532f604bc/frb_codegen/src/ir/ty.rs#L14 but StructMut. What do you think?

[fzyzcjy]

Wait a minute - can it really be mutable? In other words, if the data is mutated, will we really get data mutated as well on the Dart side? I guess not currently.

That would be an interesting feature and feel free to separately PR for it :)

[lattice0]

You're right. I was thinking about opaque types but even for them it might be unsafe to do so from various isolates, so I should support only non mutable?

[fzyzcjy]

I should support only non mutable?

Currently, I guess yes.

[lattice0]

I just remembered that https://github.com/mozilla/uniffi-rs only supports &self also, so yes this is the correct way. When you want to modify something you have to use interior mutability probably with a mutex

[lattice0]

There's no way to generate methods for a struct without a Vec<IrFunc> inside TypeDartGenerator at https://github.com/lattice0/flutter_rust_bridge/blob/89e5ac0f91664c642cf935f3feb4c4968479f2d8/frb_codegen/src/generator/dart/mod.rs#L110

can I make an option to set this vec like this

pub trait TypeDartGeneratorTrait {
    fn api2wire_body(&self) -> Option<String>;

    fn api_fill_to_wire_body(&self) -> Option<String> {
        None
    }

    fn wire2api_body(&self) -> String {
        "".to_string()
    }

    fn structs(&self) -> String {
        "".to_string()
    }

    fn set_vec_ir_func(&self, ir_func: Vec<IrFunc>)  {
       todo!()
    }
}

or maybe

pub trait TypeDartGeneratorTrait {
    fn api2wire_body(&self) -> Option<String>;

    fn api_fill_to_wire_body(&self) -> Option<String> {
        None
    }

    fn wire2api_body(&self) -> String {
        "".to_string()
    }

    fn structs(&self, ir_funcs: Vec<IrFunc>) -> String { //possibly even ir_funcs: &[IrFunc]
        "".to_string()
    }
}

[fzyzcjy]

The set_vec_ir_func sounds not like a good idea because it introduces mutability.

fn structs(&self, ir_funcs: Vec<IrFunc>) -> String { //possibly even ir_funcs: &[IrFunc] seems ok

But, maybe ir_funcs are already in ir_file? Thus, since https://github.com/lattice0/flutter_rust_bridge/blob/89e5ac0f91664c642cf935f3feb4c4968479f2d8/frb_codegen/src/generator/dart/mod.rs#L110 provides ir_file, we can already get ir_funcs from them

[lattice0]

pub struct StructWithMethod {
    pub something: String,
}
pub fn return_struct() -> StructWithMethod {
    todo!()
}

turns roughly into

class StructWithMethod {
  final String something;

  StructWithMethod({
    required this.something,
  });
}

class SomethingImpl extends FlutterRustBridgeBase<SomethingWire>
    implements Something {
  factory SomethingImpl(ffi.DynamicLibrary dylib) =>
      SomethingImpl.raw(SomethingWire(dylib));

  SomethingImpl.raw(SomethingWire inner) : super(inner);

  Future<StructWithMethod> returnStruct({dynamic hint}) =>
      executeNormal(FlutterRustBridgeTask(
        callFfi: (port_) => inner.wire_return_struct(port_),
        parseSuccessData: _wire2api_struct_with_method,
        constMeta: kReturnStructConstMeta,
        argValues: [],
        hint: hint,
      ));

as you see, class StructWithMethod has no way of calling ffi stuff like executeNormal. What do you think of class StructWithMethod extending FlutterRustBridgeBase<StructWithMethod> when the struct has a method?

[fzyzcjy]

What do you think of class StructWithMethod extending FlutterRustBridgeBase when the struct has a method?

What about this:

class StructWithMethod {
  final FlutterRustBridgeBase<SomethingWire> bridge;
  final String something;

  StructWithMethod({
    required this.bridge,
    required this.something,
  });

  Future<Something> someMethod(Something b) => bridge.someMethod(a: this, b: b);
}

[lattice0]

Ok. I guess StructWithMethod was never meant to be manually created in the dart side, right? It should be returned by return_struct

[fzyzcjy]

I guess StructWithMethod was never meant to be manually created in the dart side, right? It should be returned by return_struct

Maybe we should allow it to be created directly in Dart side.

An extreme example is struct without fields, which acts like "namespace" for apis.

[lattice0]

Ok I got

pub struct StructWithMethod {
    pub something: String,
}

impl StructWithMethod {
    pub fn do_something(&self, _u: u32, _x: String) {
        todo!()
    }
}

to compile into

class StructWithMethod {
  final SomethingImpl bridge;
  final String something;

  StructWithMethod({
    required this.bridge,
    required this.something,
  });
  Future<void> doSomething({required int u, required String x, dynamic hint}) =>
      bridge.doSomethingMethod(
        structWithMethod: this,
        u: u,
        x: x,
      );
}

class SomethingImpl extends FlutterRustBridgeBase<SomethingWire>
    implements Something {
  factory SomethingImpl(ffi.DynamicLibrary dylib) =>
      SomethingImpl.raw(SomethingWire(dylib));

  SomethingImpl.raw(SomethingWire inner) : super(inner);

  Future<void> doSomethingMethod(
          {required StructWithMethod structWithMethod,
          required int u,
          required String x,
          dynamic hint}) =>
      executeNormal(FlutterRustBridgeTask(
        callFfi: (port_) => inner.wire_do_something__method(
            port_,
            _api2wire_box_autoadd_struct_with_method(structWithMethod),
            _api2wire_u32(u),
            _api2wire_String(x)),
        parseSuccessData: _wire2api_unit,
        constMeta: kDoSomethingMethodConstMeta,
        argValues: [structWithMethod, u, x],
        hint: hint,
      ));
      //...

it only fails here:

StructWithMethod _wire2api_struct_with_method(dynamic raw) {
  final arr = raw as List<dynamic>;
  if (arr.length != 1)
    throw Exception('unexpected arr length: expect 1 but see ${arr.length}');
  return StructWithMethod(
    /// because it's missing the bridge here
    something: _wire2api_String(arr[0]),
  );
}

now I don't know if I should pass the bridge. I'm confused now

[fzyzcjy]

now I don't know if I should pass the bridge. I'm confused now

pass this :)

StructWithMethod(bridge: this, ......)

[lattice0]

but

StructWithMethod _wire2api_struct_with_method(dynamic raw) {
  final arr = raw as List<dynamic>;
  if (arr.length != 1)
    throw Exception('unexpected arr length: expect 1 but see ${arr.length}');
  return StructWithMethod(
    something: _wire2api_String(arr[0]),
  );
}

is a function, there's no this for a bridge class. It's not inside anything.

[fzyzcjy]

You are right. Then what about

_wire2api_struct_with_method(FlutterRustBridgeBase<SomethingWire> bridge, ...)

and

modify parseSuccessData such that pass the bridge into it

[fzyzcjy]

Btw I was wrong:

class StructWithMethod {
  final SomethingImpl bridge;

should be

class StructWithMethod {
  final FlutterRustBridgeBase<SomethingWire> bridge;

b/c we should not hardcode implementation class there

[lattice0]

I tried with

class StructWithMethod {
  final FlutterRustBridgeBase<SomethingWire> bridge;

but FlutterRustBridgeBase has no methods.

The thing that actually has methods is the implementation/extension of FlutterRustBridge

class SomethingImpl extends FlutterRustBridgeBase<SomethingWire>
    implements Something {
  factory SomethingImpl(ffi.DynamicLibrary dylib) =>
      SomethingImpl.raw(SomethingWire(dylib));

  SomethingImpl.raw(SomethingWire inner) : super(inner);

  Future<void> doSomethingMethod(
          {required StructWithMethod structWithMethod,
          required int u,
          required String x,
          dynamic hint}) =>
      executeNormal(FlutterRustBridgeTask(
        callFfi: (port_) => inner.wire_do_something__method(
            port_,
            _api2wire_box_autoadd_struct_with_method(structWithMethod),
            _api2wire_u32(u),
            _api2wire_String(x)),
        parseSuccessData: _wire2api_unit,
        constMeta: kDoSomethingMethodConstMeta,
        argValues: [structWithMethod, u, x],
        hint: hint,
      ));

[fzyzcjy]

You are right. So we should use Something not SomethingImpl, since the former is like an abstract interface

[fzyzcjy]

Btw I am not sure why CI does not run (except for codacy). Maybe need to resolve conflict?

Anyway no worries about CI - only need to pass before merging

[lattice0]

I removed all the trash I added to make it easier for you to review. Everything built on my side, both the rust and dart generated files, no errors. Now I still have to write the dart tests but they should pass

[lattice0]

  test('ConcatenateWith test', () async {
    final ConcatenateWith concatenateWith = ConcatenateWith(a: "hello ", bridge: api);
    final String concatenated = await concatenateWith.concatenate(b: "world");
    expect(concatenated, equals("hello world"));
  });

works! But functions that are truly static (no &self) are a bit problematic, cause they will always need the bridge. I didn't change them to be static on the dart side yet though.

Here's an example

impl ConcatenateWith {
    pub fn new(a: String) -> ConcatenateWith {
        ConcatenateWith { a }
    }
    pub fn concatenate(&self, b: String) -> String {
        format!("{}{}", self.a, b)
    }
    pub fn concatenate_static(a: String, b: String) -> String {
        format!("{}{}", a, b)
    }
}

turns into

class ConcatenateWith {
  final FlutterRustBridgeExampleSingleBlockTest bridge;
  final String a;

  ConcatenateWith({
    required this.bridge,
    required this.a,
  });
  Future<ConcatenateWith> newConcatenateWith(
          {required String a, dynamic hint}) =>
      bridge.newStaticMethodConcatenateWith(
        a: a,
      );
  Future<String> concatenate({required String b, dynamic hint}) =>
      bridge.concatenateMethod(
        concatenateWith: this,
        b: b,
      );
  Future<String> concatenateStatic(
          {required String a, required String b, dynamic hint}) =>
      bridge.concatenateStaticStaticMethodConcatenateWith(
        a: a,
        b: b,
      );
}

Do you think they should be static on the flutter side? If so, they have to receive bridge.

[fzyzcjy]

Do you think they should be static on the flutter side? If so, they have to receive bridge.

Not sure why I did not receive notification about this question...

Yes, I guess they should be static. Otherwise, suppose ConcatenateWith struct has 100 fields, then it means, in order to run the static method we have to provide a struct with 100 fields even if it is unneeded!

Feel free to ping me if I do not reply for a long time

[fzyzcjy]

Oh I did not realized this is ready for review. I will review it soon.

Btw maybe need to resolve conflicts o/w ci will not run

[fzyzcjy]

Btw we need to add a bit of documentation later, probably be siblings of this chapter http://cjycode.com/flutter_rust_bridge/feature/zero_copy.html

[fzyzcjy]

Ping me when you have finished everything (e.g. code change, or comments), and I will look at all comments and reply all :)

[lattice0]

@fzyzcjy done!

[fzyzcjy]

Again ping me when done! :)

[lattice0]

@fzyzcjy done. On some I couldn't comment, but now MethodNamingUtil is not used directly anymore, everything goes through FunctionName, except for struct_has_method.

[fzyzcjy]

I am reviewing it.

Btw some comments are folded by github and need to click load more button. e.g. item_method_to_function-problem https://github.com/fzyzcjy/flutter_rust_bridge/pull/543#discussion_r919507081, https://github.com/fzyzcjy/flutter_rust_bridge/pull/543#discussion_r918420499 , etc

[fzyzcjy]

Good job! Now there is almost no new review issues (only some old ones that may have been overlooked because of github's notification mechanism) and it is very close to merging.

[fzyzcjy]

Ping me when things are done :)

[fzyzcjy]

Great job!

[fzyzcjy]

@all-contributors please add @lattice0 for code, doc

[allcontributors[bot]]

@fzyzcjy

I've put up a pull request to add @lattice0! :tada: