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

Implement Eq for Value, Number and Map #638

Closed
smarnach opened this issue Mar 27, 2020 · 3 comments · Fixed by #640
Closed

Implement Eq for Value, Number and Map #638

smarnach opened this issue Mar 27, 2020 · 3 comments · Fixed by #640

Comments

@smarnach
Copy link
Contributor

smarnach commented Mar 27, 2020

All these types currently implement PartialEq. However, they also fulfil the requirements for Eq, so we may implement that trait as well.

The Map type currently has a custom implementation for PartialEq, which looks identical to what we would get by simply deriving PartialEq. If the preserve_order feature is enabled, the code is identical to the PartialEq implementation for IndexMap. For the case that preserve_order is disabled, the code simply defers to the implementation for BTreeMap. Both IndexMap and BTreeMap implement Eq.

The Number type has three variants, wrapping u64, i64 and f64 respectively. The former two are Eq, while the latter is only PartialEq. However, since floating-point numbers in JSON are always finite, we can assume them to be Eq as well, so we can implement Eq for Number.

Once Map and Number implement Eq, it can be derived for Value.

@dtolnay
Copy link
Member

dtolnay commented Mar 27, 2020

Could you share a bit about your use case that requires these impls?

@smarnach
Copy link
Contributor Author

The specific use case where this came up was JSON schema validation. The uniqueItems key can mark the items of an array as unique, and there are no restrictions on the nature of the items, so they may be arbitrary JSON values. One way to check for duplicates is given in this answer on StackOverflow. If Value would implement Eq, I could simply derive it for the newtype wrapper in this code. This doesn't make much of a difference for the code in that answer, but it would save the tedious work of figuring out whether Value is actually Eq.

I personally see this more as a matter of consistency rather than specific to a use case, though. If a PartialEq implementation defines an equivalence relation, the type should generally also implement Eq to mark it as an equivalence relation. The only reason not to is when it is conceivable that a future version of the PartialEq implementation might not be Eq anymore, but I don't think this is the case here – the JSON specification intentionally omitted NaN, and this isn't going to change anytime soon.

@dtolnay
Copy link
Member

dtolnay commented Mar 27, 2020

Thanks, makes sense. I would accept a PR for Eq impls.

smarnach added a commit to smarnach/json that referenced this issue Mar 27, 2020
These types already have `PartialEq` implementations which define equivalence
relations, so we can implement `Eq` as well. Fixes serde-rs#638.
smarnach added a commit to smarnach/json that referenced this issue Mar 28, 2020
These types already have `PartialEq` implementations which define equivalence
relations, so we can implement `Eq` as well. Fixes serde-rs#638.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants