How to handle Temporal.PlainMonthDay.equals() with identical month/day but different reference years?

[justingrant]

Currently Temporal.PlainMonthDay.equals() compares the ISO month, day, and reference year to determine equality.

IMHO this is incorrect behavior, because the result of equals() should return the same result as a deep-equals on the result of getFields() which only exposes { month, day, calendar }. The reference year is just an implementation detail for non-ISO calendars and therefore the reference year shouldn't affect user-facing equality.

Here's a repro showing the first day of the Hebrew calendar year (aka Rosh Hashanah) using two different ISO reference years. From the perspective of a PlainMonthDay, IMHO these should really be considered the same.

one = new Temporal.PlainMonthDay(9, 7, 'hebrew', 2021);
two = new Temporal.PlainMonthDay(9, 19, 'hebrew', 2020);
one.getFields();
// => {day: 1, month: 1, calendar: 'hebrew'}
two.getFields();
// => {day: 1, month: 1, calendar: 'hebrew'}
one.equals(two);
// expected: true
// actual: false

Note that resolution of this issue is orthogonal to #1203. Regardless of how we name and type the year-independent and year-dependent month properties in #1203, the implementation of equals() should be using the year-independent month code (e.g. "5L") to compare PlainMonthDay instances, not the year-dependent month index.

[justingrant]

Another alternative solution would be to remove equals() from PlainMonthDay and require users to manually compare the calendar, day, and month code. IMHO this would be a bad solution because it would make it harder to write cross-calendar-safe code using PlainMonthDay. Using equals() at ensures that the comparison is always done correctly.

[justingrant]

Another possible solution would be to normalize the reference year of all PlainMonthDay instances in the constructor, so that for a particular month, day, and calendar tuple the same ISO reference year would always be used. I don't think we could do this using the current API because it'd cause an infinite loop where the PlainMonthDay constructor calls into Calendar.prototype.monthDayFromFields which would have to call the constructor to create the instance. But we could add a new Calendar prototype method e.g. getReferenceYear({monthCode, day}), or we could add another optional parameter to the PlainMonthDay constructor e.g. normalize = true which Calendar.prototype.monthDayFromFields could pass to prevent the infinite loop.

EDIT: removed accidental crud below here

[justingrant]

BTW, the same issue applies to PlainYearMonth too.

[Louis-Aime]

To be honest, I do not understand how the referenceYear of the constructor can be used for equals.

• The fields in the constructor eventually designate an absolute day (say, a Julian Day number).
• Referring to the calendar, this absolute day resolves to a {month, day } bag in the custom calendar.
• Here one.equals(two) means: "day two, when expressed in custom calendar, yields same month and day elements than day one (also expressed in custom calendar)".
• Neither the ISO fields, nor the referenceYear element should be used for this comparison.

[justingrant]

@Louis-Aime - referenceYear is used because internally, all Temporal objects only store ISO data. A PlainMonthDay, for example, has 4 "slots" (a "slot" is like a private field): ISO_YEAR, ISO_MONTH, ISO_DAY, and CALENDAR. You can see these fields using the getISOFields() method:

Temporal.PlainMonthDay.from({ month: 12, day: 1, calendar: 'japanese' }).getISOFields()
// => {calendar: Calendar, isoDay: 1, isoMonth: 12, isoYear: 1972}

Calendar fields are never actually stored inside the Temporal object. Instead, they're converted to ISO on input (e.g. from({month, day, calendar}), and dynamically computed from ISO for output (e.g. .month).

The ISO_YEAR field is needed for PlainMonthDay because there needs to be a way to translate what's stored internally (ISO fields only) to what the user wants (calendar field values). Doing this translation requires an ISO year. Temporal could have also used a Julian day or any other mechanism to uniquely identify a date in a calendar-neutral way, but it was chosen (long before I was involved) to use ISO year, month, and day fields for this purpose.

Does this explanation clarify why referenceYear is needed?

BTW, referenceDay is used by PlainYearMonth for the same reason.

[Louis-Aime]

Thank you @justingrant for the explanation.

Currently Temporal.PlainMonthDay.equals() compares the ISO month, day, and reference year to determine equality.
If Temporal.PlainMonthDay.equals() only uses ISO fields, without the calendar, that cannot work. You demonstrated it!

A first issue is to make clear in the documentation for the calendar author that the ISO fields, apart for Calendar, designate a unique and unambiguous day as would a Julian Day or any other chronological day counter do.

Then it is clear that the fields in the constructor designate a date that is a possible instance of the MonthDay. This is what the calendar's author should understands when choosing a reference isoYear while writing its monthDayFromFields.

Temporal.PlainMonthDay.equals() should just calculate the Day and Month fields in the custom calendar, and return true if they are the same, using the year-independant month notation e.g. 5L.

A possible solution is to ask authors to provide an "equals" method in their Temporal.Calendar instances. They will do that from themselves if the "standard" Temporal.PlainMonthDay.equals() does not respond the user's needs.

[justingrant]

BTW, an optimization that's probably worthwhile: the PlainMonthDay.prototype.equals algorithm should first compare the internal ISO slots. If they're equal, then the instances are equal and no getters need to be called. However, if they're not equal, then the getters should be called to check whether the month, day, and calendar are equal or not, while ignoring the reference year.

Ditto for PlainYearMonth.compare and PlainYearMonth.prototype.equals, except ignoring the reference day for that type.

[ptomato]

My thought has been that JS programmers should just not do something like this code from the OP, unless they are implementing a calendar:

one = new Temporal.PlainMonthDay(9, 7, 'hebrew', 2021);
two = new Temporal.PlainMonthDay(9, 19, 'hebrew', 2020);

Calendars should be able to choose what reference years they associate with what month-day, and IMO they can't reasonably be expected to deal with objects that someone has created with an unexpected reference year. It's unfortunate that we need to expose the reference year at all, because it means that people can use the constructor to create objects with invalid internal state. But I don't think that justifies removing equals() from PlainMonthDay, or making equals() delegate to the calendar when it doesn't need to, and I'd rather say "garbage in, garbage out" for this case. We should encourage programmers to do this instead:

one = Temporal.PlainMonthDay.from({ month: 1, day: 1, calendar: 'hebrew' });
two = Temporal.PlainMonthDay.from(one);

Relatedly, I think we should require (though I don't know that we can enforce) that PlainMonthDay.from(fields) always makes a PlainMonthDay instance with the same reference year, given the same fields.

[justingrant]

Hi @ptomato - Welcome back and Happy New Year!

Relatedly, I think we should require (though I don't know that we can enforce) that PlainMonthDay.from(fields) always makes a PlainMonthDay instance with the same reference year, given the same fields.

Yep, if we retain the current design then we'd definitely want to document this, and to implement this behavior in all built-in calendars.

I assume that we should also mandate that the default PlainYearMonth reference day is always 1?

But I don't think that justifies removing equals() from PlainMonthDay, or making equals() delegate to the calendar when it doesn't need to

Other than the API change (which I know we'd really want to avoid at this stage!) what are other downsides to comparing calendar fields (via getters for non-built-in calendars, via optimized non-observable code for built-ins) and ignoring the reference year?

I agree that the constructor is not what we want developers to use, but it also seems good to eliminate a class of bugs if there's not a big cost to doing so. Especially if #1240 ends up requiring the reference year, if any developers do use the constructor then they're likely to commit this bug.

My weakly-held opinion is still that using getters seems a little safer and (for built-in calendars) about as performant. That said, I don't have a strong opinion on this one either way.

BTW, here's an implementation I had in mind:

  equals(other) {
    if (!ES.IsTemporalMonthDay(this)) throw new TypeError('invalid receiver');
    other = ES.ToTemporalMonthDay(other, PlainMonthDay);
    // optimization: avoid calling getters if slots are equal
    if (
      GetSlot(this, ISO_YEAR) !== GetSlot(other, ISO_YEAR) ||
      GetSlot(this, ISO_MONTH) !== GetSlot(other, ISO_MONTH) ||
      GetSlot(this, ISO_DAY) !== GetSlot(other, ISO_DAY)
    ) {
      // Call getters; month/day may match even if reference year doesn't
      for (const prop of ['day', 'month']) {
        const val1 = this[prop];
        const val2 = other[prop];
        if (val1 !== val2) return false;
      }
    }
    return ES.CalendarEquals(this, other);
  }

[justingrant]

Relatedly, I think we should require (though I don't know that we can enforce) that PlainMonthDay.from(fields) always makes a PlainMonthDay instance with the same reference year, given the same fields.

Yep, if we retain the current design then we'd definitely want to document this, and to implement this behavior in all built-in calendars.

One more question: should the constructor store the reference year as-is, or should it canonicalize it?

[ptomato]

What do you mean by canonicalize?

[justingrant]

What do you mean by canonicalize?

Example: should new PlainMonthDay(2, 29, 'gregory', 1980) end up storing a reference year of 1972 or 1980? "Canonicalize" in this context would mean storing 1972.

We currently canonicalize time zones and calendars in constructors:

new Temporal.ZonedDateTime(0n, 'Asia/Kolkata')
ZonedDateTime {_repr_: "Temporal.ZonedDateTime <1970-01-01T05:30:00+05:30[Asia/Calcutta]>"}

Should we do the same for reference years?

PlainMonthDay.from(fields) always makes a PlainMonthDay instance with the same reference year, given the same fields.

BTW, I think we should be even more proscriptive. For any particular (year-independent) month code and day (see #1203), the reference year should be the same. This assumes (see #1203) that there may be multiple ways to define a month in from: a string code, a numeric index into a particular year, etc. Just because I used a numeric index or a string code (or a signed year or era-based year for PlainYearMonth) shouldn't affect the reference value in the result.

[ptomato]

I'm not convinced of that approach in #1203, but assuming it's the case then I agree.

I see what you mean by canonicalizing — although wouldn't that result in an infinite loop for non-builtin calendars?

[justingrant]

I'm not convinced of that approach in #1203, but assuming it's the case then I agree.

The high-order bit is that lunisolar leap months aren't in every year. There needs to be a way to refer to those months in Temporal fields. If there are multiple ways to refer to those months (e.g. month index + year, or month code) then choosing different representations for the same underlying month shouldn't result in a different reference year.

I see what you mean by canonicalizing — although wouldn't that result in an infinite loop for non-builtin calendars?

I think the constructor could pass the calendar a fake constructor for the new instance which could avoid the infinite loop. Correct?

[ptomato]

Meeting, 14 Jan: There is no problem with equals() if the calendar is always responsible for choosing a consistent reference year for PlainMonthDay, and PlainMonthDay.from() will never take a passed-in year field into account. This is the status quo, so closing this issue.

[justingrant]

BTW, #1245 does not currently follow this best practice. Currently it starts from the current year and searches backwards until it finds a reference year where the desired date exists. I'll update #1245 to choose a constant year to start searching for matching months.

EDIT: this is fixed now. #1245 now computes the canonical year according to this issue

[justingrant]

PlainMonthDay, and PlainMonthDay.from() will never take a passed-in year field into account.

One minor note: PlainMonthDay.from() should use the passed-in year to determine the meaning of the month index (if we have a month index property that is accepted by from). But after the monthCode is determined from year and month index, then the calendar should pick a canonical reference year for that monthCode and day. This canonical reference year may (and likely will) differ from the passed-in year.