libipld icon indicating copy to clipboard operation
libipld copied to clipboard

Question: Why isn't `Eq` implemented for `Ipld` data type?

Open ProofOfKeags opened this issue 2 years ago • 9 comments

I see that PartialEq is implemented (via derivation), however, I don't see any annotation in the source for why Eq cannot be implemented. Does this have something to do with Null?

ProofOfKeags avatar Dec 12 '22 20:12 ProofOfKeags

The reason are probably the floats. Though as the IPLD forbids non-numeric floating point numbers (such as NaN), it should be possible to implement it manually.

vmx avatar Dec 12 '22 20:12 vmx

I'm taking this to mean a PR will be accepted that implements it manually.

ProofOfKeags avatar Dec 12 '22 20:12 ProofOfKeags

A problem I haven't thought about before seeing the PR is: Currently we don't enforce that there are no non-numeric numbers on the data model level. So how do we deal if someone is using a non-numeric value? Do we we just document it properly, that then equality might not work as expected? Should we put debug inserts in there? Any other ideas?

Thoughts from other would be appreciates (cc @Stebalien).

vmx avatar Dec 12 '22 20:12 vmx

Is there a way to prevent the export of the individual enum constructors and use smart constructors that forbid the use of NaN and Infinity? That's the way I'd do it in Haskell. This would prevent manual construction and enforces that Iplds can only be constructed through its blessed APIs

ProofOfKeags avatar Dec 12 '22 20:12 ProofOfKeags

So it appears Rust does not support private constructors for enums. I will adjust the PR to newtype f64 into a value that ensures finiteness.

ProofOfKeags avatar Dec 12 '22 21:12 ProofOfKeags

I'd simply define Ipld::Float(NAN) == Ipld::Float(NAN), etc. We're not asking "are these numbers equal", we're asking "are these two IPLD nodes equivalent".

Really, equivalence should be defined as a == b <-> canonical_encoding(a) == canonical_encoding(b).

Stebalien avatar Dec 20 '22 22:12 Stebalien

(which assumes NaN canonization, but we should be doing that anyways)

Stebalien avatar Dec 20 '22 22:12 Stebalien

Hm. Of course, we currently don't accept those in DAG-CBOR (per spec) then accept them in the implementations... It would be convenient to just accept them and be done with it.

Stebalien avatar Dec 20 '22 22:12 Stebalien

I just discovered that f64 got total_ord in Rust 1.62. What if we just use that? As per IPLD Data Model spec, there shouldn't be any non-number values, but if it happens, we just deal with as total_ord is defined.

vmx avatar Dec 21 '22 08:12 vmx