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

Example where lack of object shape checks in ToTemporalTimeZone and ToCalendar can be confusing #2354

Closed
anba opened this issue Jul 12, 2022 · 5 comments · Fixed by #2433
Closed
Assignees
Labels
meeting-agenda normative Would be a normative change to the proposal question

Comments

@anba
Copy link
Contributor

anba commented Jul 12, 2022

Probably just a FYI, because the underlying issue was already reported in #1652, which in turn links to #300 and #1294.

I was recently writing this incorrect code:

var tz = new Temporal.TimeZone("UTC");
var cal = new Temporal.Calendar("iso8601");
var zdt = new Temporal.ZonedDateTime(0n, cal, tz);

timeZone and calendar were passed in the wrong order, which leads to an error only after some methods on zdt.[[TimeZone]] or zdt.[[Calendar]] are called:

var duration = Temporal.Duration.from("PT48H");

// Throws: TypeError: getOffsetNanosecondsFor property is not callable
console.log(duration.round({smallestUnit: "hours", relativeTo: zdt}).toString());

Directly catching this mistake when creating the ZonedDateTime would be nicer, but I'm not sure how to best detect this case, given that accepting any object for the time zone and calendar arguments was an intentional design choice.

@ptomato
Copy link
Collaborator

ptomato commented Jul 12, 2022

Yikes, that's an easy trap to fall into, for sure.

This is indeed an intentional design choice, but as I mentioned in the other issue: if this is causing problems in the implementation, especially with respect to optimizability, I think we should put it on the agenda to discuss in a Temporal champions meeting.

@justingrant
Copy link
Collaborator

Seems like a good thing to fix. Obvious bug that's easy to encounter.

@ptomato ptomato added the normative Would be a normative change to the proposal label Sep 28, 2022
@ptomato
Copy link
Collaborator

ptomato commented Oct 14, 2022

We had a bit of discussion on this in the Temporal champions meeting of 2022-10-13 and have a temperature check for some possible solutions:

  1. Do nothing — didn't sound to me like there was strong support for this, but it did sound like everyone could grudgingly live with it.
  2. Stricter check on object shapes at entry points where Temporal.TimeZone and Temporal.Calendar are passed in — we didn't do this in the past because these checks are observable, but if it's only HasProperty and not GetProperty, people seem OK with this. Some strong preference for it, others lukewarm. In the TimeZone case it's clear which checks should be done, but in the Calendar case it's less clear-cut.
  3. Use a bag with named arguments in the Temporal.ZonedDateTime constructor, instead of one required and one optional ordered argument — some strong preference for this, others think it decreases ergonomics of the common case in which no calendar is passed
  4. Throw when passing an object with [[InitializedTemporalCalendar]] to ToTemporalTimeZone and vice versa, accepting the potential for bugs when using (very rare, specialized) plain-object protocol implementations — some strong preference for this, others think it's a hack without precedent in the language

We'll discuss this again in the future.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2022

I continue to feel pretty strongly that required args should be positional whenever possible, which seems to conflict with option 3.

@justingrant
Copy link
Collaborator

Champions meeting 2022-10-27: We'll go with #4 above:

Throw when passing an object with [[InitializedTemporalCalendar]] to ToTemporalTimeZone and vice versa, accepting the potential for bugs when using (very rare, specialized) plain-object protocol implementations — some strong preference for this, others think it's a hack without precedent in the language

ptomato added a commit that referenced this issue Nov 10, 2022
In order to prevent bugs like `new Temporal.ZonedDateTime(0n, cal, tz)`
(where the calendar and time zone arguments are switched, silently
poisoning the object) ToTemporalCalendar should throw if it encounters a
Temporal.TimeZone instance, and ToTemporalTimeZone should throw if it
encounters a Temporal.Calendar instance.

Includes implementation in the reference code.

Co-authored-by: Aditi <aditisingh1400@gmail.com>

Closes: #2354
@ptomato ptomato self-assigned this Nov 10, 2022
ptomato added a commit that referenced this issue Nov 29, 2022
In order to prevent bugs like `new Temporal.ZonedDateTime(0n, cal, tz)`
(where the calendar and time zone arguments are switched, silently
poisoning the object) ToTemporalCalendar should throw if it encounters a
Temporal.TimeZone instance, and ToTemporalTimeZone should throw if it
encounters a Temporal.Calendar instance.

Includes implementation in the reference code.

Co-authored-by: Aditi <aditisingh1400@gmail.com>

Closes: #2354
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting-agenda normative Would be a normative change to the proposal question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants