Pass overflow option value directly to CalendarDateToISO

[FrankYFTang]

I think the specification in the following two section should be changed
https://tc39.es/proposal-temporal/#sec-properties-of-the-temporal-calendar-prototype-object
https://tc39.es/proposal-temporal/#sup-properties-of-the-temporal-calendar-prototype-object

The current pattern in the spec in these two section is as below
for example, compare
https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.datefromfields
vs
https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.datefromfields

in 12.4
some common code
Assert: calendar.[[Identifier]] is "iso8601".
...
Let result be ? ISODateFromFields(fields, options).
some other common code

in 15.6.2
some common code
If calendar.[[Identifier]] is "iso8601", then
Let result be ? ISODateFromFields(fields, options).
Else
Let result be a .... that is the result of implementation-defined processing of fields, overflow, and the value of calendar.[[Identifier]].
some othercommon code

and then inside ISODateFromFields or other AO for the ISO8601 calendar, the spec text read the options and validate and throw if some fields are undefined.

I think this is wrong, becaues if the spec in this way, then there are no guaratee that other calendar will also throw on such undefined, not only that, the language "the result of implementation-defined processing of fields, overflow, and the value of calendar.[[Identifier]]" is not clear it is allow to throw exception or not, or only constrain to set the result.

I first discover this in #1614 but I think this is generally applicable to all methods which have entries in both in 12.4 and 15.6.2
I think we should make Editorial change, to move the reading of these options from inside these ISOXXX method into text in 12.4 first, and make sure if x is undefined throw is in text of 12.4 not in AO, and then pass those field into ISOXXX method as arguments, without passing in the options object to ISOXXX AO and make sure all the undefine check happen outside these ISOXXX AOs. and in the text of 15.6.2 , change it similar to 12.4 as I mentioned above, and make sure the if x is undefined throw check happen before the if calendar is iso8601 fork out. In this way, we can ensure all the necessary get field undefined check is consistent in both iso8601 and other kind of calendar. The other calendar may throw for other additional condition (for example, throw if era is undefined) . This is achieveable with Editorial change I believe since the behavior changing part which will really got impacted are the "other calendar" block in 15.6.2 and that part is already " implementation-defined processing" anyway.

[FrankYFTang]

@justingrant @ptomato @sffc @ryzokuken @Ms2ger

[FrankYFTang]

To make it clear, the only parts I think we should change are

12.1.37 ResolveISOMonth ( fields )
12.1.38 ISODateFromFields ( fields, options )
12.1.39 ISOYearMonthFromFields ( fields, options )
12.1.40 ISOMonthDayFromFields ( fields, options )
12.4.4 Temporal.Calendar.prototype.dateFromFields ( fields, options )
12.4.5 Temporal.Calendar.prototype.yearMonthFromFields ( fields, options )
12.4.6 Temporal.Calendar.prototype.monthDayFromFields ( fields, options )
15.6.2.1 Temporal.Calendar.prototype.dateFromFields ( fields, options )
15.6.2.2 Temporal.Calendar.prototype.yearMonthFromFields ( fields, options )
15.6.2.3 Temporal.Calendar.prototype.monthDayFromFields ( fields, options )

I think
ISODateFromFields should be changed to
ISODateFromFields(day, month, monthCode, year, overflow)

ISOMonthDayFromFields should be changed to
ISOMonthDayFromFields(day, month, monthCode, year, overflow)

ISOYearMonthFromFields should be changed to
ISOYearMonthFromFields(month, monthCode, year, overflow)

ResolveISOMonth should be changed to
ResolveISOMonth(month, monthCode)

and all the validation of undefined should be done outside these function and in the common (common of all calendar, regardless iso8601 or not) part of Temporal.Calendar.prototype.*Fields methods. These function may throw on other validation, but the Get and if undefined throw should happen outside these ISOXXX AOs and apply to other calendars.

[ptomato]

I thought we had done this intentionally, but I can't find the discussion anymore. If I remember correctly, it was done because non-ISO calendars may need to take other options than overflow that are not defined for the ISO calendar. But these steps only apply to built-in calendars, not userland custom calendars, and I don't think it's the case that any built-in non-ISO calendars supported by Intl would need to handle any other options than overflow.

If a calendar added as built-in in the future did need to handle other options, then we could revert back to "passing" the options object as well as overflow into the implementation-defined part. As per #1388 we do need to ensure that the implementation-defined calendar code doesn't Get the overflow property a second time after we Get it from the options object, so that might be a concern. In that regard, the current text is more future-proof. But maybe that could be solved with a note specifying that the implementation-defined operation should not Get properties that the surrounding AO already Gets.

@sffc Do you remember if my above recollection is correct?

We could make the change to ISOXXXFromFields regardless of the above, although it's probably not worth inlining those AOs anymore in that case (ISODateFromFields would just be a single call to RegulateISODate).

I think you're correct this would be an editorial change.

[FrankYFTang]

What you said is the concern of additional steps or throw in other custom calendars may want to take. What I requested to change is the minimum steps all calendars need to take. There are no conflict between these two. What we ask for minimum won't prevent any additional tasks or throw , right?

[FrankYFTang]

As per #1388 we do need to ensure that the implementation-defined calendar code doesn't Get the overflow property a second time after we Get it from the options object, so that might be a concern.

There are no way you can "ensure" that even if you pass in the options object. The implementation-defined calendar code once access the options can call Get("overflow") 10 times and pass in the options object alone won't have the ability to "stop" that.

If you try to avoid multiple Get of "overflow" to the options from the implementation-defined calendar but also allow it to read other value from the option, then I think (iffy, sorry) the the only way you can achieve that, is you "clone" every property from the options to a new copy, and pass that new copy to the implementation-defined calendar, during the process, you can first Get "overflow" , keep the value around, and not copy that value into the newly cretated cloned_options and pass in both the overflow and cloned_options into the implementation-defined calendar. In this way, every property of the options will be Get exactly once, and the Get of implementation-defined calendar call will only Get from the cloned_options, which is not observerable, but not from options which is observable. (I think. Maybe I am wrong about this)

[FrankYFTang]

Just a note- My question in #1614 actually have two different parts, one is capture as more general here so it is good you close that one, but there is another issue which si specific to ISOMonthDayFromFields so I open a new one #1636 to discuss that. That one is not related to the option reading or throwing issue but in particular how the value got constrain and the value of the reference year and should be deal separately.

[ptomato]

As per #1388 we do need to ensure that the implementation-defined calendar code doesn't Get the overflow property a second time after we Get it from the options object, so that might be a concern.

There are no way you can "ensure" that even if you pass in the options object. The implementation-defined calendar code once access the options can call Get("overflow") 10 times and pass in the options object alone won't have the ability to "stop" that.

I agree. There is no enforcement possible if we pass in the options object.

If you try to avoid multiple Get of "overflow" to the options from the implementation-defined calendar but also allow it to read other value from the option, then I think (iffy, sorry) the the only way you can achieve that, is you "clone" every property from the options to a new copy, and pass that new copy to the implementation-defined calendar, during the process, you can first Get "overflow" , keep the value around, and not copy that value into the newly cretated cloned_options and pass in both the overflow and cloned_options into the implementation-defined calendar. In this way, every property of the options will be Get exactly once, and the Get of implementation-defined calendar call will only Get from the cloned_options, which is not observerable, but not from options which is observable. (I think. Maybe I am wrong about this)

I prefer the earlier solution that you proposed (only passing overflow to the implementation-defined operation and not options.) I think it is fine to do that if no built-in non-ISO calendars would need to process any other options besides overflow. I was just waiting to hear @sffc's opinion on this.

[ptomato]

@sffc Do you have an opinion on this regarding whether it might be necessary in the future to pass more options than "overflow" to CalendarDateToISO?

[sffc]

So for the fields argument of CalendarDateToISO, all three call sites pass a bag of fields returned by PrepareTemporalFields, which never contains unknown fields. Fortunately, that part is being fixed by #2199.

For the options argument, I can imagine that a calendar may need additional information in the future that isn't covered by fields (perhaps something like a transition date or an astrological database), but I think it would be a minor change to allow that additional data to flow if/when we need it. For now it seems clean enough to only pass the one overflow option.

[ptomato]

OK, based on the foregoing discussion I think we can make this editorial change.

[ptomato]

This was already done at some point.