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.3k stars 301 forks source link

Advice: transfer the logic of PartialEq from Rust to Dart #2238

Open dbsxdbsx opened 3 months ago

dbsxdbsx commented 3 months ago

Currently(Frb v2.2.0), for a Rust type with macro like this

#[derive(PartialEq, Eq, Debug, Serialize, Deserialize, Hash)]
pub enum ProxyNodeEnum {
    Trojan(TrojanNode),
    Vmess(VmessNode),
    Vless(VlessNode),
}

impl ProxyNodeEnum {
...
}

or manually written like this:

#[derive(Debug, Serialize, Deserialize, Hash)]
pub enum ProxyNodeEnum {
    Trojan(TrojanNode),
    Vmess(VmessNode),
    Vless(VlessNode),
}

impl PartialEq for ProxyNodeEnum {
    fn eq(&self, other: &Self) -> bool {
        self.get_name() == other.get_name()
    }
}

impl Eq for ProxyNodeEnum {}

The logic to compare the equivalence of instance of the type can't be transferred to Dart. As we know that Dart would generally compare the memory addresses of instances but not content of the compared instance.

I hope this logic stated in Rust could be transferred to Dart, even not in default, at least it could be transferred by some flag setting or frb attribute macro tag.

fzyzcjy commented 3 months ago

Yes, that looks quite reasonable and feel free to PR for this new feature!

Currently I guess you may workaround by: #[frb(name="operator==")] (and also remember the hashCode function).

dbsxdbsx commented 3 months ago

Currently I guess you may workaround by: #[frb(name="operator==")] (and also remember the hashCode function).

After testing, seems that this #[frb(name="operator==")] takes no effect.

Yes, that looks quite reasonable and feel free to PR for this new feature!

I would give it a try after I finish my current job if this feature is still not implemented then.

fzyzcjy commented 3 months ago

After testing, seems that this #[frb(name="operator==")] takes no effect.

Hmm, then maybe need to check what makes it ignored.

I would give it a try after I finish my current job if this feature is still not implemented then.

Looking forward to it! I guess it will not be too hard, especially the v2 codebase is clearer than v1.

SilverMira commented 2 months ago

Currently, I'm using Riverpod which accepts a RustOpaque parameter which is obtained from public accessor of a non_opaque struct. However, since RustOpaque does not seem to have an underlying override of operator== and hashCode, Riverpod is not able to realize that the same underlying RustOpaque is being used across different calls.

#[frb(non_opaque)]
pub struct MyStruct {
   pub a: i32,
   pub b: third_party::SomeType
}

/// types.rs

pub use third_party::SomeType;
#[frb(opaque)]
#[frb(mirror(SomeType))
struct _SomeType();

For those that are facing this issue, the below seems to work fine: It seems to get us almost there but not quite.

/// types.rs
pub use third_party::SomeType;
#[frb(opaque)]
#[frb(mirror(SomeType))
#[frb(dart_code="
  @override
  bool operator ==(Object other) {
      return other is SomeType && equals(other as SomeType);
  }
")]
struct _SomeType();

#[ext]
pub(crate) impl SomeType {
  #[frb(sync)]
  #[frb(positional)]
  fn frb_override_equals(&self, other: &SomeType) -> bool {
    self == other
  }
}

Directly using #[frb(name="operator==")] on frb_override_equals won't work because Object.operator== expects a bool operator==(Object other) signature, and so will result in invalid_override compilation error.

The above code gets us

abstract class SomeType implements RustOpaqueInterface, SomeTypeExt {
  @override
  bool equals(SomeType other);

  @override
  bool operator ==(Object other) {
    return other is SomeType && equals(other);
  }
}

/// frb_generated.dart
@sealed
class SomeTypeImpl extends RustOpaque implements SomeType {
  // ...
  bool equals(SomeType other) => MyLib.instance.api
      .crateApiSomeTypeFrbOverrideEquals(that: this, other: other);
  // ...
}

Because the concrete type FRB returns is actually SomeTypeImpl and its declaration is class SomeTypeImpl extends RustOpaque implements SomeType, it doesn't actually work, our custom operator== is in SomeType, implements doesn't allow us to inherit the methods from SomeType

it looks like perhaps making the declaration as class SomeTypeImpl extends RustOpaque with SomeType and abstract mixin class SomeType would work.

fzyzcjy commented 2 months ago

Brainstorm: Another way may be, put the custom dart_code in both SomeType and SomeTypeImpl. (since making SomeType a mixin may not be ideal since users may want to use it somewhere)

SilverMira commented 2 months ago

Pretty sure abstract mixin class does allow users to use it as abstract interface exactly as before, but with the benefit of allowing it be used as a mixin, which will allow the SomeTypeImpl class to inherit concrete methods and properties when used as a mixin alongside the usual implements functionality.

We definitely need the definition to be on both the SomeType and SomeTypeImpl, since as of now, if we were to write custom dart_code of a new method that's not one of the builtin ones that Object has like operator== or hashCode, we actually get a compilation error.

And we can do that by either duplicating dart_code or using the inheriting behavior of mixins. Personally I think that mixins will probably be better, since otherwise the concrete methods in SomeType after duplicating dart_code will never be used since we are not extending from it.

fzyzcjy commented 2 months ago

Hmm, will changing abstract class to abstract mixin class be a breaking change? We need to ensure compatibility (unless creating 3.x)

And I am not sure that, since we expose SomeType to end users, they may feel confused "this is reasonable to be an abstract class, but why do you make it a mixin?". So I would like to hide the mixin if possible.

Copy-paste I guess is not a problem, since everything is auto-generated, which is unlike normal copy-pasting (which is a problem) ;)

SilverMira commented 2 months ago

I'm fairly certain abstract class to abstract mixin class is not a breaking change, see Dart 3.0 class modifiers. It just enables the class to be used as a mixin along side what the abstract class already allows (to be extended from, or to be implemented).

As for changing implements to class SomeTypeImpl extends RustOpaque with SomeType, it shouldn't be breaking too unless SomeType have concrete members that FRB itself is generating already (I can't confirm on this), since using with would mean SomeTypeImpl will get inherit all the concrete behaviors (methods/properties) from SomeType.

In any case, if all else fails, we can always reach for the almighty copy pasta. Though, I think dart_code probably should be phased out when dart class augmentation eventually drops, so users can just do whatever they want with the generated classes without having some obscure functionality be taken into account in the codegen.

dbsxdbsx commented 2 months ago

@SilverMira, you just mentioned using RiverPod for statement management, and this just remind me that a case I met when using GetX, for example:

import 'package:get/get.dart';

class MyStruct {
  String name;
  int age;
  double height;

  MyStruct({required this.name, required this.age, required this.height});
}

class MyTestController extends GetxController {
  final Rx<MyStruct> myStruct =
      MyStruct(name: 'John Doe', age: 25, height: 175.0).obs;

  @override
  void onInit() {
    super.onInit();

    // Listen for changes in myStruct
    ever(myStruct, (obj) {

    });
}
...
}

But here I just want to react(the body of ever) only when name field is changed, but not anything else from MyStruct instance, one way is to change the def of String name; to RxString name;--- so, if MyStruct is a generated class, this way would be poisoned, and leads to complicated maintenance even one day frb would generated such kind of code...

Therefore, I finally give up the way with modifying the generated class, in my case, I put the listen logic code just in the MyTestController:

import 'package:get/get.dart';

class MyStruct {
  String name;
  int age;
  double height;

  MyStruct({required this.name, required this.age, required this.height});
}

class MyTestController extends GetxController {
  final Rx<MyStruct> myStruct =
      MyStruct(name: 'John Doe', age: 25, height: 175.0).obs;

  // Create a reactive variable to track the name field of myStruct
  late final RxString _trackedName;

  @override
  void onInit() {
    super.onInit();

    // Initialize _trackedName and associate it with myStruct.name
    _trackedName = myStruct.value.name.obs;

    // Listen for changes in myStruct and update _trackedName
    ever(myStruct, (obj) {
      print('myStruct has been updated: ${obj}');
      _trackedName.value = myStruct.value.name;
    });

    // Only listen for changes in _trackedName
    ever(_trackedName, _onNameChanged);
  }

  void _onNameChanged(String newName) {
    print('Name has been updated to: $newName');
  }
}

This does add some code, but at least semantically easy to maintain. Just for reference, I am not familiar with RiverPod , don't know if there is similar way to work around your case.

SilverMira commented 2 months ago

This does add some code, but at least semantically easy to maintain. Just for reference, I am not familiar with RiverPod , don't know if there is similar way to work around your case.

Or course there are ways to workaround the issue, I can use riverpod to cache the opaque type (which will remain usable until eventually a method that moves the opaque type is called if any). But it does feel that the ergonomics can be improved here without all the manual plumbing work.

Currently my workaround for this is to use custom encoders to convert my opaque type (actually a url::Url) to dart's core Uri type through intermediary string representation, which let me avoid this situation.

Actually, I think this issue only applies for RustOpaques, because frb can generate operator== for non_opaques without issues.

stale[bot] commented 2 days 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.