Various issues and bugs

[anba]

Some issues I found while prototyping Temporal for SM. Non-exhaustive list (I've omitted most "?" / "!" errors, spelling mistakes, etc.):

2.2.4 SystemDateTime ( [ temporalTimeZoneLike [ , calendarLike ] ] )
2.2.5 SystemZonedDateTime ( [ temporalTimeZoneLike [ , calendarLike ] ] )

• Absent arguments don't default to undefined.

3.3.19 Temporal.PlainDate.prototype.add ( temporalDurationLike [ , options ] )
3.3.20 Temporal.PlainDate.prototype.subtract ( temporalDurationLike [ , options ] )

• RejectDurationSign is a no-op after ToLimitedTemporalDuration. Either remove RejectDurationSign or change to Assert: ValidateTemporalDuration(...) is true.
• Order of calls to GetOptionsObject and CreateTemporalDuration not consistent.
  • add calls CreateTemporalDuration and then GetOptionsObject.
  • subtract calls GetOptionsObject and then CreateTemporalDuration.

3.5.2 ToTemporalDate ( item [ , options ] )

• ValidateISODate should either be removed or changed to assertion, because ParseTemporalDateString always returns a valid date via ParseISODateTime.
• RegulateISODate is a no-op on a valid date, so this call should be removed.

3.5.3 DifferenceISODate ( y1, m1, d1, y2, m2, d2, largestUnit )

• Is only called with one of "years", "months", "weeks", or "days", so the assertion can be more strict.
• Step 2 can then also be removed.
• Tautology in step 2.p in DifferenceISODate #1649 mid.[[Year]] is equal to mid.[[Year]] comparison it obviously incorrect.
• Too few arguments passed to AddISODate.
• Rewriting the loop in step 4.f as follows may be easier to understand:

Let year be smaller.[[Year]].
Repeat, while year < greater.[[Year]],
  Set days to days + !ISODaysInYear(year).
  Set year to year + 1.

3.5.6 RejectISODate ( year, month, day )

• RejectISODate has only two uses. These should inline RejectISODate, because explicitly calling ValidateISODate for error checking is more common throughout the spec.

4.3.10 Temporal.PlainTime.prototype.add ( temporalDurationLike )
4.3.11 Temporal.PlainTime.prototype.subtract ( temporalDurationLike )

• RejectDurationSign is a no-op after ToLimitedTemporalDuration. Either remove RejectDurationSign or change to Assert: ValidateTemporalDuration(...) is true.
• AddTime returns a balanced time, which makes RegulateTime a no-op. Either remove or change to change Assert: ValidateTime(...).
• CreateTemporalTime is infallible, so should be prefixed with "!" instead of "?".
  • ❌ OrdinaryCreateFromConstructor is fallible

4.3.18 Temporal.PlainTime.prototype.toZonedDateTime ( item )

• Should throw a TypeError when item is not an object, cf. Get calls.

4.3.19 Temporal.PlainTime.prototype.getISOFields ( )

• Missing this value check to be a PlainTime object.

4.5.2 ToTemporalTime ( item [ , overflow ] )

• Record returned from ParseTemporalTimeString doesn't have a [[Calendar]] field.
• ValidateTime check never throws, because ParseTemporalTimeString only returns valid times via ParseISODateTime.
• CreateTemporalTime is infallible, so should use "!" instead of "?".
  • ❌ OrdinaryCreateFromConstructor is fallible

4.5.3 ToPartialTime ( temporalTimeLike )

• Always called with temporalTimeLike being an Object, so step 1 should be changed to an assertion.

4.5.4 RegulateTime ( hour, minute, second, millisecond, microsecond, nanosecond, overflow )

• Returns an extra [[Day]] field, but no caller uses this field.

5.3.41 Temporal.PlainDateTime.prototype.getISOFields ( )

• Alphabetical order wrong, "isoMonth" comes after "isoMinute".

5.3.26 Temporal.PlainDateTime.prototype.add ( temporalDurationLike [ , options ] )
5.3.27 Temporal.PlainDateTime.prototype.subtract ( temporalDurationLike [ , options ] )

• RejectDurationSign is a no-op after ToLimitedTemporalDuration. Either remove RejectDurationSign or change to Assert: ValidateTemporalDuration(...) is true.

5.3.30 Temporal.PlainDateTime.prototype.round ( options )

• GetOptionsObject is a no-op after the type check for options, so the call should be removed.
  • ❌ not a no-op for primitive types except undefined

5.3.31 Temporal.PlainDateTime.prototype.equals ( other )

• Should call CompareISODateTime to avoid duplicating steps for date-time comparison.

5.5.1 GetEpochFromISOParts ( year, month, day, hour, minute, second, millisecond, microsecond, nanosecond )

• MakeDay uses a zero-indexed month value, i.e. month - 1 must be passed to it.
• MakeDate can return NaN for large year values, because MakeDay has wishy-washy definition when NaN can be returned for large values: (Review MakeDay/MakeDate/TimeFromYear callers for possibly out-of-range values #2315)

[...], but if this is not possible (because some argument is out of range), return NaN.

• This issue should only apply when GetEpochFromISOParts is called from ISODateTimeWithinLimits, so I think the best way to avoid this issue, is to avoid calling GetEpochFromISOParts in ISODateTimeWithinLimits and instead manually check for the date-time limits, i.e. the two dates "-271821-04-20" and "+275760-09-13".

5.5.2 ISODateTimeWithinLimits ( year, month, day, hour, minute, second, millisecond, microsecond, nanosecond )

• 24 hours is 8.64×10¹³ nanoseconds, not 8.64×10¹⁶.
• Returns three states "too early", "too late", and "in range", but all callers only compare against "in range". Should be changed to a simple boolean result.

5.5.4 ToTemporalDateTime ( item [ , options ] )

• InterpretTemporalDateTimeFields doesn't return a [[Calendar]] field, which makes ToOptionalTemporalCalendar(result.[[Calendar]]) invalid.
• ValidateISODateTime should either be removed or changed to assertion, because ParseTemporalDateTimeString always returns a valid date-time via ParseISODateTime.

5.5.5 ValidateISODateTime ( year, month, day, hour, minute, second, millisecond, microsecond, nanosecond )

• Should call ValidateISODate instead of duplicating the same steps.

5.5.10 AddDateTime ( year, month, day, hour, minute, second, millisecond, microsecond, nanosecond, calendar, years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, options )

• Too few arguments passed to CreateTemporalDuration.

5.5.9 CompareISODateTime ( y1, mon1, d1, h1, min1, s1, ms1, mus1, ns1, y2, mon2, d2, h2, min2, s2, ms2, mus2, ns2 )

• Should call CompareISODate and CompareTemporalTime to avoid repeating the same steps multiple times.

5.5.12 DifferenceISODateTime ( y1, mon1, d1, h1, min1, s1, ms1, mus1, ns1, y2, mon2, d2, h2, min2, s2, ms2, mus2, ns2, calendar, largestUnit [ , options ] )

• Missing calendar argument in CreateTemporalDate calls.
• LargerOfTwoTemporalDurationUnits misspelled as LargerOfTwoTemporalUnits.

5.5.11 RoundISODateTime ( year, month, day, hour, minute, second, millisecond, microsecond, nanosecond, increment, unit, roundingMode [ , dayLength ] )

• Absent dayLength parameter not correctly handled in call to RoundTime.

6.3.32 Temporal.ZonedDateTime.prototype.withPlainDate ( plainDateLike )

• Calls GetISO8601Calendar(), but doesn't use the returned calendar anywhere.

6.3.39 Temporal.ZonedDateTime.prototype.round ( options )

• GetOptionsObject is a no-op after the type check for options, so the call should be removed.
  • ❌ It throws for non-undefined primitive values
• Missing fallback argument in ToTemporalRoundingMode call.
• Must check dayLengthNs is non-zero, otherwise we later end up with a division through zero.
• Probably should also reject negative dayLengthNs values. (Normative: Disallow negative day lengths #2261)

6.5.4 TemporalZonedDateTimeToString ( zonedDateTime, precision, showCalendar, showTimeZone, showOffset [ , increment, unit, roundingMode ] )

• ToString(zonedDateTime.[[Calendar]]) is unconditionally executed whereas ToString(timeZone) is only conditionally executed. This should be harmonised. (Normative: Avoid ToString(calendar) when the calendar-id is unused #2269)

6.5.5 AddZonedDateTime ( epochNanoseconds, timeZone, calendar, years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, options )

• Multiple callers throughout the complete spec don't pass the options parameter.

6.5.6 DifferenceZonedDateTime ( ns1, ns2, timeZone, calendar, largestUnit [ , options ] )

• The options parameter may not be present, so it can't be passed unconditionally to DifferenceISODateTime().
• No caller does actually pass options to this operation, so it should probably be removed.
• Should use direct comparison instead of subtracting ns2 - ns1 and then testing for zero.

6.5.7 NanosecondsToDays ( nanoseconds, relativeTo )

• Missing a type check for relativeTo being an Object before checking for internal slots.
• modulo implies the result is always positive, cf. definition of the modulo operation.
• The [[DayLength]] field can be negative, but the only caller inspecting this field uses abs(r.[[DayLength]]). NanosecondsToDays should be changed to return a non-negative [[DayLength]].

6.3.35 Temporal.ZonedDateTime.prototype.add ( temporalDurationLike [ , options ] )
6.3.36 Temporal.ZonedDateTime.prototype.subtract ( temporalDurationLike [ , options ] )

• RejectDurationSign is a no-op after ToLimitedTemporalDuration. Either remove RejectDurationSign or change to Assert: ValidateTemporalDuration(...) is true.

6.3.50 Temporal.ZonedDateTime.prototype.toPlainYearMonth ( )
6.3.51 Temporal.ZonedDateTime.prototype.toPlainMonthDay ( )

• I don't understand the indirection through YearMonthFromFields resp. MonthDayFromFields when for example toPlainDate, toPlainTime, and toPlainDateTime can directly create the result object.
  • See comment below
• This happens in multiple places, maybe there should be a comment describing why this is necessary or alternatively this should be simplified. → Various editorial issues from Anba's feedback #2459

7.3.21 Temporal.Duration.prototype.total ( options )

• The options parameter should be mandatory instead of being coerced through GetOptionsObject(), because step 6 will otherwise always throw an error anyway.

7.3.21 Temporal.Duration.prototype.total ( options )

• If unit is "nanoseconds" case is missing.

7.5.2 ToTemporalDurationRecord ( temporalDurationLike )

• ToNumber() returns a Number value, but floor expects a mathematical value.
• If val is undefined, ToNumber(undefined) returns NaN, which means floor(NaN) ≠ NaN will evaluate to true when assuming Math.floor() semantics should be applied. So an absent value will always throw a RangeError.

7.5.8 CreateTemporalDuration ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds [ , newTarget ] )

• -0 should be normalised to +0, because currently, depending on which operations are used, -0 may or may not be normalised to +0.

7.5.11 BalanceDuration ( days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, largestUnit [ , relativeTo ] )

• Doesn't correctly handle the case when relativeTo is absent.
• Also missing a type check for relativeTo being an Object before checking for internal slots.
• Has an explicit nanoseconds < 0 check where other operations defer to Sign() and then apply an extra step to handle the is zero case. This should be normalised throughout the spec.

7.5.12 UnbalanceDurationRelative ( years, months, weeks, days, largestUnit [ , relativeTo ] )

• All callers pass relativeTo, so it shouldn't be an optional argument.
• Directly access [[Month]] from DurationObject instead of using Get(untilResult, "months"). Otherwise we can't be sure the value is a number.
• Probably "If any of years, months, and weeks are not zero" instead of "If any of years, months, and days are not zero", right?
• Should use value ≠ 0 instead of abs(value) > 0, because the former is more straightforward.

7.5.13 BalanceDurationRelative ( years, months, weeks, days, largestUnit, relativeTo )

• Directly access [[Month]] from DurationObject instead of using Get(untilResult, "months"). Otherwise we can't be sure the value is a number.
• CalendarDateAdd avoids repeatedly accessing the "dateAdd" property, but this still happens when CalendarDateAdd is called from MoveRelativeDate. (Normative: Perform a single "dateAdd" lookup for MoveRelativeDate #2267)
  • Either add dateAdd parameter to MoveRelativeDate which can then be passed to the CalendarDateAdd call in MoveRelativeDate.
  • Or alternatively accept that the "dateAdd" property is accessed multiple times.

7.5.14 AddDuration ( y1, mon1, w1, d1, h1, min1, s1, ms1, mus1, ns1, y2, mon2, w2, d2, h2, min2, s2, ms2, mus2, ns2 [ , relativeTo ] )

• relativeTo parameter is marked as optional, but all callers provide it. So it should be changed to non-optional.
• ValidateTemporalDuration misspelled as ValidateDuration.

7.5.18 RoundDuration ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, increment, unit, roundingMode [ , relativeTo ] )

• Calls MoveRelativeDate with PlainDate objects, but MoveRelativeDate expects its relativeTo parameter to be a PlainDateTimeObject.
• Perform ? RequireInternalSlot(relativeTo, [[InitializedTemporalDateTime]]) should be an assertion. If relativeTo isn't undefined, it is guaranteed to be either a ZonedDateTime object or a PlainDateTime object.
• CalendarDateAdd avoids repeatedly accessing the "dateAdd" property, but this still happens when CalendarDateAdd is called from MoveRelativeDate. (Normative: Perform a single "dateAdd" lookup for MoveRelativeDate #2267)
  • Either add dateAdd parameter to MoveRelativeDate which can then be passed to the CalendarDateAdd call in MoveRelativeDate.
  • Or alternatively accept that the "dateAdd" property is accessed multiple times.

7.5.21 TemporalDurationToString ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, precision )

• If precision is 0, the complete decimalPart is added to the result. This is clearly wrong.

8.3.7 Temporal.Instant.prototype.add ( temporalDurationLike )
8.3.8 Temporal.Instant.prototype.subtract ( temporalDurationLike )

• AddInstant is fallible, so should be prefixed with "?" instead of "!".
• RejectDurationSign is a no-op after ToLimitedTemporalDuration. Either remove RejectDurationSign or change to Assert: ValidateTemporalDuration(...) is true.

8.3.11 Temporal.Instant.prototype.round ( options )

• The options parameter should be mandatory instead of being coerced through GetOptionsObject(), because ToSmallestTemporalUnit will end up throwing anyway when options is undefined.
• Also compare to other round functions which always throw an error when options is absent.

8.5.5 ParseTemporalInstant ( isoString )

• result.[[TimeZoneOffsetString]] is never undefined, so step 4 should be an assertion.
• utc - offsetNanoseconds can result in an invalid instant, but all callers of ParseTemporalInstant expect the return value is a valid instant. That means there needs to be an explicit check that utc - offsetNanoseconds is a valid epoch nanoseconds value.

8.5.7 AddInstant ( epochNanoseconds, hours, minutes, seconds, milliseconds, microseconds, nanoseconds )

• Addition on mixed arguments (BigInts and Number values).

8.5.10 TemporalInstantToString ( instant, timeZone, precision )

• Should the time zone offset "+00:00" be normalised to "Z"?
  • ❌ No, see Should Absolute.prototype.toString(timeZone) have options to omit the offset or time zone? #741 (comment) and Instant.p.toString using {timeZone: 'UTC'} option emits +00:00 offset, not Z as expected #2057.
• As currently spec'ed, new Temporal.Instant(0n).toString() and new Temporal.Instant(0n).toString({timeZone: "UTC"}) give different results.
  • ❌ By design, see Should Absolute.prototype.toString(timeZone) have options to omit the offset or time zone? #741 (comment) and Instant.p.toString using {timeZone: 'UTC'} option emits +00:00 offset, not Z as expected #2057.

9.3.12 Temporal.PlainYearMonth.prototype.add ( temporalDurationLike [ , options ] )
9.3.13 Temporal.PlainYearMonth.prototype.subtract ( temporalDurationLike [ , options ] )

• RejectDurationSign is a no-op after ToLimitedTemporalDuration. Either remove RejectDurationSign or change to Assert: ValidateTemporalDuration(...) is true.
• CalendarDaysInMonth can return any value, but CreateTemporalDate expects an integer.

9.3.14 Temporal.PlainYearMonth.prototype.until ( other [ , options ] )
9.3.15 Temporal.PlainYearMonth.prototype.since ( other [ , options ] )

• DateFromFields() expects an options argument, but none is passed.
• CreateTemporalDuration called with too few arguments.

9.3.21 Temporal.PlainYearMonth.prototype.toPlainDate ( item )

• "with duplicates removed" is ambiguous, for example the result of ["a", "b", "a"] can be either ["a", "b"] or ["b", "a"]. (Editorial: Add an explicit operation for merging calendar field names #2294)

9.5.1 ToTemporalYearMonth ( item [ , options ] )

• ParseTemporalYearMonthString doesn't return a [[Calendar]] field.
• result.[[Day]] is undefined condition is never true, because ParseTemporalYearMonthString always returns [[Day]] is a number via ParseISODateTime.
• CreateTemporalYearMonth called with year = undefined, but must instead be an integer.
• Call ValidateISODate not necessary, because also performed in CreateTemporalYearMonth.
• Why is there an extra indirection through YearMonthFromFields when result.[[Day]] is defined?
  • ❌ See from and with must set canonical reference day (for PlainYearMonth) or reference year (for PlainMonthDay) #1316

9.5.7 CreateTemporalYearMonth ( isoYear, isoMonth, calendar, referenceISODay [ , newTarget ] )

• Prefer ISODateTimeWithinLimits to make sure referenceISODay can't make this is an invalid date-time value.
  • ❌ That wouldn't be correct, see comment below

10.1.1 Temporal.PlainMonthDay ( isoMonth, isoDay [ , calendarLike [ , referenceISOYear ] ] )

• Uses referenceISOYear is not given, but elsewhere in the spec undefined checks are preferred.
• This means new Temporal.PlainMonthDay(1, 1, undefined) and new Temporal.PlainMonthDay(1, 1, undefined, undefined) are currently spec'ed to return different results.

10.5.1 ToTemporalMonthDay ( item [ , options ] )

• ParseTemporalMonthDayString doesn't return a [[Calendar]] field.
• CreateTemporalMonthDay called with year = undefined, but must instead be an integer.
• Call ValidateISODate not necessary, because also performed in CreateTemporalMonthDay.
• result.[[Year]] not passed to CreateTemporalMonthDay. → Typo in ToTemporalMonthDay #2457
• CreateTemporalMonthDay called with the wrong argument order.
• Why is there an extra indirection through MonthDayFromFields when result.[[Year]] is defined?
  • ❌ See from and with must set canonical reference day (for PlainYearMonth) or reference year (for PlainMonthDay) #1316

10.5.2 CreateTemporalMonthDay ( isoMonth, isoDay, calendar, referenceISOYear [ , newTarget ] )

• Should call ISODateTimeWithinLimits to ensure the year has at most six digits, because other parts of the spec assume this is always the case. (Normative: Disallow arbitrary integers for the reference ISO year in PlainMonthDay #2266)

10.3.12 Temporal.PlainMonthDay.prototype.toPlainDate ( item )

• options type check in step 6 should be moved to an earlier step for consistency with other methods in the proposal.
• "with duplicates removed" is ambiguous, for example the result of ["a", "b", "a"] can be either ["a", "b"] or ["b", "a"]. (Editorial: Add an explicit operation for merging calendar field names #2294)

11.1.1 IsValidTimeZoneName ( timeZone )

• Uses full Unicode Default Case Conversion, whereas ECMA-402 only handles ASCII characters.
• Using full Unicode Default Case Conversion only works until Unicode any non-ASCII characters whose upper-case form is one of "U", "T", or "C", cf. U+0131 and U+017F for related cases.
• There should either be a note that we don't expect any non-ASCII character to upper-case map to one of "U", "T", or "C". Or alternatively the operation should be rewritten to avoid this issue.

11.4.6 Temporal.TimeZone.prototype.getPlainDateTimeFor ( instant [ , calendarLike ] )

• Was the RequireInternalSlot call intentionally omitted? If yes, can we at least require that the this value is an object, because as currently spec'ed, it's necessary that both BuiltinTimeZoneGetPlainDateTimeFor and GetOffsetNanosecondsFor accept arbitrary (non-Object!) values.

11.4.9 Temporal.TimeZone.prototype.getNextTransition ( startingPoint )
11.4.10 Temporal.TimeZone.prototype.getPreviousTransition ( startingPoint )

• The next/previous transition may be an invalid instant, but CreateTemporalInstant only accepts valid epoch nanoseconds. That means there needs to be an explicit RejectInstant call before CreateTemporalInstant. (Normative: Check for invalid epoch nanoseconds in Temporal.TimeZone.prototype.get{Next,Previous}Transition #2311)

11.6.1 ParseTemporalTimeZone ( string )

• Record returned from ParseTemporalTimeZoneString has a [[Name]] field, here incorrectly accessed as [[TimeZoneName]].
• Doesn't handle offset-only time zones.

11.6.2 CreateTemporalTimeZone ( identifier [ , newTarget ] )

• CanonicalizeTimeZoneName(identifier) is incorrect for offset-only time zones.

11.6.6 GetIANATimeZoneNextTransition ( epochNanoseconds, timeZoneIdentifier )
11.6.7 GetIANATimeZonePreviousTransition ( epochNanoseconds, timeZoneIdentifier )

• Operation preamble should note that null is a valid return value.

11.6.8 ParseTimeZoneOffsetString ( offsetString )

• Neither hours or sign will ever be undefined, so step 4 should be removed resp. changed into an assertion.

11.6.9 FormatTimeZoneOffsetString ( offsetNanoseconds )

• floor() called on negative values, but floor() rounds towards +∞. So floor(4.3) = 4, but floor(-4.3) = -5. Is this intended here?
• post isn't always defined, but unconditionally consumed in the return step.

11.6.11 GetOffsetNanosecondsFor ( timeZone, instant )

• Use 8.64×10¹³ instead of 86400×10⁹ for consistency with the rest of the spec.
• Change ">" to "≥" to disallow full-day time zone offsets. (Normative: Disallow one day long time zone offsets #2260)

11.6.12 BuiltinTimeZoneGetOffsetStringFor ( timeZone, instant )
11.6.13 BuiltinTimeZoneGetPlainDateTimeFor ( timeZone, instant, calendar )
11.6.14 BuiltinTimeZoneGetInstantFor ( timeZone, dateTime, disambiguation )

• Are all prefixed with "BuiltinTimeZone", but actually allow any object. Maybe drop "Builtin" from the name? → Various editorial issues from Anba's feedback #2459

11.6.14 BuiltinTimeZoneGetInstantFor ( timeZone, dateTime, disambiguation )

• AddDateTime incorrectly called: wrong number of arguments and options not passed as an Object.
• AddDateTime returns a record, but GetPossibleInstantsFor expects an object.

12.1.2 IsBuiltinCalendar ( id )

• Should calendar ids be case-normalised? For example time zones names are case-normalised and UTS-35 also ignores casing. (Case-normalisation and/or canonicalisation for Calendar identifiers? #2314)

12.1.3 GetBuiltinCalendar ( id )

• Should directly call CreateTemporalCalendar.

12.1.6 CalendarMergeFields ( calendar, fields, additionalFields )

• Return value must be validated to be an Object, because all callers expect an Object value.

12.1.13 CalendarDayOfWeek ( calendar, dateLike )
12.1.14 CalendarDayOfYear ( calendar, dateLike )
12.1.15 CalendarWeekOfYear ( calendar, dateLike )
12.1.16 CalendarDaysInWeek ( calendar, dateLike )
12.1.17 CalendarDaysInMonth ( calendar, dateLike )
12.1.18 CalendarDaysInYear ( calendar, dateLike )
12.1.19 CalendarMonthsInYear ( calendar, dateLike )
12.1.20 CalendarInLeapYear ( calendar, dateLike )

• Abstract operation preamble mentions the result is validated, but no actual validation takes place. (Normative: Validate results of Calendar operations #2265)

12.1.25 YearMonthFromFields ( calendar, fields, options )
12.1.26 MonthDayFromFields ( calendar, fields, options )

• Not all callers pass the options argument to these operations.

12.1.21 ToTemporalCalendar ( temporalCalendarLike )

• ParseTemporalCalendarString can return a non-supported calendar string, but CreateTemporalCalendar asserts the input is a supported, built-in calendar string.

12.1.37 ResolveISOMonth ( fields )

• the substring of monthCode from 1 instead of the substring of monthCode from 2.

12.1.40 ISOMonthDayFromFields ( fields, options )

• Let result be undefined. is unnecessary and therefore should be removed.
• Why is year not validated when monthCode is undefined in steps 13-14?
  • ❌ It is validated by PrepareTemporalFields in step 2
• The returned record ignores any explicitly provided year value.
  • ❌ By design

12.1.45 ISOMergeFields ( fields, additionalFields )

• If newKeys does not contain either "month" or "monthCode" is wrong, for example new Temporal.PlainMonthDay(1,1).with({day:2}) currently always throws. It should check if the result contains these keys:

Let mergedKeys be ? EnumerableOwnPropertyNames(merged, key).
If mergedKeys contains neither "month" nor "monthCode", then
   ...

12.4.19 Temporal.Calendar.prototype.monthsInYear ( dateOrDateTime )
12.4.20 Temporal.Calendar.prototype.inLeapYear ( dateOrDateTime )

• Parameter names (in header and preamble) weren't updated to temporalDateLike.

13.7 DurationHandleFractions ( fHours, minutes, fMinutes, seconds, fSeconds, milliseconds, fMilliseconds, microseconds, fMicroseconds, nanoseconds, fNanoseconds )

• floor() called on negative values, but floor() rounds towards +∞. So floor(4.3) = 4, but floor(-4.3) = -5. Is this intended here?

13.16 ToTemporalRoundingIncrement ( normalizedOptions, dividend, inclusive )

• Unlimited increment for "hour", "month", "week", and "day" has some issues:

1. When +∞ ends up in RoundNumberToIncrement, the mathematical expression x / increment is not defined, because +∞ isn't a mathematical number.
2. Too large increments don't seem useful for the user, because the other input is always restricted to a certain range. For example the number of days between two dates is restricted to 2×10⁸ days by definition. When the rounding increment is then set to, let's say 10⁹, the rounding result will always either be 0 or 10⁹, depending on the rounding mode. (Or in other cases TypeErrors, because some intermediate results can't be represented.)
3. And unlimited increments make harder to implement this, because it's not possible to use fixed size integer types.

Therefore I'd like to propose to limit the maximum rounding increment for "hour", "month", "week", and "day" to reasonable limits.

• → Proposal to limit rounding increments for hour, month, week, and day #2458

13.24 ToTemporalDurationTotalUnit ( normalizedOptions )

• Should directly throw a RangeError when the GetOption result is undefined instead of requiring the callers to perform this check.

13.26 ToRelativeTemporalObject ( options )

• Calls ParseISODateTime on an arbitrary string, which gives bogus results. Should this be ParseTemporalZonedDateTimeString?

13.39 ISO 8601 grammar

• CalendarDate production is unused.
• The number of ":" for productions rules should be checked with the definitions in ECMA-262, cf. where ECMA-262 uses "::" and ":::". → ISO 8601 grammar uses incorrect separating punctuation #2190
• Not all valid IANA time zone names are allowed, eg. "PST8PDT" or "GMT0". (Normative: Support IANA legacy names #2292)

13.41 ParseTemporalInstantString ( isoString )

• Returns [[TimeZoneZ]] and [[TimeZoneName]], but no caller uses these fields.

13.42 ParseTemporalZonedDateTimeString ( isoString )

• isoString must be parseable as both TemporalZonedDateTimeString and TemporalTimeZoneString. Is this really intended?
• If that's really the case, the TemporalZonedDateTimeString production should be changed accordingly:

TemporalZonedDateTimeString ∩ TemporalInstantString :
  Date TimeZoneNumericUTCOffset TimeZoneBracketedAnnotation Calendar_opt
  Date DateTimeSeparator TimeSpec TimeZoneNumericUTCOffset TimeZoneBracketedAnnotation Calendar_opt

• timeZoneResult.[[Z]] is always undefined, because UTCDesignator isn't allowed in TemporalZonedDateTimeString anyway.
• Also: When called from ToTemporalZonedDateTime, ToTemporalZonedDateTime handles the case when no time zone name is present, but that can't actually happen, cf. the intersection production TemporalZonedDateTimeString ∩ TemporalInstantString

13.46 ParseTemporalDurationString ( isoString )

• DurationHandleFractions called with too few arguments.

13.52 PrepareTemporalFields ( fields, fieldNames, requiredFields )

• Table 13 contains "hour" two times.

13.53 PreparePartialTemporalFields ( fields, fieldNames )

• Always called with fields being an Object, so step 1 should be changed to an assertion.

[ptomato]

Thanks, this is incredibly comprehensive and helpful.

Some of these are probably already covered by #1411, #1413, #1424, and #1425, so I'll work on those first. Then what's left I'll separate into editorial and normative changes, fix the editorial ones, and prepare a consensus PR for the normative ones.

[justingrant]

@anba - Really impressive feedback. Thanks for this!

[wirthi]

5.5.11 DifferenceISODateTime

• Calls BalanceDuration(years, months, weeks, ...) while that function accepts (days, hours, minutes, ...)

[Ms2ger]

Hi @wirthi, please file a new issue.

[wirthi]

If you prefer I can do that.

I thought it would be a good idea to collect the issues here in one place, doesn't that make it easier for the editors as well? I ran into many of the same issues that @anba reported as well, so I just wanted to amend the list here.

[Ms2ger]

I prefer that, that's why I asked.

[Ms2ger]

ParseTemporalInstant ( isoString )
result.[[TimeZoneOffsetString]] is never undefined, so step 4 should be an assertion.

Can the "hours is undefined" branch not happen in ParseTemporalTimeZoneString in this case?

[anba]

ParseTemporalInstant calls ParseTemporalInstantString and ParseTemporalInstantString checks the input matches TemporalInstantString. TemporalInstantString has TimeZoneOffsetRequired and therefore the "hours is undefined" branch can't happen.

[ptomato]

Verify that everything from the checklist is resolved, open separate issues for anything that's left.

[ptomato]

6.3.50 Temporal.ZonedDateTime.prototype.toPlainYearMonth ( )
6.3.51 Temporal.ZonedDateTime.prototype.toPlainMonthDay ( )
I don't understand the indirection through YearMonthFromFields resp. MonthDayFromFields when for example toPlainDate, toPlainTime, and toPlainDateTime can directly create the result object. This happens in multiple places, maybe there should be a comment describing why this is necessary or alternatively this should be simplified.

It's not possible to directly create a PlainYearMonth or PlainMonthDay given a full date in a non-ISO calendar. To create it directly, you need the ISO date of the first day of the PlainYearMonth, or the ISO date of the PlainMonthDay in the reference year, respectively. Only the calendar has this knowledge. (For example, today's date 2023-01-05 in the Hebrew calendar is 12 Tevet 5783. If you want the PlainYearMonth of Tevet 5783, you need to create it with a reference ISO date of 2022-12-25. This is accomplished by the CalendarFields / CalendarYearMonthFromFields calls in toPlainYearMonth, where fields is something like { year: 5783, monthCode: 'M04' }. ) I've added clarifying notes about this in #2459.

8.5.10 TemporalInstantToString ( instant, timeZone, precision )
Should the time zone offset "+00:00" be normalised to "Z"?
As currently spec'ed, new Temporal.Instant(0n).toString() and new Temporal.Instant(0n).toString({timeZone: "UTC"}) give different results.

This is by design, see #741 (comment) and #2057.

9.5.1 ToTemporalYearMonth ( item [ , options ] )
Why is there an extra indirection through YearMonthFromFields when result.[[Day]] is defined?

Similar to the question about toPlainYearMonth/toPlainMonthDay, this is so that the reference ISO date is set consistently. result.[[Day]] is user input and may not be the correct reference value. See #1316. (There are already clarifying notes about this step in the spec text, so I think no further action is needed here.)

9.5.7 CreateTemporalYearMonth ( isoYear, isoMonth, calendar, referenceISODay [ , newTarget ] )
Prefer ISODateTimeWithinLimits to make sure referenceISODay can't make this is an invalid date-time value.

That wouldn't be correct, because PlainYearMonth internal slots must be allowed to contain a reference date that's outside the limits for ISODateTimeWithinLimits. e.g., Temporal.Instant.fromEpochSeconds(-86400_00000000).toZonedDateTimeISO('UTC').toPlainYearMonth() validly contains the ISO reference date -271821-04-01 which is outside the limits for PlainDate/PlainDateTime.

10.5.1 ToTemporalMonthDay ( item [ , options ] )
result.[[Year]] not passed to CreateTemporalMonthDay.

I believe this is a typo! I opened a new issue for it: #2457

Why is there an extra indirection through MonthDayFromFields when result.[[Year]] is defined?

Same as above, see #1316.

11.6.12 BuiltinTimeZoneGetOffsetStringFor ( timeZone, instant )
11.6.13 BuiltinTimeZoneGetPlainDateTimeFor ( timeZone, instant, calendar )
11.6.14 BuiltinTimeZoneGetInstantFor ( timeZone, dateTime, disambiguation )
Are all prefixed with "BuiltinTimeZone", but actually allow any object. Maybe drop "Builtin" from the name?

Sure, done in #2459.

12.1.40 ISOMonthDayFromFields ( fields, options )
Why is year not validated when monthCode is undefined in steps 13-14?

year is validated by the call to PrepareTemporalFields in step 2.

The returned record ignores any explicitly provided year value.

That's by design, the returned record should have the canonical reference year for the ISO calendar of 1972.

12.1.45 ISOMergeFields ( fields, additionalFields )
If newKeys does not contain either "month" or "monthCode" is wrong, for example new Temporal.PlainMonthDay(1,1).with({day:2}) currently always throws. It should check if the result contains these keys:

Let mergedKeys be ? EnumerableOwnPropertyNames(merged, key).
If mergedKeys contains neither "month" nor "monthCode", then
  ...

I believe this has been fixed in the current DefaultMergeCalendarFields. new Temporal.PlainMonthDay(1, 1).with({day: 2}) would result in a call to DefaultMergeCalendarFields with fields { day: 1, month: 1, monthCode: "M01", year: 1972 } and additionalFields { day: 2 }, and would return the correct fields { day: 2, month: 1, monthCode: "M01", year: 1972 }.

13.16 ToTemporalRoundingIncrement ( normalizedOptions, dividend, inclusive )
Unlimited increment for "hour", "month", "week", and "day" has some issues:

1. When +∞ ends up in RoundNumberToIncrement, the mathematical expression x / increment is not defined, because +∞ isn't a mathematical number.
2. Too large increments don't seem useful for the user, because the other input is always restricted to a certain range. For example the number of days between two dates is restricted to 2×10⁸ days by definition. When the rounding increment is then set to, let's say 10⁹, the rounding result will always either be 0 or 10⁹, depending on the rounding mode. (Or in other cases TypeErrors, because some intermediate results can't be represented.)
3. And unlimited increments make harder to implement this, because it's not possible to use fixed size integer types.
   Therefore I'd like to propose to limit the maximum rounding increment for "hour", "month", "week", and "day" to reasonable limits.

(1) is solved, it's no longer possible to create infinite rounding increments. I've opened an issue to discuss the other two points: #2458

13.39 ISO 8601 grammar
The number of ":" for productions rules should be checked with the definitions in ECMA-262, cf. where ECMA-262 uses "::" and ":::".

Duplicate of #2190.

[ptomato]

That leaves everything in the checklist either addressed, or with a separate issue open; I'll close this.

[justingrant]

The returned record ignores any explicitly provided year value.

That's by design, the returned record should have the canonical reference year for the ISO calendar of 1972.

Some context: all calendars (not just ISO) canonicalize the reference year in from, so that for any given { month, day } pair, the same reference year should be chosen.

This is helpful because it makes equals work regardless of which year was provided to from. Example:

d1 = Temporal.PlainMonthDay.from('2000-01-01[u-ca=chinese]')
  // => 1972-01-11[u-ca=chinese
anotherYear = d1.toPlainDate({year: 2001}).toString()
  // => '2002-01-08[u-ca=chinese]'
d2 = Temporal.PlainMonthDay.from(anotherYear)
  // => 1972-01-11[u-ca=chinese]
d2.equals(d1)
  // => true

Note that the canonical year is not always 1972 for some dates and non-ISO calendars, because for some calendars leap days or (for lunisolar calendars) leap months don't exist in 1972. It's not specified how the canonical year should be chosen, only that it must be deterministic based on the month/day inputs. In the polyfill, we start at 1972 and walk backwards one year at a time until we find a year where that month/day exists in that calendar.

PlainYearMonth works the same way, but it's easier because the ReferenceDay is always canonicalized to 1.

@ptomato - The relevant PR is #1331 that outlines the requirements above for ISO, but did we ever add the corresponding non-ISO requirements to either the Temporal spec or the 402 spec? I don't remember where the responsibility for specifying non-ISO calendar behavior ended up.

[ptomato]

@ptomato - The relevant PR is #1331 that outlines the requirements above for ISO, but did we ever add the corresponding non-ISO requirements to either the Temporal spec or the 402 spec? I don't remember where the responsibility for specifying non-ISO calendar behavior ended up.

Good catch. In fact, this is not specified in Temporal.Calendar.prototype.yearMonthFromFields and Temporal.Calendar.prototype.monthDayFromFields. It's a bit unwieldy that both of these go through CalendarDateToISO whereas what we really need to specify is that yearMonthFromFields returns an ISO date corresponding to the 1st of the calendar month, and monthDayFromFields picks a reference year. I've opened #2461 for this.