Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Where should month without year or monthCode be rejected? #2664

Closed
gibson042 opened this issue Aug 23, 2023 · 15 comments · Fixed by #2663
Closed

Where should month without year or monthCode be rejected? #2664

gibson042 opened this issue Aug 23, 2023 · 15 comments · Fixed by #2663

Comments

@gibson042
Copy link
Collaborator

gibson042 commented Aug 23, 2023

(derived from #2663 (comment) )

(EDITED by @sffc on 2023-09-15)

  1. Accept fully-specified input.
    1. ✔︎ Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1, calendar: "iso8601" })
    2. ✔︎ Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1, calendar: "hebrew" })
    3. ✔︎ Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1, calendar: "gregory" })
  2. Accept absent calendar as implicitly ISO 8601.
    1. ✔︎ Temporal.PlainMonthDay.from({ monthCode: "M08", day: 1 })
    2. ✔︎ Temporal.PlainMonthDay.from({ month: 8, day: 1 })
    3. ✔︎ Temporal.PlainMonthDay.from("08-01")
  3. Explicit string calendar (reject to avoid data-driven exceptions?)
    1. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: "iso8601" })
    2. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: "hebrew" }) (must reject for ambiguity)
    3. Temporal.PlainMonthDay.from("08-01[u-ca=iso8601]")
    4. Temporal.PlainMonthDay.from("08-01[u-ca=hebrew]") (must reject for ambiguity)
    5. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: "gregory" })
    6. Temporal.PlainMonthDay.from("08-01[u-ca=gregory]")
  4. Explicit object calendar (reject to avoid data-driven exceptions?)
    1. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: Temporal.Calendar.from("iso8601") })
    2. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: Temporal.Calendar.from("hebrew") }) (must reject for ambiguity)
    3. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: customCalendar })
    4. Temporal.PlainMonthDay.from({ month: 8, day: 1, calendar: Temporal.Calendar.from("gregory") })
  5. monthDayFromFields with a specific valid calendar instance receiver
    1. Temporal.Calendar.from("iso8601").monthDayFromFields({ month: 8, day: 1 })
    2. Temporal.Calendar.from("hebrew").monthDayFromFields({ month: 8, day: 1 }) (must reject for ambiguity)
    3. Temporal.Calendar.from("gregory").monthDayFromFields({ month: 8, day: 1 })

I believe the only potentially variant points are 3.i, 3.iii, 4.i, 4.iii, and 5.i. The current spec as of #2500 is to reject 3.iii but accept the rest, and the previous state was to reject all but 4.iii. I feel strongly that 5.i should be accepted because of 2, and therefore that 4.i and 4.iii should be accepted as well. If that has agreement, then the only remaining questions are 3.i and 3.iii, where 3.iii has been rejected for a long time and is not in contention AFAIK. So I'll start a list of two options and anticipate potential growth:

A: Reject 3.i. calendar: string always requires month to be qualified by year and/or monthCode (inconsistent between calendar: "iso8601" vs. calendar: Temporal.Calendar.from("iso8601") but otherwise similar to pre-#2500 and in particular always rejects { month, day, calendar: str } but delegates { month, day, calendar: obj } to the calendar).

B: Accept 3.i. ISO 8601 calendar always resolves month to monthCode (current state; consistent between string vs. object ISO 8601 calendar but rejection of { month, day, calendar: str } is dependent upon the value of calendar rather than the total input shape).

@gibson042
Copy link
Collaborator Author

gibson042 commented Aug 23, 2023

I personally can live with A or B, which are both easy to explain but prioritize different aspects of behavior.

But my preference is B, because A encourages either

  • more construction of built-in Calendar instances where a string would be preferable, e.g.
     // The "iso8601" calendar accepts month and day without month code.
    -Temporal.PlainMonthDay.from({ month, day, calendar: String(calendarId) })
    +// ...but only as an object rather than a string.
    +Temporal.PlainMonthDay.from({ month, day, calendar: Temporal.Calendar.from(String(calendarId)) })
    or
  • extra code that is difficult to get right, e.g.
    const calendarId = typeof calendar === "string"
      ? calendar.toLowerCase()
      : undefined;
    const monthDay = Temporal.PlainMonthDay.from(
      // We need special treatment for the ISO 8601 calendar
      // because { month, day, calendar: string } always throws :(.
      calendarId === "iso8601"
        ? { month, day }
        : { month, day, calendar },
    );

@justingrant
Copy link
Collaborator

justingrant commented Aug 25, 2023

@sffc do you want to weigh in here too?

My take:

  • The PlainMonthDay.from cases (3) and (4) vary only by the type of the calendar property of the input bag, so I'd be inclined to treat them identically. Strings vs. objects are just an optimization and so shouldn't behave differently.
  • For (3) and (4), my opinion is that the simplest explanation to developers (and the most consistent with our "avoiding data driven exceptions" goal) is: in a yearless PlainMonthDay.from, any non-undefined calendar property is rejected: strings or objects, ISO or not, custom or not. So all the ❓ cases in (3) and (4) would throw.
  • re: Calendar#monthDayFromFields (5), this is an advanced API that will likely only be called directly by polyfill writers and custom calendar authors. Whether monthDayFromFields is consistent with (2), (3), and (4) doesn't seem that important, because the tiny set of userland developers who'll call monthDayFromFields directly are advanced enough to adapt to its behavior however we specify it. So my weakly-held opinion is that, assuming (3) and (4) cases all throw, that we should make the (5i) behavior whatever is easiest to specify & implement OR avoids churn in the spec.

A consequence of my opinions above is that users of a custom calendar can never do what the ISO calendar does, which is allow creating a PlainMonthDay without including the year in the input. IMO this is OK, because it seems like a tiny set (maybe the null set?) of custom calendars that will be able to deterministically map all yearless month/day pairs to a specific ISO date.

One process note: whatever we do here, it's not a new normative change that we need to take to plenary. Rather, I see it as a bug fix to #2500 which was previously approved.

@sffc
Copy link
Collaborator

sffc commented Aug 25, 2023

I'm a little iffy about 3 and 4 having different behavior (built-in calendar ID string vs calendar object) but I can see the argument both ways.

So my preference would be to say that in Temporal.PlainMonthDay.from, any form of an explicit calendar (ISO string, built-in calendar ID, or calendar object) must have a year or monthCode, and that is where the common RangeError gate lives, and monthDayFromFields has calendar-dependent behavior, succeeding where possible. That would mean rejecting 3.i, 3.iii, 4.i, and 4.iii.

@justingrant
Copy link
Collaborator

I'm fine with Shane's recommendation above.

@ptomato
Copy link
Collaborator

ptomato commented Aug 26, 2023

I agree we should fix the consistency bug that slipped into #2500. I also agree that custom calendars don't need to have the capability to accept yearless month/day like the ISO 8601 calendar because nobody should need to polyfill the ISO 8601 calendar as it is fully specified. But I've stared at this for a while, and I can't really seem to muster any strong opinion about whether the principle of avoiding data-driven exceptions weighs stronger here, or the congruence of Temporal.PlainMonthDay.from and Temporal.Calendar.prototype.monthDayFromFields.

@justingrant
Copy link
Collaborator

@gibson042, would you be OK to accept @sffc's proposal above? (excerpting below)

in Temporal.PlainMonthDay.from, any form of an explicit calendar (ISO string, built-in calendar ID, or calendar object) must have a year or monthCode, and that is where the common RangeError gate lives, and monthDayFromFields has calendar-dependent behavior, succeeding where possible. That would mean rejecting 3.i, 3.iii, 4.i, and 4.iii.

I'm agnostic about monthDayFromFIelds (OK with either throwing or not) but strongly agree with Shane about rejecting 3.i, 3.iii, 4.i, and 4.iii.

If you're OK with this plan then I think we can unblock all the other PRs on this topic.

If not, then let's discuss!

@gibson042
Copy link
Collaborator Author

I give reasons in the top description for why I believe that 4.i and 4.iii should be accepted (specifically because of 5.i, which is lower-level than 2 and must be valid for 2 to make sense). Discussion it is!

@justingrant
Copy link
Collaborator

justingrant commented Sep 14, 2023

Meeting 2023-09-14:

  • Accept 5.i
  • Reject 3.i, 3.iii, 4.i, 4.iii

This fix goes in https://tc39.es/proposal-temporal/#sec-temporal-totemporalmonthday:

  • Update 5.i to throw if calendarAbsent is false

Current:

          1. If _calendarAbsent_ is *true*, and _month_ is not *undefined*, and _monthCode_ is *undefined* and _year_ is *undefined*, then
            1. Perform ! CreateDataPropertyOrThrow(_fields_, *"year"*, 𝔽(_referenceISOYear_)).

Desired:

          1. If _month_ is not *undefined*, and _monthCode_ is *undefined* and _year_ is *undefined*, then
            1. If _calendarAbsent_ is *false*, throw a *RangeError* exception.
            1. Perform ! CreateDataPropertyOrThrow(_fields_, *"year"*, 𝔽(_referenceISOYear_)).
  • Update step 9 to use similar logic as the object case above, probably by introducing its own calendarAbsent so "unknown calendar" errors are not preempted.

  • While we're in the neighborhood, update these lines to change the ? to ! because fields is a purely internal null-prototype object from PrepareTemporalFields. (And if the same issue is present in other AOs, maybe split this fix into a separate editorial PR.)

          1. Let _month_ be ? Get(_fields_, *"month"*).
          1. Let _monthCode_ be ? Get(_fields_, *"monthCode"*).
          1. Let _year_ be ? Get(_fields_, *"year"*).

@gibson042
Copy link
Collaborator Author

gibson042 commented Sep 14, 2023

There's a problem with the above plan—some calendars accept era and eraYear as a substitute for year (e.g., plainMonthDay.equals({ calendar: "gregory", era: "ad", eraYear: 2000, month: 5, day: 2 }) should not throw per test262), but era awareness has intentionally been removed from the ECMA-262 part of Temporal. So I'm now back to preferring the post-#2500 spec (i.e., reject 3.iii Temporal.PlainMonthDay.from("08-01[u-ca=iso8601]") but delegate all object input to an explicit calendar).

EDIT: However, it looks like even 3.iii isn't currently being rejected. Here's one way to fix that (an alternative is early errors, but those are easier to overlook in my opinion):

diff
diff spec/abstractops.html
--- a/spec/abstractops.html
+++ b/spec/abstractops.html
@@@ -1375,8 -1375,8 +1375,9 @@@ ParseISODateTime (
             1. For each |Annotation| Parse Node _annotation_ contained within _parseResult_, do
               1. Let _key_ be the source text matched by the |AnnotationKey| Parse Node contained within _annotation_.
               1. Let _value_ be the source text matched by the |AnnotationValue| Parse Node contained within _annotation_.
-              1. If CodePointsToString(_key_) is *"u-ca"*, and _foundCalendar_ is *undefined*, then
-                1. Set _foundCalendar_ to CodePointsToString(_value_).
+              1. If CodePointsToString(_key_) is *"u-ca"*, then
+                1. If _goal_ is |TemporalMonthDayString|, throw a *RangeError* exception.
+                1. If _foundCalendar_ is *undefined*, set _foundCalendar_ to CodePointsToString(_value_).
             1. If _foundCalendar_ is not *undefined* and the ASCII-lowercase of _foundCalendar_ is not *"iso8601"*, throw a *RangeError* exception.
       1. If _parseResult_ is not a Parse Node, throw a *RangeError* exception.
       1. Let each of _year_, _month_, _day_, _hour_, _minute_, _second_, and _fSeconds_ be the source text matched by the respective |DateYear|, |DateMonth|, |DateDay|, |TimeHour|, |TimeMinute|, |TimeSecond|, and |TimeFraction| Parse Node contained within _parseResult_, or an empty sequence of code points if not present.
diff polyfill/lib/ecmascript.mjs
--- a/polyfill/lib/ecmascript.mjs
+++ b/polyfill/lib/ecmascript.mjs
@@@ -557,8 -557,8 +557,8 @@@ export function ParseTemporalMonthDayString(isoString) {
     month = ToIntegerOrInfinity(match[1]);
     day = ToIntegerOrInfinity(match[2]);
     calendar = processAnnotations(match[3]);
-    if (calendar !== undefined && calendar !== 'iso8601') {
-      throw new RangeError('MM-DD format is only valid with iso8601 calendar');
+    if (calendar !== undefined) {
+      throw new RangeError('MM-DD format is not valid with a calendar annotation');
     }
   } else {
     let z;

@sffc
Copy link
Collaborator

sffc commented Sep 15, 2023

I'll offer another solution.

I edited the OP and added some more lines (not changing the numeric identifiers of existing ones). They use the calendar "gregory".

We could make all of the built-in human calendars require the month code or year disambiguation, but let "iso8601" continue to accept numeric month and year.

This would somewhat reduce the potential of data-driven exceptions because programmers introducing a calendar system likely do so because they want something other than iso8601. We can inform those programmers quickly by making all of the built-in CLDR calendars require the same shape of the fields.

This would mean the following results:

  • Accept: 3.i, 3.iii, 4.i, 5.i
  • Reject: 3.v, 3.vi, 4.iv, 5.iii
  • Unspecified: 4.iii

@justingrant
Copy link
Collaborator

somewhat reduce the potential of data-driven exceptions because programmers introducing a calendar system likely do so because they want something other than iso8601

I think this argument sounds reasonable, esp. given the problem that Richard found that made the previous solution won't work.

@sffc - could you fix your numbering above? There is no 3.v, 3.vi, etc.

@sffc
Copy link
Collaborator

sffc commented Sep 15, 2023

@sffc - could you fix your numbering above? There is no 3.v, 3.vi, etc.

There is; I added them as noted above.

@justingrant
Copy link
Collaborator

Oh, whoops my page didn't refresh. Now I see. Your proposal sounds good to me.

  • Unspecified: 4.iii

Does that mean it's up to the calendar to decide whether to throw or not? If so, then I support.

gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Sep 16, 2023
@gibson042
Copy link
Collaborator Author

We could make all of the built-in human calendars require the month code or year disambiguation, but let "iso8601" continue to accept numeric month and year.

This would somewhat reduce the potential of data-driven exceptions because programmers introducing a calendar system likely do so because they want something other than iso8601. We can inform those programmers quickly by making all of the built-in CLDR calendars require the same shape of the fields.

This would mean the following results:

  • Accept: 3.i, 3.iii, 4.i, 5.i
  • Reject: 3.v, 3.vi, 4.iv, 5.iii
  • Unspecified: 4.iii

Good news: that is the current state of #2663.

gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Sep 17, 2023
ptomato added a commit to gibson042/test262 that referenced this issue Sep 19, 2023
Built-in non-ISO calendars require either monthCode/day, or month/day plus
some form of year specification.

This adds test coverage for each of the categories listed in
tc39/proposal-temporal#2664, of which some must
currently reside in the test/intl402/ folders.
ptomato added a commit to gibson042/test262 that referenced this issue Sep 19, 2023
Built-in non-ISO calendars require either monthCode/day, or month/day plus
some form of year specification.

This adds test coverage for each of the categories listed in
tc39/proposal-temporal#2664, of which some must
currently reside in the test/intl402/ folders.
ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue Sep 19, 2023
@sffc
Copy link
Collaborator

sffc commented Sep 19, 2023

  • Unspecified: 4.iii

Does that mean it's up to the calendar to decide whether to throw or not? If so, then I support.

Yes that's what it means. Calendar decides when to throw. ECMAScript establishes a convention that ISO-8601 doesn't throw on missing monthCode, but all human calendars do.

ptomato added a commit to gibson042/test262 that referenced this issue Sep 28, 2023
Built-in non-ISO calendars require either monthCode/day, or month/day plus
some form of year specification.

This adds test coverage for each of the categories listed in
tc39/proposal-temporal#2664, of which some must
currently reside in the test/intl402/ folders.
gibson042 pushed a commit to gibson042/test262 that referenced this issue Oct 26, 2023
Built-in non-ISO calendars require either monthCode/day, or month/day plus
some form of year specification.

This adds test coverage for each of the categories listed in
tc39/proposal-temporal#2664, of which some must
currently reside in the test/intl402/ folders.
gibson042 pushed a commit to tc39/test262 that referenced this issue Oct 26, 2023
Built-in non-ISO calendars require either monthCode/day, or month/day plus
some form of year specification.

This adds test coverage for each of the categories listed in
tc39/proposal-temporal#2664, of which some must
currently reside in the test/intl402/ folders.
ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants