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

Impl PartialEq for Dictionary and VariantArray #990

Open
chitoyuu opened this issue Dec 7, 2022 · 3 comments
Open

Impl PartialEq for Dictionary and VariantArray #990

chitoyuu opened this issue Dec 7, 2022 · 3 comments
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library

Comments

@chitoyuu
Copy link
Contributor

chitoyuu commented Dec 7, 2022

Created during survey of commented code (#377).

Dictionary and VariantArray are missing PartialEq implementations. This is immediately required by the serde serialization test, but should be generally useful to end users as well.

@chitoyuu chitoyuu added feature Adds functionality to the library c: core Component: core (mod core_types, object, log, init, ...) labels Dec 7, 2022
@Bromeon
Copy link
Member

Bromeon commented Dec 7, 2022

One interesting decision here -- do we use Rust equality or Godot equality?

In most cases it should amount to the same, but I could imagine some subleties, especially with types that involve float. I think it's good that you explicitly mention PartialEq (and not Eq).

@chitoyuu
Copy link
Contributor Author

chitoyuu commented Dec 7, 2022

That's good point you bring up. I'd lean toward using Godot's equality operator here to be in line with Variant, but there is an important distinction: the operator== for Dictionary implements reference equality instead of value equality. Using Godot equality here would be consistent from an implementation viewpoint, but inconsistent from a behavior one.

This could be a good argument for an explicit wrapper as suggested here: #712 (comment).

The original suggestion is about a separate type from Ref, but with GATs stablized it should be much easier to write a single reference type encompassing everything now, so that's less of a concern.

@Bromeon
Copy link
Member

Bromeon commented Dec 7, 2022

the operator== for Dictionary implements reference equality instead of value equality. Using Godot equality here would be consistent from an implementation viewpoint, but inconsistent from a behavior one.

Noteworthy:

  • This has been fixed in Godot 4: Dictionary now uses value equality (see PR).
  • At the same time, Godot 3 now has the deep_equal utility function (see PR).

I'm not sure if an explicit wrapper type is truly needed, or if a deep_equal method would do the job as well.


I'd lean toward using Godot's equality operator here to be in line with Variant,

I'm not sure if the equivalence (a == b) == (a.to_variant() == b.to_variant()) currently holds for all cases.
I quickly tested, it holds for f32::NAN and f64::NAN though.


It eventually amounts to a choice between:

  1. Do we want to keep the broken GDScript behavior, but stay consistent with Godot (and Rust's Variant in its current implementation).
    • Helpful for people porting GDScript code or largely familiar with GDScript.
  2. Or do we want to make the default operation the intuitive one in Rust (value equality), while tripping up people porting GDScript code?
    • Helpful for people starting with the Rust binding, or deliberately choosing Rust for better type safety.
    • Should we even try to "fix" Variant equality one day?

I don't think godot-rust has very clearly favored one way over the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library
Projects
None yet
Development

No branches or pull requests

2 participants