Please restrict the input type of Temporal.Calendar.prototype.fields(fields)

[FrankYFTang]

I would suggest we restrict the input type of fields in Temporal.Calendar.prototype.fields (fields) as well as the step after a. Set fieldsArray to ? Call(fields, calendar, « fieldsArray »). in CalendarFields

from iterable to just an array and also put more restriction on it.

1. The input type https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.fields 12.4.21 Temporal.Calendar.prototype.fields ( fields )

Currently this is the only input in the Temporal proposal to accept iterable. But I found it is really an overkill to take it as an iterable

First, of all, the fields method is designed intened as an internal communcation prototocol between the rest of the Temporal object to support different kind of calendar system, it allow each Calendar to filter/add fields before the calling of PrepareTemporalFields() to gather necessary fields

In the proposal the only place which will call into calendar is via https://tc39.es/proposal-temporal/#sec-temporal-calendarfields 12.1.5 CalendarFields ( calendar, fieldNames )

and CalendarFields is always turning fieldNames into a fieldsArray before the Call() , therefore, we know any internal calling to the Temporal.Calendar.prototype.fields () is always an array, never any other kind of iterable . Therefore, there are no reason, for the purposing of seving internal usage to take anything other than Array (an Object with "length" property)

2. the Input value, if we read through the spec, the only values which call into CalendarFields are restricted to the following $ egrep -n CalendarFields |egrep "«" abstractops.html:497: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "month", "monthCode", "year" »). plaindate.html:295: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plaindate.html:310: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "monthCode" »). plaindate.html:386: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "month", "monthCode", "year" »). plaindate.html:691: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "month", "monthCode", "year" »). plaindatetime.html:396: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year" »). plaindatetime.html:674: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plaindatetime.html:689: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "monthCode" »). plaindatetime.html:917: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year" »). plainmonthday.html:154: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "month", "monthCode", "year" »). plainmonthday.html:244: 1. Let receiverFieldNames be ? CalendarFields(calendar, « "day", "monthCode" »). plainmonthday.html:248: 1. Let inputFieldNames be ? CalendarFields(calendar, « "year" »). plainmonthday.html:364: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "month", "monthCode", "year" »). plainyearmonth.html:236: 1. Let fieldNames be ? CalendarFields(calendar, « "month", "monthCode", "year" »). plainyearmonth.html:259: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plainyearmonth.html:286: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plainyearmonth.html:319: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plainyearmonth.html:356: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plainyearmonth.html:456: 1. Let receiverFieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). plainyearmonth.html:458: 1. Let inputFieldNames be ? CalendarFields(calendar, « "day" »). plainyearmonth.html:565: 1. Let fieldNames be ? CalendarFields(calendar, « "month", "monthCode", "year" »). zoneddatetime.html:569: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year" »). zoneddatetime.html:961: 1. Let fieldNames be ? CalendarFields(calendar, « "monthCode", "year" »). zoneddatetime.html:979: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "monthCode" »). zoneddatetime.html:1111: 1. Let fieldNames be ? CalendarFields(calendar, « "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year"* »).

therefore, I think we should restrict the values of the element to the fiels to only contains the following: « "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year" »

I think we should assert if the lengh of the array is > 10 and if the item is not in any of the above

3. return type (for subclass) Also, since the specification said it should return an Array, we therefore should not need to handle the return of the Call, in 12.1.5 CalendarFields ( calendar, fieldNames ), to be any iteratable, but only treat it as an array
4. Return ? IterableToListOfType(fieldsArray, « String »).

Allowing an iteratable as fields for the input of Temporal.Calendar.prototype.fields(fields) is an overkill and at least in v8 will make the implementation more complex than it needed to. I would suggest we change the specification and only take Array with length < 10 as input, and throw if the input is not one of the 10, and also change the specification to only allow the return of the fields to be Array, not a general iterable (so the subclass of calendar have to always return an Array)

I would suggest we change as below

12.4.21 Temporal.Calendar.prototype.fields ( fields )

1. Let _calendar_ be the this value.
2. Perform ? RequireInternalSlot(_calendar_, [[InitializedTemporalCalendar]]).
3. Assert: _calendar_.[[Identifier]] is "iso8601".
- 4. Let _fieldsNames_ be ? IterableToListOfType(_fields_, « String »).
+ 4. Let _fieldsNames_ be ? CreateListFromArrayLike(_fields_, « String »).
+ 5.  For each element _name_ of _fieldsNames_, do
+  a. If _name_ is  not one of « *"day"*, *"hour"*, *"microsecond"*, *"millisecond"*, *"minute"*, *"month"*, *"monthCode"*, *"nanosecond"*, *"second"*, *"year"* », throw a RangeError exception.
6. Return ! CreateArrayFromList(_fieldsNames_).

15.6.2.20 Temporal.Calendar.prototype.fields ( fields )

1. Let _calendar_ be the this value.
2. Perform ? RequireInternalSlot(_calendar_, [[InitializedTemporalCalendar]]).
- 3. Let _fieldsNames_ be ? IterableToListOfType(_fields_, « String »).
+ 3. Let _fieldsNames_ be ? CreateListFromArrayLike(_fields_, « String »).
+ 4.  For each element _name_ of _fieldsNames_, do
+  a. If _name_ is  not one of « *"day"*, *"hour"*, *"microsecond"*, *"millisecond"*, *"minute"*, *"month"*, *"monthCode"*, *"nanosecond"*, *"second"*, *"year"* », throw a RangeError exception.
5. If calendar.[[Identifier]] is "iso8601", then
a. Let result be fieldNames.
6. Else,
a. Let result be the result of implementation-defined processing of fieldNames and the value of calendar.[[Identifier]].
7. Return ! CreateArrayFromList(result).

and

12.1.5 CalendarFields ( calendar, fieldNames )

1. Let _fields_ be ? GetMethod(_calendar_, "fields").
2. Let fieldsArray be ! CreateArrayFromList(_fieldNames_).
3. If _fields_ is not undefined, then
a. Set _fieldsArray_ to ? Call(_fields_, _calendar_, « _fieldsArray_ »).
- 4. Return ? IterableToListOfType(_fieldsArray_, « String »).
+ 4. Return ? CreateListFromArrayLike(_fieldsArray_, « String »).

If we make such change, we then can also remove the whole 13.1 IterableToListOfType ( items, elementTypes ) section since there are no other places in the spec use it.

[FrankYFTang]

@justingrant @sffc @ptomato @Ms2ger @ljharb @ryzokuken I would like to know why currently it is necessary for the Temporal.Calendar.prototype.fields to take iterable instead of arraylike as input and why CalendarFields need to take it's return as iterable, instead of as arraylike. Change both to arraylike will simplified the implementation.

[ljharb]

What new things in the language take an array/arraylike and not an iterable?

[FrankYFTang]

What new things in the language take an array/arraylike and not an iterable?

sorry @ljharb could you rewrite your question? not quit sure what you are asking.

[FrankYFTang]

I am not quit sure what @ljharb is really asking but if he is asking what in ECMA262 and ECMA402 is taking arry/arraylike now [I do not know what is the definition of "new things" and what is not "new thing" in TC39, sorry ], here is the list ECMA262 the argArray in 20.2.3.1 Function.prototype.apply ( thisArg, argArray ) https://tc39.es/ecma262/#sec-function.prototype.apply

argumentsList of 28.1.1 Reflect.apply ( target, thisArgument, argumentsList ) https://tc39.es/ecma262/#sec-reflect.apply

argumentsList of 28.1.2 Reflect.construct ( target, argumentsList [ , newTarget ] ) https://tc39.es/ecma262/#sec-reflect.construct

also... 22.1.2.4 String.raw ( template, ...substitutions ) https://tc39.es/ecma262/#sec-string.raw

in ECMA402 locales of 8.3.1 Intl.getCanonicalLocales ( locales ) https://tc39.es/ecma402/#sec-intl.getcanonicallocales

locales in all Intl.* (except Intl.Locale) construcctors, and their supportedLocalesOf method (Intl.Collator, Intl.DateTimeFormat, Intl.NumberFormat, Intl.DisplayNames, Intl.ListFormat, Intl.RelativeTimeFormat, Intl.PluralRules)

[FrankYFTang]

The situration here about Temporal.Calendar.prototype.fields, from my understanding , is very close to the kind of usage of the first three I listed in ECMA262 sicne this function is really used for argument negotiation between the Temporal.Plain* and Temporal.Calendar => it is also a close set, not an open set. If it is used correctly, it will only have those fields which the Temporal system know about, not any random string, or any thing a custom calendar know about (the output array may have, but not the input)

[ljharb]

Why is it more complex to take an iterable versus taking an arraylike? The spec steps are similar.

[FrankYFTang]

It is much more complext to implement a function which take an iterable than take an arraylike, at least in v8.

Also, I think it is an overkill usage especailly in this case when the argument is not intend to be any kind of string which should be put in with any kind of length (the current spec make it looks like that). It is a communication between Temporal.Plain* toward Temporal.Calendar and there are at most 10 different values could be put in for a reasonable usage of that. The current spec give the implementation of Temporal.Calendar unnecessary burden to fulfill the spec. For example, under the current spec, I will write a test to feed an iteratable which will generate 1 million "year" as input to Temporal.Calendar.prototype.fieldes and expect it will return an array with 1 millon "year" to fulfill the spec. If it return me 999,999 "year" or throw exception, it does not fulfill the spec, but this is not what a reasonable design should ask a functional Temporal.Calendar to implement, right?

The current definition of Temporal.Calendar.prototype.fields (for iso8601) currently ask the implementation to return an array of string for whatever the iterable will generate, which also create a hole for security attack.

In the other hand, if the input is require to be an array AND we also constraint the length of array can be at most what the Temporal.Plain* could send to this method (which is 10) and the possible values in this array, the implemention will then have no such burden and could throw if the test code pass in an array with length set to > 10 or contains any items which is out of the 10 known field value.

[FrankYFTang]

Consider the following code

let evilButCorrectInput = { cnt: 0};  
evilButCorrectInput[Symbol.iterator] = function *() { while ( this.cnt++  < 1000000) {yield "year";}};
let fieldsOut = Temporal.Calendar.from('iso8601').fields(evilButCorrectInput)

The correct implementation which follow the current spec is obligate to return an array of 1,000,000 "year" in fieldsOut. But why do we need to give such burden to the implementation of Temporal.Calendar for something is meaningless for real usage?

[FrankYFTang]

Notice I am not asking to change from iterable to arraylike, I am asking to change from iterable to arraylike w/ length and item restriction. If we simply change from iterable to arraylike such as let evilArrayInput = {length: 1000000, 0: 'year', 1: 'year', 1000000: 'year'} we may still suffer similar problem but if we place more restriction (length + item check) on arraylike, we won't.

Correction: it will throw right now because evilArrayInput[2] is undefined and the current spec will throw by step 4-b-2 of IterableToListOfType so evilArrayInput won't have the same problem as iterable.

[FrankYFTang]

From my point of view, both the fact that Temporal.Calendar.prototype.fields(fields) take iterable as input and ask the CalendarFields to take the return value of Temporal.Calendar.prototype.fields as iterable create unnecessary security vulnerability. Consider a subclass of Temporal.Calendar which will return an iteratable (instead of Array) which will generate 1,000,000,000 items (let's say all are 'year'), the current CalendarFields specification will take that input and call IterableToListOfType and turn that into a list of 1,000,000,000 items and consume a lot of memory easily. I think CalendarFields should consider some limitation on the return value of the fields() call

[ljharb]

How is that a security vulnerability? Something that takes a callback, where I pass it () => { while (true) {} }, isn't a vulnerability.

[ptomato]

This was done as requested by the editors' review for stage 3. (See https://github.com/tc39/proposal-temporal/issues/1427.) I guess for that reason the default resolution would be not to change anything, unless there is an overriding concern.

[ptomato]

(That said, personally, I think it'd be fine to implement the other restrictions on the input array mentioned in point number 2)

[FrankYFTang]

I strongly suggest we rollback that change. That change request is mentioned 3 days before March TC39 meeting and the spec before March TC39 was not reflecting the change. I do not believe this change is discussed / mentioend in the March TC39 meeting which agree to move Temporal to Stage 3. Therefore, from my point of view, what TC39 March agree to move to Stage 3 is the version with array as input and output, not the one with iteratble and this is a post Stage 3 changes.

@syg @sffc

[FrankYFTang]

() => { while (true) {}

won't force the function to allocate any additional memory in order to fulfill the specification, right? The issue is the current spec said

4. Let fieldNames be ? IterableToListOfType(fields, « String »).
5. Return ! CreateArrayFromList(fieldNames).

Which force the implementation of this specification to internally create array (which cause memory allocation)

[ptomato]

I strongly suggest we rollback that change. That change request is mentioned 3 days before March TC39 meeting and the spec before March TC39 was not reflecting the change. I do not believe this change is discussed / mentioend in the March TC39 meeting which agree to move Temporal to Stage 3. Therefore, from my point of view, what TC39 March agree to move to Stage 3 is the version with array as input and output, not the one with iteratble and this is a post Stage 3 changes.

@syg @sffc

That's incorrect. #1427 it is mentioned explicitly on this slide as one of the editors' requests which formed the conditional changes under which Temporal was allowed to move to Stage 3. So, it's exactly the opposite — the version without this change, with array as input, was not allowed to move to Stage 3.

[FrankYFTang]

This was done as requested by the editors' review for stage 3. (See #1427.) I guess for that reason the default resolution would be not to change anything, unless there is an overriding concern.

If you read the issue carefully, the reviewer didn't ASK to change, here is what he wrote

Consuming array-likes is kind of strange; why not iterables? #1427 There's a half-dozen calls to CreateListFromArrayLike. Is there a reason to prefer that instead of consuming an iterable?

@bakkot @littledan

I am here to provide the REASON here.

Also from https://github.com/tc39/proposal-temporal/issues/1427 I see no conclusion of that discussion. In other words, it is a QUESTION (not request) asked by a Stage 3 reviewer and no conclusion made before TC39 March meeting, right?

Is there a reason we cannot rollback to what it look like when the Stage 3 got passed in March TC39, in particular now I show you how difficult it could be to implement and all the problem it could cause?

I think both https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.fields https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.fields and https://tc39.es/proposal-temporal/#sec-temporal-calendarfields

are very badly defined.

[bakkot]

FWIW I'm fine with changing it back to only accepting Arrays. I didn't have a strong reason to prefer iterables, just a general principle that it should generally be the default.

This sort of implementation feedback is what Stage 3 is designed for, so I don't think there's a problem with changing the semantics (with consensus).

(I don't think I understand exactly what the objection to iterables is, so don't take this as an endorsement of reverting, just that I am not opposed to reverting if there's good reason to do so.)

[FrankYFTang]

@bakkot, thanks

From my point of view, the issue is not an iteratable vs array only issue (datatype), but instead, what it could take (content in the array or iterable) issue.

Since I am going to implement the ISO8601 calendar for v8 and all other kind of calendars supported by ECMA402 and ICU in v8, I have to ensure the implementation fulfill the spec text of

https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.fields https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.fields

My understanding is the real motivation for this fields() method is to take a collection of at most (as the Temporal spec now) 10 items (« "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year" ») the Temporal.Plain* could send to Calendar, without duplication, as all the placesed mentioned in the spec while calling CalendarFields , process it, and return a new collection of items, either the same as the input, or plus/minus some times, as long as the items in the enw collections could be handled by the Temporal.Calendar.prototype.(dateFromFields|monthDayFromFields|yearMonthFromFields) methods .

Therefore, there are no reason the input to Temporal.Calendar.prototype.fields() would need to take more than 10 items, and for what it would return there are no reason it will it will return a collection which is much more than 10 items. We know for some calendar, it may return "era" and "eraYear" in addition to the 10 items I listed above, and for some other calendar, it may return additional more, but it should not be collection of a long list.

I can totally see the desire for the fields() to take the input as a Set and allow it to also return a Set in additional to taking an ArrayLike and return an ArrayLike, but that is a very different requirement for taking an Iterable and require the CalendarField to take the return value as an Iterable. (Both arrayLike and Set has way to find out the size easily) I am totally fine if we accept both ArrayLike and Set as input. Both ArrayLike and Set we can easily check the size of the input (and return).

The problem is currently the spec mandate the Temporal.Calendar.prototype.fields, while it is "iso8601", to return what ever got put in, without any restriction. I think that is troublesome. Notice this is NOT a "garbage in gabarge out" requirement, but a "garbage in, need to duplicate my gabage out" requirement. I really believe Temporal.Calendar.prototype.fields(fields) should put more restriction on the input, not just the input type as Iterable or not, but what the content could be.

[FrankYFTang]

That's incorrect. #1427 it is mentioned explicitly on this slide as one of the editors' requests which formed the conditional changes under which Temporal was allowed to move to Stage 3.

The slide clearly said "Use Iterables instead of Lists" NOT "Use Iterables instead of Array" and the post Stage 3 changes landed in Apr 26 in https://github.com/tc39/proposal-temporal/commit/79fee277153a3698c63e7d1f9b99b0cf68458750

is not changing from "instead of Lists", but is changing from "instead of array".

[ljharb]

I think it would be very bizarre for any api to accept an array but not an iterable. That’s just not how the language works anymore.

[ptomato]

Frank, I'm not sure what to make of this thread. I agreed in my earlier comment that we could consider this, but this is Stage 3. The default should be to do nothing, unless there is an overriding reason. You've provided a reason, and I think it's a valid concern that we should address, but it looks like at least Jordan and myself don't agree that it's enough of an overriding reason to revert back to not accepting iterables. You haven't yet addressed Jordan's earlier comment about why this is a security vulnerability, for one thing.

I'll put it on the agenda for the next Temporal champions meeting, but it would help if you would address the comments concisely.

In other words, I'd say you've received a skeptical but not negative response to your request. Given that, I am really annoyed about your reaction of accusing the champions of sneaking a normative change into the proposal without consensus. I don't think this is a constructive way to engage with your fellow delegates.

[FrankYFTang]

Given that, I am really annoyed about your reaction of accusing the champions of sneaking a normative change into the proposal without consensus. I don't think this is a constructive way to engage with your fellow delegates.

I agree w/ you that my comment was non constructive and applogy to you and / or anyone else if I offended you. Please accept my sincere applogy. It was truely non construcive and I should not go into that direction. And it is pointless for me to talk about those for what I intend to express anyway.

I think there are really 8 different thing involved in this part fo the spec:

1. the data type of what the Temporal.Calendar.prototype.fields() accept (arraylike vs iterable)
2. the content of what the ISO8601 (since that is clearly spec out) calendar implementation of Temporal.Calendar.prototype.fields() should accept (currently, the only restriction is all have to be string, any string are acceptable)
3. the data type of what the Temporal.Calendar.prototype.fields() return (arraylike vs iterable) should return. Currently in both 12.4.21 and 15.6.2.20 it clearly said no matter which calendars (ISO8601 or others) it should return Array and Array only.
4. the data type of what the CalendarFields expect an implementation of Temporal.Calendar.prototype.fields() could return (arraylike vs iterable) , currently the spec said it should accept iterable even 12.4.21 and 15.6.2.20 will only return Array. I believe this open an opportunity for the subclassing calendar or other custom calendar a chance to return other type.
5. The content (regarless it is in Array or iterable) of what an ISO8601 Temporal.Calendar.prototype.fields() should return, currently, the spec said everything the caller passed in. It must be garbage in garbage out.
6. The content (regarless it is in Array or iterable) of what a calendar other than ISO8601 Temporal.Calendar.prototype.fields() should return, currently, 15.6.2.20 in the spec said calendar and implementation defined.
7. the content of what calendar other than ISO8601 calendar implementation of Temporal.Calendar.prototype.fields() MAY accept (currently, the only restriction is all have to be string, any string MAY be accepted by other calendars)
8. WHEN (from an observable point of view) can a calendar other than ISO8601 have the opportunity to throw in the fields() method in the calendar specific logic.

I have no issue with what currently spec out for item 6 so just want to list here to make it clear that is not what I am asking to change. I also have no issue with item 3 above. I am very happy the spec only mandate the Temporal.Calendar.prototype.fields() to RETURN array but not iterable.

My conern is about the combination of item 1, 2, 4, 5, 7, 8 all together. Notice item 2, 5, 7, and 8 are unrelated to the data type of the "container" (array vs iterable) . My conern is not only item 1 and 4. And also notice my concern about item 2 and 5 are only limited to ISO8601 calendar and are not concerning about how any other calendars should behave.

I understand the default in a Stage 3 spec is "do nothing" to the spec. But please also remember the default of a Stage 3 spec in TC39 is also "stay in Stage 3 forever and never go into Stage 4" unelss something else happen. I am pretty sure we all love to see this spec move into Stage 4. I have convinced my manager to let me budget most of my Q3 work to design and implement the whole Temporal spec into v8 (see my draft design doc so far in https://docs.google.com/document/d/1Huu2OUlmveBh4wjgx0D7ouC9O9vSdiZWaRK3OwkQZU0/edit ) and hopefully we can have a good prototype under a flag checked in by end of Q3. I hope my dedication of work would help one day to move Temporal into Stage 4. All my comments here are either 1) try to address my lack of knowledge of prior convesionation / decision, 2) seek understanding of bigger picture of design reason which is not observable from the spec text itself (simply due to the required spec writing style from TC39), or 3) hit design and implementation difficulty in term of memory usage, speed, unncessary complexity, etc.

I understand my comments might seems LATE to you since this spec is ALREADY in Stage 3 and you folks spend a very long time and hard work to reach this stage and wish nothing to change from now on, but if you look from the angle of what it might take for Temporal to reach Stage 4 , I believe my comment is still come in a very early stage in the "moving from Stage 3 to Stage 4" process.

Here is my detail concern about item 2, 5 and 7 above a. [about item 2 and 5 above] I think it is unreasonable to ask the implementation of ISO8601 calendar to accept any items other than « "day", "hour", "microsecond", "millisecond", "minute", "month", "monthCode", "nanosecond", "second", "year" » Notice the current spec mandate the implementaiton allocate memory and return. There are no reasone an ISO8601 calendar should accept, allocate memory, and repeat items excep the above 10 items. There are no use case for anything other than the above 10. b. [about item 2 and 5 above] it is also unreasonable to ask the implementation of ISO8601 calendar to accept and return any repeated items. For example, if the the iso8601 calendar fields(["year", "year"]) is called, it is unerasonable for this method to return ["year", "year"] since there are no use case for such behavior. c. [about item 7 and 8] I also belive it is unreasonable to mandate the calendar other than iso8601 to only throw AFTER all the iterable got converted into list. Notice in https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.fields the calendar specific block is AFTER the completion of IterableToListOfType, which mean the implementation for other calendar are not allow to throw (the order of throw) until the completion of IterableToListOfType- which is an observable behavior. This PREVENT other calendar to perform any early throwing for ill combination of fields in input and force the implementation to take all the input from iterable into list before it have the opportunity to throw. Lets say if I implement an new calendar system and decide to throw if the input has 1,000,000 repeated "year" in it, I won't be able to implementation one to fulfill the spec unless I read all the 1,000,000 items into list first because the calendar specific logic happen after the IterableToListOfType. If I throw before the completion of the IterableToListOfType, my implementation will violate the current spec. But this is an unreasonable burden for the implementaiton of calendar (ISO8601 or other kind).

[ljharb]

To be clear, effectively the only thing it takes to reach stage 4 is "engines and browsers ship it". Design changes that aren't motivated by implementability or scale concerns - ie, consistency - are not expected once a proposal has reached stage 3; at stage 3 it's supposed to be finished. The default, in other words, is that no changes are made, it gets shipped as-is, and then it becomes stage 4, whether just de facto or officially.

That said, bringing up these kinds of things as soon as they're discovered is useful - just wanted to clarify the process stuff.

[FrankYFTang]

Design changes that aren't motivated by implementability or scale concerns - ie, consistency - are not expected once a proposal has reached stage 3

Sure, from my piont of view, the design of a method (to be exact, the iso8601 calendar version of Temporal.Calendar.prototype.fields () ) which supposed to only deal with calendar operation but current spec text mandate the method to have the ability allocate an infinity number of items from the input iterable and allocate all of them into Array before return is a "scale concern" for a method which does not, conceptually, need to carry such kind of "scalability" in operation. The fact that 15.6.2.20 mandate all calendars have to exam every items in the input iterable (which could scale to 1,000000000000 items or more, for example) BEFORE it can throw is also a scalability concern in implementation.

The only reason I file this issue is because " implementability on scale concerns" - the current spec tex ton that method expect an "unnecessary scale" of input requirement on the implementation while the concept of what it intend ot provide it does not need it to provide so. This is surely a " implementability of scale concerns", it mandate the method to reserer more memory than it need to while the input came in with a "unresaonable hunge amount of gabarge information" (say repeat 1000000 "year") which won't help to address the normal operation. This is surely a "scalability" issue.

[FrankYFTang]

effectively the only thing it takes to reach stage 4 is "engines and browsers ship it".

Yes, and that only thing may not happen until someone (for example, me in v8) take action to do it, right? While you, spec champions, see 99% of the work happen prior to Stage 3, someone like me, who is going to implement the spec in v8, see 99% of the work happem between Stage 3 and 4. I am here try to articulate the implementation difficulity feedback to you.

[ljharb]

To clarify again, by scalability i meant solely “it must be exposed to a large number of users before this feedback could be discovered”

[bakkot]

For what it's worth, I don't interpret stage 3 that strictly. In my mind stage 3 is an appropriate time for implementations to raise concerns that they discover in the process of implementing it, even if those concerns are just about performance and not "it is not possible to implement".

[FrankYFTang]

“it must be exposed to a large number of users before this feedback could be discovered” Sorry, I have no idea what does this mean. Where is such definition came from?

Here is what I saw on https://tc39.es/process-document/

Stage 3: Candidate Acceptance Signifies: The solution is complete and no further work is possible without implementation experience, significant usage and external feedback. Post-Acceptance Changes Expected: Limited: only those deemed critical based on implementation experience

Stage 4: Finished Acceptance

• Test262 acceptance tests have been written for mainline usage scenarios, and merged
• Two compatible implementations which pass the acceptance tests Significant in-the-field experience with shipping implementations, such as that provided by two independent VMs
• A pull request has been sent to tc39/ecma262 with the integrated spec text
• All ECMAScript editors have signed off on the pull request Spec Quality: Final: All changes as a result of implementation experience are integrated

This spec is in Stage 3 and not yet Stage 4, so based on the document I quote, the word "scale" or "scalability" is not mentioned, therefore what Jordan specified is not based any written conseus in that document but only his personal interpretation (I am not disagreeing with his intepretation, but just stating the fact that word is not mentioend in the TC39 document nor with an official definition)

The TC39 document tell me, who is an engineering that is working on VM which struggle with "implementation experience": A. Stage 3 proposal acceptance is possible WITHOUT full implementation experience, significant usage and external feedback. (Of course there are usage and external feedback provided to champion but the "Acceptance Signifies:" of Stage 3 clearly stated that the proposal only need to as complete as possible under the condiction WITHOUT them. I am not saying Temporal proposal is without them, I am saying TC39 Stage 3 does not require them. It only require as complete as possible without implementation experience. This mean TC39 process realize it would be too much burden for champion to advance to Stage 3 with implementation experience. But it also mean any Stage 3 proposal may have design defects/issues discovered during Stage 3 after implementation experience, right?)

B. Stage 3 "Post-Acceptance Changes Expected: Limited: only those deemed critical based on implementation experience" also show very clearly that any CRITICAL issue BASED on " implementation experience" should be considered. It only say the issue is limited to CRITICAL and must be BASED on "implementation experience". It mention no word about "scale", "number of users" or "before ...could be discovered". The word "critical" is not defined in this docment and I am sure it is subject to personal interpretation. For me, I think this issue surely fit CRITICAL and BASED on " implementation experience" and should be at least minimun "considered"

What Jordan stated "stage 3 it's supposed to be finished" is NOT TRUE, because the TC39 doc clearly label Stage 3 as "Candidate" and Stage 4 as "Finished" in the first column of that table. Using the word "finished" to describe Stage 3 violate the definition of https://tc39.es/process-document/ and should be ignored.

What Jordan stated "the only thing it takes to reach stage 4 is "engines and browsers ship it". is ALSO not true based on the TC39 doc, the "Entrance Criteria" column in that doc clearly add 5 additional requirement:

* Test262 acceptance tests have been written for mainline usage scenarios, and merged
* Two compatible implementations which pass the acceptance tests
* Significant in-the-field experience with shipping implementations, such as that provided by two independent VMs
* A pull request has been sent to tc39/ecma262 with the integrated spec text
* All ECMAScript editors have signed off on the pull request

which is more than "engines and browsers ship it"

Let's say one year from now A) I write and submit a test262 test which call iso8601 Temporal.Calendar.prototype.fields() with a generator which generate 1,000,001 "year" and passed to the method, and verify it will return me an array with 1,000,001 "year" in it, purely based on the current spec, AND B) I implmement and ship Temporal in v8 but with the Temporal.Calendar.prototype.fields() throw when the input has more than 1,000,000 item in it, and therefore failed the test

Then in that condiction, the "Two compatible implementations which pass the acceptance tests" will not include v8 but you can only count on Mozilla, JSC and some other VM to do that, right?

Notice A) is perfectly reasonable to put into test262 because it test the method 100% according to the current spec and B) is also perfectly reasonable for me to do because no reasonable usage of that method would call this method with more than 10 items and accepting 1,000,000 is more than reasonable.

Is that you want to see 1 year from now? If yes, feel free to ignore this issue and I will ask you to code review A) tomorrow in test262 PR...

[FrankYFTang]

So... I would also make another request here- I would like to ask the proposal chamion to clarify what is the "mainline usage scenarios" of the method Temporal.Calendar.prototype.fields()
under both 12.4.21 Temporal.Calendar.prototype.fields ( fields ) https://tc39.es/proposal-temporal/#sec-temporal.calendar.prototype.fields and 15.6.2.20 Temporal.Calendar.prototype.fields ( fields ) https://tc39.es/proposal-temporal/#sup-temporal.calendar.prototype.fields

so far the only docment I can find beside the spec is in https://tc39.es/proposal-temporal/docs/calendar.html

"To interoperate with libraries or other code that you didn't write, then you should implement the id property and the fields(), mergeFields(), and toJSON() methods as well. "

"calendar.fields(fields: Iterable) : Iterable Parameters:

fields (array of strings, or other iterable yielding strings): A list of field names. Returns: a new list of field names.

This method does not need to be called directly except in specialized code. It is called indirectly when using the from() static methods and with() methods of Temporal.PlainDateTime, Temporal.PlainDate, Temporal.PlainMonthDay, Temporal.PlainYearMonth, and Temporal.ZonedDateTime, and a number of other methods.

Custom calendars should override this method if they accept fields in from() or with() other than the standard set of built-in calendar fields: year, month, monthCode, and day. The input array contains the field names that are necessary for a particular operation (for example, 'monthCode' and 'day' for Temporal.PlainMonthDay.prototype.with()). The method should make a copy of the array and add additional fields as needed."

So my undrstanding that the "mainline usage scenarios" of fields() is "called indirectly when using the from() static methods and with() methods of Temporal.PlainDateTime, Temporal.PlainDate, Temporal.PlainMonthDay, Temporal.PlainYearMonth, and Temporal.ZonedDateTime, and a number of other methods."

However, if only above is THE "mainline usage scenarios", then the current spec which accept iterable instead of array is surely a design feature OUT OF "mainline usage scenarios", right? because the only place inside Temporal is CalendarFields https://tc39.es/proposal-temporal/#sec-temporal-calendarfields, which is calling it with ALWAYS an Array with less or equal of 10 unique (not repeatable) pre-defined string and any input more than that is NOT in the "mainline usage scenarios", right?

[FrankYFTang]

Feel FREE to LGTM https://github.com/tc39/test262/pull/3036 if you really think the current spec is reasonable.

[ptomato]

Frank, I told you already last week that I agree the concern about garbage input should be addressed. I also told you that the issue is on the agenda for the next champions meeting. I don't understand what else you want from me at this point.

Submitting this passive-aggressive PR to test262 to make your point is really unconstructive. You are not only wasting your own time, you're wasting my time and anyone else who's following this thread, and now you've forced the test262 maintainers into this argument that they weren't involved with and are wasting their time as well.

I'm thoroughly puzzled about what you expect to gain from this course of action?

[FrankYFTang]

You replied me later saying the following

In other words, I'd say you've received a skeptical but not negative response to your request.

I agree with your assessment. This issue so far only receive " skeptical but not negative response"... so ... how can I make it less "skeptical"?

Therefore, I have to show people the possible harm if this issue got ignored and the champions decide to take "The default should be to do nothing, unless there is an overriding reason." path, right? How can I show how harmful it could be? Simply write a test to test against the current spec if it is not changed. What is a better way to demostrate that, if you have a better idea, please let me know. I cannot think about any more.

I created the test262 PR in order to reduce skeptical view of this issue. I do not understand why you call it a "passive-aggressive PR". It is a PR to test262 that faithfully test against the current Stage 3 spec text that Jordan believe could only be changed IF "solely “it must be exposed to a large number of users before this feedback could be discovered”" . Many times I saw you and Jordan reply me with the "default is not to change it" message. So... how can I show you it is wrong to "change nothing"?

I'm thoroughly puzzled about what you expect to gain from this course of action?

I would love to see either A) more people look at that test and jump up and said such test is harmful and expose a design flaw in the current Temporal spec text WHICH then will "be exposed to a large number of users before this feedback could be discovered” and fulfill what Jordan expect a necessary change need to be. OR B) in the other hand, if everyone is happy with that test then let's land that test and use to test all the implementation which will faithfully implment the current spec text.

A simple positive "this is a series issue raise by Frank and we will take a careful look at it and maybe change it" message will reduce my need to alarm more TC39 members to fulfill the “it must be exposed to a large number of users before this feedback could be discovered”" requirement Jordan asked. While I filed this issue, I do not feel the need to do so, but with all the "skeptical" and "defaul do nothing" response, you folks make me believe that is a necessary step to take to make such a change. Or am I wrong?

[FrankYFTang]

I don't understand what else you want from me at this point.

I have stated in https://github.com/tc39/proposal-temporal/issues/1610#issuecomment-878476655 In summary - clarify what is the "mainline usage scenarios" of the method Temporal.Calendar.prototype.fields()

I believe that is a reasonable request.

[FrankYFTang]

now you've forced the test262 maintainers into this argument that they weren't involved with and are wasting their time as well.

Notice I didn't mention any argument when I submit the PR to test262 and only ask test262 maintainer to see does that test faithfully test against the current Temporal spec which in Stage 3. @Ms2ger gave me construtive feedback of how to better write that test and jugglinmike think "Yup, it looks accurate to me! Thanks for writing it!" as a positive feedback for my submission. So now the test is merged into test262 to make test all the implementation will faithfully execute as the current spec which by default won't get changed. IF, and only IF, you folks believe the spec should be changed and change it, then I will help to change that test to what the new spec you folks come out with. Otherwise, this test will be there to make sure that part of Temporal got passed in March has good test coverage. Isn't that "constructive" and "positive" of your great work which got Stage 3 already?

[FrankYFTang]

BTW, take a careful look at the test I submitted (and already MERGED) into test262 https://github.com/tc39/test262/pull/3036/files It is written 100% according to the Spec you wrote (but I objected). In other word, I am writing the test according to the current spec text you submited in April , which I raise concern about and like to change. I am not writing the test according to what I would like to see but a test which respect and according to the result of your prior-work. IF that will offend you, then I do not understand how to write the test in a way to please you.

[ptomato]

A reminder that this issue is on the agenda for the next Temporal champions meeting, on Thursday July 22. For an invitation, please ping @sffc.

[ptomato]

Meeting, July 22: We decided to propose a normative change. Temporal.Calendar.prototype.fields() should throw if the iterable that it receives yields more than 10 items, yields any duplicate items, or yields any items that are not one of "year", "month", "monthCode", "day", "hour", "minute", "second", "millisecond", "microsecond", or "nanosecond".

What to do if the userland implementation of fields() returns garbage (such as an infinite iterable), and whether it is a security issue, is still an open question. @FrankYFTang, you mentioned checking with your security team, would you be able to do this for the next meeting?

[ljharb]

Seems like you don't need the "more than 10"; if duplicates are prohibited and the list of allowed values is explicit then that should cover it?

As for an infinite iterable, you'd stop iterating and throw once you received the first item that was either duplicated, or outside the set of allowed values, so it wouldn't be able to be infinite.

[ptomato]

You're right, "more than 10" is not necessary.

The open question about the infinite iterable applies to the function's return value, which may be returned from an implementation of fields() in user code; the decision from the meeting applies to how we treat the function's argument in the default implementation of fields() in the spec text.

[ptomato]

@FrankYFTang Did you get a chance to follow up with your security team about whether the return value of fields() would be a concern?

[ptomato]

As per the champions meeting of 2021-11-04, this can be closed now.