Span: the Eq/PartialEq trait implementation can lead to benign-looking code that is actually buggy.

[jhseu]

I'm doing a experimental migration of my project from chrono to jiff, and I've run into subtle errors involving Span comparisons. This usually involves subtracting two DateTimes and checking that its TimeDelta/Span is some value.

The documentation says:
A Span implements the PartialEq and Eq traits, but not the PartialOrd or Ord traits. In particular, its Eq trait implementation compares for field-wise equality only. This means two spans can represent identical durations while comparing inequal

Perhaps Span should not implement PartialEq and Eq at all. I would rather the compiler give an error if I'm doing the wrong thing rather than have a subtle bug go through. This behavior breaks the principle of least surprise. If a person unfamiliar with jiff sees &dt1 - &dt2 == some_span in code review, they probably would not think that it's a serious bug.

[BurntSushi]

(Note: I think what you might want is Span::compare.)

When doing the API design, I did think about this case and wondered what the right trade off is. In the way things are setup now, it's not that the Eq impl is wrong per se, because sometimes you do want to check for precise field-by-field equality. But this does bring up an important expectation mismatch with other datetime libraries that lean more heavily on absolute duration types (like std::time::Duration or TimeDelta from Chrono). In that case, doing direct equality comparisons is correct because they don't distinguish between, e.g., 120 seconds and 2 minutes.

This is a tough one because the Eq impl is incredibly convenient, especially for writing tests. Putting it into a method like Span::fieldwise_eq would be a major hit to ergonomics. But for tests, this can be solved with a macro or something, and so shouldn't be that big of a deal.

With all that said, the fact that dt1 - dt2 == some_span looks totally benign but potentially isn't is, I agree, a significant demerit to Jiff's current API.

[BurntSushi]

I'm calling this a bug in the API because I think we probably need to do something about this unfortunately.

[rben01]

I wonder if it would make sense to remove PartialEq from Span and then add types (available via methods or as newtype wrappers or both) that implement PartialEq in different ways. E.G., span1.calendrical() == span2.calendrical() for a field-wise check and span1.temporal() == span2.temporal() for a time delta check. This would force the user to make explicit which kind of equality they want.

[BurntSushi]

I plan to at least remove the PartialEq and Eq trait implementations on Span for jiff 0.2.

I'm not sure if adapter types make sense yet. The interesting thing is that neither calendrical() nor temporal() are fully correct. The fully correct implementation is Span::compare, and requires a relative date for dealing with non-uniform units. (Which are usually calendar units, making the naming of calendrical() for fieldwise comparison very confusing IMO.)

[BurntSushi]

I ended up implementing this. There's a new SpanFieldwise type with PartialEq, Eq and Hash trait impls. And a Span::fieldwise method to conveniently create SpanFieldwise values.

I think this is probably overall the right thing to do here. The trap demonstrated in this issue is extremely easy to fall into. With that said, I do find it annoying to deal with types that don't implement PartialEq or Eq. They are very fundamental. I can just imagine the problems folks are going to have: they put Span into some struct, derive Serde traits and then get annoyed that they can't do assert_eq! easily in tests. This could wind up pushing folks to use SignedDuration instead, which would be a bummer.

So I'm not quite sure what the right trade-off here is, but I think it makes sense to move to the more conservative stance in jiff 0.2 (with these traits not implemented). If it ends up being a mistake, it's easy to fix by just re-adding the trait implementations.

Also, this changes was very annoying to do in Jiff because of all the doc tests doing assert_eq!(span1, span2). Every one of those assertions had to be updated. Normally this probably would have swung me towards leaving these trait implementations in tact, but I'm pretty sure that Jiff is somewhat of a special case in that it really wants to have a bunch of tests on the precise Span value in memory as opposed to the actual duration.

So in summary, I am willing to revisit this issue.