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

Synchronise Intl updates with latest ECMA-402 #2904

Merged
merged 5 commits into from
Jul 1, 2024
Merged

Conversation

anba
Copy link
Contributor

@anba anba commented Jun 24, 2024

Synchronise Intl.DateTimeFormat updates with latest ECMA-402, see individual commit messages for more in-depth explanations.

anba added 2 commits June 24, 2024 08:46
- Update ecma262-biblio to a more recent version, but don't use the
  latest version (2.1.2736), because that requires replacing
  `IsIntegralNumber`, which has been removed upstream.
- Add ecma402-biblio to replace the manual biblio entries in
  "spec/biblio.json".
General changes:
- Synchronise with latest ECMA-402 draft, including changes from tc39/ecma402#901.
- Change `CreateDateTimeFormat` to always store the hour-cycle in
  `dateTimeFormat.[[HourCycle]]`, so later formatting operations can select the
  correct pattern string. Update `Intl.DateTimeFormat.prototype.resolvedOptions`
  to only output "hourCycle" and "hour12" when `[[DateTimeFormat]]` actually
  has an `[[hour]]` field. This is needed to match the existing behaviour.
- Change `Value Format Records` to prefer upper-case fields and replace
  `[[pattern]]` and `[[rangePatterns]]` with `[[Format]]` which holds a
  `DateTime Format Record`.
- Correctly specify `CreateTemporalDateTime` as infallible in
  `HandleDateTimeTemporal{Date,MonthDay,Time}`. `CreateTemporalDateTime` in
  `HandleDateTimeTemporalYearMonth` is fallible, because the ISO reference day
  can make it an out-of-range date-time value.
- Remove unreachable case for a `null` format record in
  `HandleDateTimeTemporal{DateTime,Instant}`.
- Correct value conversion in `HandleDateTimeOthers`, because `ℤ` can't be applied
  on a Number value.
- Update `Temporal.ZonedDateTime.prototype.toLocaleString` to accept offset
  time zones, now that upstream ECMA-402 supports offset time zones.
- Add standalone "month" as a required format to the `[[formats]]` list. This change
  is needed to correctly process `Temporal.PlainYearMonth` and `Temporal.PlainMonthDay`,
  because right now only "year+month" and "month+day" are required formats. When the
  user requests a standalone "month", `BasicFormatMatcher` will compute the same
  penalty for "year+month" and "month+day", so both formats have the same preference.
  This will then result in either incorrectly using "year+month" with
  `Temporal.PlainMonthDay` or using "month+day" with `Temporal.PlainYearMonth`.
  This incorrect case can't apply when standalone "month" is a required format.
- It's not possible to request that all options are supported in a standalone
  context, because for example "year" with "en-u-ca-islamic" will currently also
  add "era" to the output string in implementations.

Notes:
- `FormatDateTimePattern` is now an infallible operation, but adding "ins" / "del"
  marker tags doesn't seem to work in structured types definitions.

---

Updated date-time format selection:

Case 1: `dateStyle` or `timeStyle` is present.

When one of the style options is present, then the previous condition

> If bestFormat does not have any fields that are in Supported Fields, [...]

is equivalent to testing which of `dateStyle` or `timeStyle` is `undefined`,
because for example when `dateStyle` isn't `undefined`, then "year", "month",
and "day" are guaranteed to be present. That also means only a single call to
`DateTimeStyleFormat` is needed to compute the correct format for all types.

Case 2: `dateStyle` or `timeStyle` are both absent.

Add the new abstract operation `GetDateTimeFormat` which implements the same
formatting options processing which is currently inlined in `CreateDateTimeFormat`.

The logic is now as follows:
- If at least one option from `requiredOptions` is present, then use the user inputs.
- Otherwise use the default options from the `defaultOptions` list.
- `requiredOptions` and `defaultOptions` have different entries based on the
  `required` and `defaults` arguments.
- The `inherit` parameter selects which user options in addition to the members
  of `requiredOptions` are copied over:
  - When `inherit=all`, then all user inputs are copied over, even if they are
    unexpected in the context given by `required` and `defaults`. This is needed
    for backwards compatibility with the current ECMA-402 spec. For example
    `new Date().toLocaleTimeString("en", {era: "long"})` currently returns
    `"Anno Domini, 7:46:53 AM"` in all engines, which means the "era" option is
    used even though the formatting context is a local-time string.
  - When `inherit=relevant`, only relevant additional options are copied over. These
    are options which are relevant, but can't be elements of `requiredOptions`:
    - "era" can't occur in a standalone context, there it's not a member of the
      `requiredOptions` list.
    - "hourCycle" is needed for the pattern selection, see tc39/ecma402#571.
  - The `inherit=relevant` case only applies to `Temporal.PlainX` types to ignore
    unsupported options. For example
    `new Intl.DateTimeFormat("en", {day: "numeric"}).format(new Temporal.PlainTime())`
    should return the string `"12:00:00 AM"` and don't display any "day" parts.
  - `Temporal.Instant` and `Temporal.ZonedDateTime` support all date-time options,
    so no additional filtering is needed for them.
- In addition to "era", the "timeZoneName" option also can't occur in a standalone
  context, therefore it's also not an element of the `requiredOptions` list.

The default options processing in GetDateTimeFormat matches the previous
Temporal draft, but it's not clear at this point if this implements previous
concensus, because it means the unsupported format case can only apply when
`dateStyle` or `timeStyle` are used. IOW `new Intl.DateTimeFormat("en", {day: "numeric"})`
works with all `Temporal` types, including `Temporal.PlainTime`, whereas
`new Intl.DateTimeFormat("en", {dateStyle: "short"})` throws when used with
`Temporal.PlainTime`.
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't go through the second commit in detail yet, but now that we have CanonicalizeUValue in ECMA-402 would you like to include your conclusion from tc39/ecma402#828 (comment) in this PR as well?

spec/biblio.json Outdated Show resolved Hide resolved
@anba
Copy link
Contributor Author

anba commented Jun 28, 2024

[...] now that we have CanonicalizeUValue in ECMA-402 would you like to include your conclusion from tc39/ecma402#828 (comment) in this PR as well?

Added b0b9467 to replace ASCII lowercase normalisation for calendar identifiers with CanonicalizeUValue.

@ptomato
Copy link
Collaborator

ptomato commented Jun 28, 2024

@anba It's labelled "Normative", but I'm not sure it is really normative — it seems to me we are simply adjusting to a normative change that was already made upstream in ECMA-402.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. It looks much cleaner and easier to understand now.

I don't think I fully understand the hourCycle/hour12 issue but if you say that is what's needed to replicate existing behaviour, I believe that.

I'd consider this ready to merge, just would like to clear up your expectations for the CanonicalizeUValue commit — do you think this needs to be presented at plenary? I would say it does not.

Optionally, consider removing the resolvedOptions table and the definition of AvailableCalendars in favour of linking to the biblio versions of them. However I wouldn't want to delay the PR waiting for this, so I can also just do those later.

spec/intl.html Outdated Show resolved Hide resolved
@anba
Copy link
Contributor Author

anba commented Jul 1, 2024

I'd consider this ready to merge, just would like to clear up your expectations for the CanonicalizeUValue commit — do you think this needs to be presented at plenary? I would say it does not.

I don't think it's necessary to present this is at plenary. I've only marked this as "normative" because the polyfill needs to be changed, so there's at least one implementation where this change has a normative effect.

Optionally, consider removing the resolvedOptions table and the definition of AvailableCalendars in favour of linking to the biblio versions of them. However I wouldn't want to delay the PR waiting for this, so I can also just do those later.

I've removed the resolvedOptions table, but I wasn't sure about what to do with AvailableCalendars, because AvailableCalendars is called from the main spec of Temporal, but upstream AvailableCalendars is from ECMA-402.

@ptomato
Copy link
Collaborator

ptomato commented Jul 1, 2024

Thanks.

I've looked at updating the reference code to also canonicalize calendars, to match the latest ECMA-402. However, there's one test262 test which treats islamicc and islamic-civil as different, so we'd need to update that first. I don't want to block this PR on that, so I'll open a follow up issue.

@ptomato ptomato merged commit 25d0b97 into tc39:main Jul 1, 2024
5 checks passed
@ptomato
Copy link
Collaborator

ptomato commented Jul 1, 2024

I'll also deal with AvailableCalendars in a follow up issue.

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 this pull request may close these issues.

None yet

2 participants