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

The chosen best format in Intl.DateTimeFormat is potentially null #3049

Open
trflynn89 opened this issue Nov 27, 2024 · 4 comments
Open

The chosen best format in Intl.DateTimeFormat is potentially null #3049

trflynn89 opened this issue Nov 27, 2024 · 4 comments
Assignees

Comments

@trflynn89
Copy link
Contributor

With the following Intl.DateTimeFormat object:

new Intl.DateTimeFormat([], { era: "narrow" });

We will enter CreateDateTimeFormat with required=ANY and defaults=DATE. And since there is no defined dateStyle or timeStyle, we hit this step to create the best format:

a. Let bestFormat be GetDateTimeFormat(formats, formatMatcher, formatOptions, required, defaults, ALL).

At this point, formatOptions will have an era field of "narrow".

When we enter GetDateTimeFormat, we will set requiredOptions to « "weekday", "year", "month", "day", "dayPeriod", "hour", "minute", "second", "fractionalSecondDigits" » because required=ANY.

We will then end up with anyPresent set to true, because formatOptions has an era field.
And we will have needDefaults set to true, because formatOptions does not have any of the fields in requiredOptions.

Thus we hit this step and return null:

17. If anyPresent is true and needDefaults is true, return null.

I don't think bestFormat is supposed to be nullable?

@trflynn89
Copy link
Contributor Author

Another case where this could happen is:

new Date().toLocaleDateString([],  { hour: "numeric" });

This invokes CreateDateTimeFormat with required=DATE and defaults=DATE.

So we have requiredOptions set to « "weekday", "year", "month", "day" », and anyPresent is true because hour is present, and needDefaults is true because the required options are all missing. Thus we return null for the best format again.

@ptomato ptomato self-assigned this Dec 5, 2024
@ptomato
Copy link
Collaborator

ptomato commented Dec 5, 2024

This seems like a regression from a94a793. It seems like we accidentally extended the "throw TypeError if you don't provide any relevant options to the Temporal type" behaviour described in #2795 (comment) to the existing Date behaviour, which was not the intention.

That could be solved by only returning null from GetDateTimeFormat if inherit=RELEVANT. That would preserve the intended behaviour of new Date().toLocaleDateString([], { hour: "numeric" }); while still making Temporal.Now.plainDateISO().toLocaleString([], { hour: "numeric" }) throw, as intended.

However, I will tentatively say that there still seems to be a problem around era.

ptomato added a commit that referenced this issue Dec 5, 2024
In a94a793 the desired behaviour when formatting Temporal objects was
accidentally applied to formatting Date objects. So

new Date().toLocaleDateString([],  { hour: "numeric" });

would result in bestFormat being null, and according to the type assertion
of the Intl.DateTimeFormat [[DateTimeFormat]] internal slot, that is not
possible.

Checking that _inherit_ = ~relevant~ here ensures the new behaviour is
only applied to Temporal objects, and the type assertion is not hit.

See: #3049
@ptomato
Copy link
Collaborator

ptomato commented Dec 6, 2024

I've addressed the main issue in #3053. new Date().toLocaleDateString([], { hour: "numeric" }); should now behave as it always has, and new Intl.DateTimeFormat([], { era: "narrow" }); should now not give a null bestFormat.

However, after looking at this a bit more, I do think there is still a problem. The DateTimeFormat object created by new Intl.DateTimeFormat([], { era: "narrow" }); seems like it still has wrong values in its internal slots:

  • [[DateTimeFormat]]: DateTime Format Record matched from options { [[era]]: 'narrow', [[year]]: 'numeric', [[month]]: 'numeric', [[day]]: 'numeric' }
  • [[TemporalPlainDateFormat]]: null
  • [[TemporalPlainYearMonthFormat]]: null
  • [[TemporalPlainMonthDayFormat]]: null
  • [[TemporalPlainTimeFormat]]: null
  • [[TemporalPlainDateTimeFormat]]: null (Note: this is not allowed as per the type of this internal slot, so it's an assertion failure)
  • [[TemporalInstantFormat]]: DateTime Format Record matched from options { [[era]]: 'narrow', [[year]]: 'numeric', [[month]]: 'numeric', [[day]]: 'numeric', [[hour]]: 'numeric', [[minute]]: 'numeric', [[second]]: 'numeric' }

One possibility is to have special treatment for era, in that it is deemed relevant to formatting PlainDate, PlainYearMonth, and PlainDateTime, and irrelevant to PlainMonthDay and PlainTime, but also that its presence does not prevent defaults for the other date fields from being copied in to the format matcher algorithm.

The other possibility is that it's actually a bug in ECMA-402 that CreateDateTimeFormat step 42.b makes it so that era can never occur by itself in a format. i.e., if you give era by itself, then defaults for year, month, and day are still provided to the format matcher. In fact there is already an issue open for this: tc39/ecma402#461.

My preference would be to settle the ECMA-402 bug and then 'rebase' Temporal on that.

Ms2ger pushed a commit that referenced this issue Dec 6, 2024
In a94a793 the desired behaviour when formatting Temporal objects was
accidentally applied to formatting Date objects. So

new Date().toLocaleDateString([],  { hour: "numeric" });

would result in bestFormat being null, and according to the type assertion
of the Intl.DateTimeFormat [[DateTimeFormat]] internal slot, that is not
possible.

Checking that _inherit_ = ~relevant~ here ensures the new behaviour is
only applied to Temporal objects, and the type assertion is not hit.

See: #3049
@anba
Copy link
Contributor

anba commented Dec 16, 2024

Removing "era" from the property names list in GetDateTimeFormat, step 14 should be enough to fix the remaining issue. That way "era" is treated the same as "timeZoneName", so it's an allowed value, but still triggers using default options when it's used as a standalone option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants