Skip to content

Conversation

ptomato
Copy link

@ptomato ptomato commented Jul 10, 2024

See #69 (comment). This is intended as a starting point for discussion about what's possible.

@ptomato ptomato marked this pull request as draft July 10, 2024 00:32
Copy link

@lann lann left a comment

Choose a reason for hiding this comment

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

I like this change overall. I've added some feedback on the actual changes plus some other notes that can be punted to a separate PR if you don't want to deal with them.

@ptomato
Copy link
Author

ptomato commented Aug 30, 2025

I've rebased this on top of #79. The changes added in this PR can be seen in bakkot/wasi-clocks@clocks-improvements-on-three...ptomato:wasi-clocks:proposed-clocks-improvements.

@bakkot
Copy link

bakkot commented Aug 30, 2025

@ptomato, sorry, I rebased my PR and broke this one again. Working link to view changes in this PR: bakkot/wasi-clocks@f1a63db...ptomato:wasi-clocks:proposed-clocks-improvements

bakkot and others added 7 commits September 1, 2025 11:45
The "timezone of an instant" is a misnomer because the `instant` type
doesn't carry timezone information. The host has a currently configured
time zone (or has no such concept, in which case the only sensible thing
to do is use UTC).
This flag is problematic because when a time zone is "in DST" is not well
defined. Most time zones in the world don't use DST at all. Of the ones
that do, most go to DST during the summer for half the year, but not all.

For example, the function in Moment.js that provides this functionality
comes with a giant caveat:
https://momentjs.com/docs/#/query/is-daylight-saving-time/
Other possibilities: "tzid", "iana-id", "identifier", "iana-identifier".

Returning a user-displayable name as part of timezone-display would
require more information: the user's preferred language, and the preferred
style for the name:
 - abbreviated or not
 - year-round or specific to the Instant
E.g., the time zone with the IANA id "America/Los_Angeles" might be
displayed as "Pacific Time", "Pacific Standard Time", "Pacific Daylight
Time", "PT", "PST", "PDT", "Nordamerikanische Westküstenzeit"...

The Rust iana_time_zone crate uses IANA time zone IDs, so if this
interface needs to be able to implement iana_time_zone, timezone-display
should have an IANA ID and not a user-displayable name.
There are time zones that used sub-minute or even sub-second UTC offsets
for instants in the past. E.g., when built using Vanguard format, the UTC
offset in the TZDB for "Asia/Ho_Chi_Minh" before July 1906 is
7:06:30.133333333.
If the timezone-display ID is an IANA ID, and we are going with the
approach of not making the localized ("PST" vs "PDT" vs "PT") name part of
this component, then the current time zone doesn't depend on the current
time.

After removing the isDST flag, timezone-display contains two pieces of
data, the ID and the UTC offset. The UTC offset is already available via
a function that takes an Instant as input. The ID could just be available
via its own function that doesn't take any input.

In that case there would be no need for timezone-display.
Instead of only specifying the epoch of the instant returned from now(),
specify the epoch of the instant type. It then follows that now()
returns an instant with that epoch.
@ptomato ptomato force-pushed the proposed-clocks-improvements branch from c91853b to 0c3e042 Compare September 1, 2025 18:59
@ptomato
Copy link
Author

ptomato commented Sep 1, 2025

OK, I pulled in Kevin's branches, this should be the comparison link now: ptomato/wasi-clocks@b340dbe...ptomato:wasi-clocks:proposed-clocks-improvements

We wish to be able to represent "is completely unaware of time zones" or
"failure to discover a time zone" in the API, distinct from a host that
always uses UTC.
Use types::duration for the return type of get-resolution instead. It
supports durations up to 584 years which is more than sufficient for any
clock resolution.

(A more capable duration type would be needed if we were doing any
arithmetic between instants.)
Copy link

@lann lann left a comment

Choose a reason for hiding this comment

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

One small suggestion but overall looking good. Thanks for this!


/// The same as `display`, but only return the UTC offset.
/// The number of nanoseconds difference between UTC time and the local
Copy link

Choose a reason for hiding this comment

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

I'd be moderately inclined to leave this as seconds. There is some appeal to being consistent with duration but I think it makes more sense to match instant.seconds here.

From what I've been reading there shouldn't be any need for sub-second offsets.

Copy link
Author

Choose a reason for hiding this comment

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

I'd encourage sticking with nanoseconds here — we found a number of cases in the tzdata where there were sub-second offsets. See Justin's original recommendation at #61 (comment)

If the concern is that it would be easier to just do instant.seconds - offset or whatever to get the local wall-clock time, then I'd suggest instead adding a method that does that, because you probably want wall-clock time to be expressed in Y-M-D-H-M-S and not epoch nanoseconds...

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.

3 participants