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

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

Open
dbsxdbsx opened this issue Aug 9, 2024 · 10 comments
Open

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

dbsxdbsx opened this issue Aug 9, 2024 · 10 comments
Labels
awaiting Waiting for responses, PR, further discussions, upstream release, etc enhancement New feature or request

Comments

@dbsxdbsx
Copy link
Contributor

dbsxdbsx commented Aug 9, 2024

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.

@dbsxdbsx dbsxdbsx added the enhancement New feature or request label Aug 9, 2024
@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 9, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

fzyzcjy commented Aug 13, 2024

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.

@fzyzcjy fzyzcjy added the awaiting Waiting for responses, PR, further discussions, upstream release, etc label Aug 13, 2024
@SilverMira
Copy link
Contributor

SilverMira commented Sep 19, 2024

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
Copy link
Owner

fzyzcjy commented Sep 19, 2024

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
Copy link
Contributor

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
Copy link
Owner

fzyzcjy commented Sep 19, 2024

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
Copy link
Contributor

SilverMira commented Sep 19, 2024

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
Copy link
Contributor Author

dbsxdbsx commented Sep 20, 2024

@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
Copy link
Contributor

SilverMira commented Sep 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting Waiting for responses, PR, further discussions, upstream release, etc enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants