Add Eq and Ord for PlainYearMonth + Eq for PlainMonthDay

[nekevss]

This PR adds Eq and Ord trait implementations for PlainYearMonth and an Eq trait impl for PlainMonthDay to enable implementing Temporal.PlainYearMonth.compare, Temporal.PlainYearMonth.prototype.equals and Temporal.PlainMonthDay.prototype.equals.

[jedel1043]

LGTM!

[jedel1043]

Wait, now that I think about it, why do we derive Eq but implement Ord manually? I'd think we should pick one or the other, since it would be pretty weird to have both differ on their results when two dates are the same except for their calendar.

[nekevss]

The primary reason is because equals methods assert whether the iso date is equal and is the same calendar, but the compare methods only compare the iso date. So this should align with the specification.

Instead of implementing Ord and Eq, we could switch to having an equals and compare method, but the functionality would be the same, and I'm not entirely sure if that's a net benefit to the general API 😕

[jedel1043]

I think it would be very surprising to have two very similar traits that return two completely different results. The std actually documents that having both a < b and a == b be true is a logical bug:

https://doc.rust-lang.org/stable/std/cmp/trait.PartialOrd.html

The methods of this trait must be consistent with each other and with those of PartialEq. The following conditions must hold:
a == b if and only if partial_cmp(a, b) == Some(Equal).

If we need a different equality operation, I would suggest having it as a standalone method; maybe weak_cmp/eq, then have the strict versions (considering the calendar in the equality) be the implementations of Eq and Ord.

[nekevss]

I don't disagree that it may be surprising. But then again, there are plenty of surprising things with date/time. I'm not entirely sure that we can do a weak_cmp/equal.

I'm not sure I agree with the PartialOrd/PartialEq being the strict implementation though. I could be overlooking something, but in order to do a calendar aware PartialOrd implementation, we would need to calculate the calendar specific date values in the PartialOrd implementation, which may be fallible or offer less precision at certain date limits depending on the calendar. Basically doing so could turns a seemingly innocuous comparison date1 > date2 into a computation + comparison (granted, we'd have to test a lot of cases to be sure).

If we keep the trait implementations, I think the better route would be to have the "weaker" iso specific comparison be the trait implementations with a strict_equals, but then we are opening up the door for date1 == date2 to not actually be equal to each other because they are the same IsoDate, but they are not the same calendar date.

Both could probably be remedied by proper documentation though. Although, I am leaning towards the best option being to have methods rather than the trait implementations. I'd curious to hear the thoughts of the Temporal champions / if they had any similar discussion.

EDIT - Potentially relevant issue: tc39/proposal-temporal#1431

[nekevss]

Is there any preference on moving forward here with the context from 1431?

Given some of the context from the linked issue, I'd propose two separate solutions:

1. All temporal_rs calendar date types implement PartialEq and PartialOrd acccording to the specification, but not Ord or Eq.
2. Remove any trait implementations and add compare and equals methods to all calendar date types.

The above would then move beyond implementation on PlainYearMonth and PlainMonthDay to include PlainDate, PlainDateTime, ZonedDateTime as well for consistency.

My preference is 1 as I think that aligns with the comments in some of the temporal proposal issues. But I think both are valid approaches.

[jedel1043]

Yeah implementing a partial ordering seems like the best option here.

[jedel1043]

Shouldn't this check that both have the same calendar first?

[nekevss]

On PartiaOrd? Not if we're going based off specification. We can check calendar and return None, but we'll end up failing intl402 tests.

[jedel1043]

Uhhh that's awkward...
The thing is that we cannot break things like a <= b <=> a < b || a == b, or some libraries depending on that property could cause hard to debug problems...

I'd just implement the operations as methods then. That way we're not involuntarily breaking downstream code, and we can document the spec behaviour on the methods.

[nekevss]

I can that's probably the best approach.

I do think it's fine to implement though as is if we wanted since conditions 2-5 are upheld by PartialOrd. The question mostly seems to be around the language "a == b if and only if partial_cmp(a, b) == Some(Equal)." Depending how "if and only if" is applied, I think this is still technically upheld because we'd be returning a subset of values. We never return a value where partial_cmp is not Some(Equal).

But for safety, it's probably best to just use methods

[jedel1043]

"a == b if and only if partial_cmp(a, b) == Some(Equal)."

This property is the same as saying a != b iff partial_cmp(a,b) != Some(Equal), which with the current implementation would be broken by two dates that are the same ISO date with different calendars, so you can definitely break that invariant trivially.

[nekevss]

Yeah 😕 that's true.

Methods are the best way to go.

[jedel1043]

Wild idea. What if we derived PartialEq/Eq, but exposed comparisons as a compare_iso method? I can see == being useful to have, and we can document on compare_iso the reason why we don't implement PartialOrd/Ord.

[nekevss]

Hmmmmm, sounds fine to me. That strikes a decent enough balance between the two and would represent the general functionality well.

[jedel1043]

LGTM!