Problem in ISOMonthDayFromFields

[FrankYFTang]

I have hard time to understand the logic in ISOMonthDayFromFields
https://tc39.es/proposal-temporal/#sec-temporal-isomonthdayfromfields

1. The step 7 throw, why do we need to throw under that condition ?

7. If month is not undefined, and monthCode and year are both undefined, then
a. Throw a TypeError exception.

2. It seems once we set referenceISOYear to 1972 in step 11, no code will ever change it, so what got set into
   the [[ReferenceISOYear]] in the return of ISOMonthDayFromFields is always 1972 even if year is not undefined and used in 12-a, is that intentional?

3. why do we decide to use 1972 vs year in RegulateISODate or not based on the "12. If monthCode is undefined", should we depend on "if year is undefined" or not instead?

The whole logic looks very stange to me and I have hard time to undersand what it intend ot do

@justingrant @sffc @pdunkel @ptomato @ryzokuken @Ms2ger

[justingrant]

The whole logic looks very stange to me and I have hard time to undersand what it intend ot do

Yep, this is still one of the hardest parts of Temporal for me to understand, and I helped design it! :-) Below is my best understanding of these issues, but @sffc and @ptomato may know more.

1 - Good question. The docs (quoted below) and #1203 (comment) (where I believe we decided on behavior) say that if calendar is omitted from the input, then { month, day } is sufficient and shouldn't throw. Here's the docs:

Must have either a string monthCode or month property. If month is used and calendar is provided, then year must be provided as well because month is ambiguous in some calendars without knowing the year.

(Note that the docs are not actually correct either, because it's also OK to provide an eraYear / era pair instead of year.)

But the polyfill and the spec say that it should throw, even if calendar is omitted.

@sffc @ptomato - Which is correct? It may be a case where the polyfill code didn't match what we'd decided in GitHub, and then the spec was authored to match the polyfill. Or did we decide later to throw in ISO too if year and monthCode are omitted?

2 - Yes, it's intentional. See #1239. [[ReferenceISOYear]] is always normalized by from and with, where "normalized" means that the same monthCode/day pair should always result in the same [[ReferenceISOYear]] in the output. (If month and year or month and era/eraYear are used, then those are used to calculate monthCode and then the normalization happens.)

Normalization is required so that comparing one PlainMonthDay instance to another (e.g. in equals) can always use slots without having to call into the calendar to normalize years.

Calendars are free to pick any algorithm they want as long as the same monthCode/day combination yields the same [[ReferenceISOYear]] value. For example, the ISO calendar normalizes all years to 1972. For non-ISO calendars, here's the algorithm currently used in the polyfill:

Look backwards starting from the ISO year 1972 up to 100 calendar years to find a year that has this monthCode and day. Normal months and days will match immediately, but for leap days and leap months we may have to look for a while.

Note that this algorithm isn't in the 262 spec because it only applies to non-ISO calendars. The only thing that the 262 spec needs to do is to assign 1972 for the ISO calendar.

Note that this normalization behavior above applies to from and with. For the PlainMonthDay constructor, if I remember correctly, no normalization is done because we didn't want to call into the calendar from the constructor because that would risk an infinite loop. But I'm not 100% sure. @ptomato do you remember? See #1239 to jog your memory.

3 - I don't understand this question. Could you clarify?

[FrankYFTang]

about my Q3
Currently the spec said

11. Let referenceISOYear be 1972 (the first leap year after the Unix epoch).
12. If monthCode is undefined, then
a. Let result be ? RegulateISODate(year, month, day, overflow).
13. Else,
a. Let result be ? RegulateISODate(referenceISOYear, month, day, overflow).
14. Return the new Record { [[Month]]: result.[[Month]], [[Day]]: result.[[Day]], [[ReferenceISOYear]]: referenceISOYear }.

I wonder why the spec is NOT

11. Let referenceISOYear be 1972 (the first leap year after the Unix epoch).
12. If year is undefined, then
a. Let referenceISOYear be year
13. Let result be ? RegulateISODate(referenceISOYear, month, day, overflow).
14. Return the new Record { [[Month]]: result.[[Month]], [[Day]]: result.[[Day]], [[ReferenceISOYear]]: referenceISOYear }.

or

11. Let referenceISOYear be 1972 (the first leap year after the Unix epoch).
12. If monthCode is undefined, then
a. Let result be ? RegulateISODate(year, month, day, overflow).
b. Return the new Record { [[Month]]: result.[[Month]], [[Day]]: result.[[Day]], [[ReferenceISOYear]]: year }.
13. Else,
a. Let result be ? RegulateISODate(referenceISOYear, month, day, overflow).
b. Return the new Record { [[Month]]: result.[[Month]], [[Day]]: result.[[Day]], [[ReferenceISOYear]]: referenceISOYear }.

how come currently we use year in 12-a but not 1972 but always set 1972 in [[ReferenceISOYear]] as return?

[FrankYFTang]

Notice, I am NOT talking about the algorithm in DIFFERENT calendar, I am talking about the algoirthm inside ISOMonthDayFromFields, which is NOT a calendar depend part of algorithm and is specified will NOT be changed depend on calendar. It is NOT defined in Chapter 15 again but ONLY defined Chapter 12.
The text said
The ISOMonthDayFromFields abstract operation implements the calendar-specific logic in the Temporal.Calendar.monthDayFromFields method for the ISO 8601 calendar.

[FrankYFTang]

https://tc39.es/proposal-temporal/#sec-temporal-isomonthdayfromfields

[FrankYFTang]

1 - Good question. The docs (quoted below) and #1203 (comment) (where I believe we decided on behavior) say that if calendar is omitted from the input, then { month, day } is sufficient and shouldn't throw. Here's the docs:

I do not understand why we need to discuss "calendar is omitted" here. This algorithm is used for ISO8601 calendar and only ISO8601 calendar, right? so we know the calendar is ISO8601 in this case and won't be any other calendar.

[FrankYFTang]

For example, the ISO calendar normalizes all years to 1972. :

... then... why in 12-a we have "Let result be ? RegulateISODate(year, month, day, overflow)."
instead of "Let result be ? RegulateISODate(referenceISOYear, month, day, overflow)." ?
IF what you said is true ("normalizes all years to 1972") then why we are still using year instead of referenceISOYear in 12-a?

[FrankYFTang]

@justingrant Notice, the non-iso8601 calendar logic, in https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.monthdayfromfields
is in the ELSE block of "5. If calendar.[[Identifier]] is "iso8601",
and ISOMonthDayFromFields is only called INSIDE the IF block. So everything inside ISOMonthDayFromFields I have issue with is only while we need to deal with ISO8601 calendar and ISO8601 only. And argument/concern about any other calendar is unrelated to these logic.

[FrankYFTang]

I put what I believe it should looke like in https://github.com/tc39/proposal-temporal/pull/1615/files . the purpose of that PR is to seek for understanding of what is missing from that. I understand you folks work a long time on Temporal and must have a good reason of what this part look like right now but I have hard time to understand why is it not what I specified in that PR. Just try to seek for understanding. In particular I find it very confusing about Justin's answer about 1972 and what really got used in the spec, since if year is not undefined it is used in the current spec but Justine said 1972 should always be used....

[justingrant]

1 - Good question. The docs (quoted below) and #1203 (comment) (where I believe we decided on behavior) say that if calendar is omitted from the input, then { month, day } is sufficient and shouldn't throw. Here's the docs:

I do not understand why we need to discuss "calendar is omitted" here. This algorithm is used for ISO8601 calendar and only ISO8601 calendar, right? so we know the calendar is ISO8601 in this case and won't be any other calendar.

Whoops, I was thinking of a different part of the spec. The validation I described above is in https://tc39.es/proposal-temporal/#sec-temporal-totemporalmonthday which is earlier in the call stack than the section you're talking about. I believe that you're correct that by the time https://tc39.es/proposal-temporal/#sec-temporal-isomonthdayfromfields runs, it's OK for year and monthCode to be undefined, because the error case (e.g. PlainMonthDay.from({month, day, calendar: 'iso8601'}) would have been caught earlier.

Therefore, I think you are correct that the spec text below (from https://tc39.es/proposal-temporal/#sec-temporal-isomonthdayfromfields) is wrong, because the validation has already happened in https://tc39.es/proposal-temporal/#sec-temporal-totemporalmonthday. (EDIT: fixed wrong spec reference right before this)

7. If `month` is not `undefined`, and `monthCode` and `year` are both `undefined`, then
a. Throw a `TypeError` exception.

@ptomato - are Frank and I correct that the spec text above is wrong?

[ptomato]

Sorry for the delay in responding to this. It is a tricky part of the spec.

I'm not sure if you still need an answer to the question about ToTemporalMonthDay from an earlier part of the thread, but I don't think there's anything wrong with the spec text in ToTemporalMonthDay:

• If calendarAbsent is true, and month is not undefined, and monthCode is undefined and year is undefined, then
  • Perform ! CreateDataPropertyOrThrow(fields, "year", 𝔽(referenceISOYear)).
• Return ? MonthDayFromFields(calendar, fields, options).

I believe this correctly expresses what @justingrant wrote in prose: if the calendar is absent, then { month, day } is OK. If the calendar is present, then you must have { monthCode, day }, { year, month, day }, or { era, eraYear, month, day } (the last of which should not be described in the spec text, because it is covered by the "implementation-defined processing" of step 6.c of the Intl-aware version of Temporal.Calendar.prototype.monthDayFromFields().) FWIW, I acknowledge that the meeting notes and the discussion in #1203 (comment) don't describe the decision as well as they could, but I think the intention is unambiguously expressed by these tests: https://github.com/tc39/proposal-temporal/blob/main/polyfill/test/plainmonthday.mjs#L79-L96

On to the question about ISOMonthDayFromFields. It's correct that as far as Temporal.PlainMonthDay.from({ month, day }) is concerned, the validation has already happened in ToTemporalMonthDay by the time we reach ISOMonthDayFromFields. So, the question here is about what happens when you call Temporal.Calendar.from('iso8601').monthDayFromFields({ month, day }) directly.

In my recollection, we made the decision on when to allow { month, day } based on the 'shape' of the data that we have, following the same principle that we did elsewhere in the API. This is why we allow { month, day } if the calendar is absent and disallow it if the calendar is present (rather than allow it if the calendar is ISO and disallow it if not.) By that principle, the calendar is present (as the receiver object) in Temporal.Calendar.from('iso8601').monthDayFromFields({ month, day }), so this code should throw. In other words, you don't want someCalendar.monthDayFromFields({ month, day }) to throw sometimes depending on whether your someCalendar variable is the ISO calendar or not.

So, I don't agree that the spec text is wrong, I think it expresses the intention that the champions had when we made this decision. I agree this is useful to bring up in a discussion (though at this time, I personally would disagree with changing it because I don't see any new information that we didn't consider when we made the decision.)

[FrankYFTang]

So, the question here is about what happens when you call Temporal.Calendar.from('iso8601').monthDayFromFields({ month, day }) directly.

In my recollection, we made the decision on when to allow { month, day } based on the 'shape' of the data that we have, following the same principle that we did elsewhere in the API. This is why we allow { month, day } if the calendar is absent and disallow it if the calendar is present (rather than allow it if the calendar is ISO and disallow it if not.) By that principle, the calendar is present (as the receiver object) in Temporal.Calendar.from('iso8601').monthDayFromFields({ month, day }), so this code should throw. In other words, you don't want someCalendar.monthDayFromFields({ month, day }) to throw sometimes depending on whether your someCalendar variable is the ISO calendar or not.

No, that is NOT my concern. My concern is NOT monthDayFromFields, but only ISOMonthDayFromFields. For what ever you describe above, the right way is to code that is to move the if/throw logic to both
12.4.6 Temporal.Calendar.prototype.monthDayFromFields ( fields, options )
https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.monthdayfromfields
and
15.6.2.3 Temporal.Calendar.prototype.monthDayFromFields ( fields, options )
https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.monthdayfromfields

instead of INSIDE ISOMonthDayFromFields . Because if you code it inside ISOMonthDayFromFields, you cannot guarantee in OTHER calendar it will also throws. For example, if you look at
https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.monthdayfromfields
if the calendar is a different calendar other than iso8601, there are guarnatee that "you don't want someCalendar.monthDayFromFields({ month, day }) to throw sometimes depending on whether your someCalendar variable is the ISO calendar or not." because other calendars may NOT throw.
So in the current spec, since you code the throwing behavior inside ISOMonthDayFromFields, instead of inside both version of Temporal.Calendar.prototype.monthDayFromFields, your desire of different consistent behavior is not guaranteed. But you can change it by moving that if / throw from inside ISOMonthDayFromFields to the beginning of Temporal.Calendar.prototype.monthDayFromFields, before the if/else of "5. If calendar.[[Identifier]] is "iso8601", then" in https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.monthdayfromfields
then you can reach your goal of "you don't want someCalendar.monthDayFromFields({ month, day }) to throw sometimes depending on whether your someCalendar variable is the ISO calendar or not." .

[ptomato]

OK, I get what your concern is now. I think this was also intentional but I'll have to look it up in the notes.

Since #1623 is the same but extended to more methods, I suggest we close this one and continue the discussion there?

[FrankYFTang]

I agree w/ you this issue should be discuss more generally first. But I there there are still "stange" thing in the text here (Stange in a sense that I have problem to understand, maybe just my lack of understanding and comprehansion, not necessary imply anything wrong in the spec). Let's first address the general issue in #1623 and maybe I will reopen this ticket once the more general issue got addressed if I still have problem with. (I have problem to understand what got returned in [[ReferenceISOYear]] )