TimeZone.SYSTEM encourages static dependency

[JakeWharton]

Following on from the Instant.now() static dependency on the system clock, TimeZone.SYSTEM has a similar static dependency on the system.

Would it make sense to put this as an instance member on Clock as well? That's how (one of) our clock abstractions exposes it.

Without this you'll have the ability to control time in places like tests. Anyone calling into TimeZone.SYSTEM and combining it with an instant from a fake clock will produce a time in the hosts zone rather than a stable zone likely producing flaky tests.

[JakeWharton]

As it happens I was reading the JS proposal for their new date/time stuff this morning. They expose the TZ on Temporal.now which is an object whose shape acts somewhat as a clock: https://tc39.es/proposal-temporal/docs/index.html#Temporal-now

It is also the object that acts as a source for Temporal.Absolute which is their equivalent to an instant.

That seems to lend credibility to this change, and since presumably in the distant future this JS API will replace js-joda as the backing implementation it also will align nicely.

[JakeWharton]

I can't fork to create a PR while the repo is private nor do I have direct push access, but here's a take on this:

0001-Expose-TimeZone-through-Clock.patch.txt

(Had to add .txt suffix for GitHub to allow the upload)

[kevincianfarini]

Is there any interest in accepting this proposal? I'm finding people on my team using the static dependency quite a lot.

[dalewking]

I am very much against the idea of adding TimeZone into the Clock interface. One of the best parts of kotlinx-datetime is the way that there is a stark separation between the world of instants and the world of local representations of those instants. Coming from Klock i can tell you that the blurring of the line leads to lots of confusion. When you start having to do things like x.local.local something is wrong.

I also understand that there is a great need for a single interface for dealing with timezones along with instants for having a way to inject the sense of current time to code to control tests. But that should not be done by polluting the Clock interface, but instead should be done by extending the Clock interface.

That is what I did in our project where I created a DateTimeSource interface like this:

interface DateTimeSource : Clock, TimeSource {
    val timeZone: TimeZone
    val localDate: LocalDate
    val localTime: LocalTime
    val localDateTime: LocalDateTime

    fun toLocalDateTime(instant: Instant): LocalDateTime
    fun fromLocalDateTime(dateTime: LocalDateTime): Instant
}

Any part of the code that has to deal with knowing what the current time is or converting between utc and local has an instance of this interface injected into it. I have a Standard implementation that is injected for production:

open class StandardDateTimeSource : DateTimeSource, Clock by Clock.System {
    override val localDateTime get() = now().toLocalDateTime(timeZone)
    override val localDate get() = todayIn(timeZone)
    override val localTime get() = localDateTime.time

    override val timeZone: TimeZone get() = TimeZone.currentSystemDefault()

    override fun markNow() = asTimeSource().markNow()

    override fun toLocalDateTime(instant: Instant) = instant.toLocalDateTime(timeZone)

    override fun fromLocalDateTime(dateTime: LocalDateTime) = dateTime.toInstant(timeZone)
}

I also have an implementation which is used for tests that extends the standard implementation allowing you to precisely control when now is and how to convert between UTC and local:

class OverridableDateTimeSource(var now: Instant = Clock.System.now()) : StandardDateTimeSource() {
    override fun now() = now

    override var localDateTime get() = now.toLocalDateTime(timeZone)
        set(value) { now = value.toInstant(timeZone) }
    override var localDate get() = todayIn(timeZone)
        set(value) { localDateTime = LocalDateTime(value, localTime) }
    override var localTime get() = localDateTime.time
        set(value) { localDateTime = LocalDateTime(localDate, value) }

    override var timeZone: TimeZone = TimeZone.currentSystemDefault()

    operator fun plusAssign(duration: Duration) { now += duration }

    operator fun minusAssign(duration: Duration) { now -= duration }
}

[dalewking]

Did some refactoring and renaming:

interface DateTimeSource : Clock, TimeSource {
    val timeZone: () -> TimeZone

    val localDateTime get() = toLocalDateTime(now())

    val localDate get() = localDateTime.date

    val localTime get() = localDateTime.time

    override fun markNow(): TimeMark = asTimeSource().markNow()

    fun toLocalDateTime(instant: Instant) = instant.toLocalDateTime(timeZone())

    fun fromLocalDateTime(dateTime: LocalDateTime) = dateTime.toInstant(timeZone())

    class Standard(
        val clock: Clock = Clock.System,
        override val timeZone: () -> TimeZone = TimeZone.Companion::currentSystemDefault,
    ) : DateTimeSource, Clock by clock
}

class TestDateTimeSource(
    var now: Instant = Clock.System.now(),
    override var timeZone: () -> TimeZone = TimeZone.Companion::currentSystemDefault,
) : DateTimeSource {
    override fun now() = now

    override var localDateTime get() = super.localDateTime
        set(value) { now = fromLocalDateTime(value) }

    override var localDate get() = super.localDate
        set(value) { localDateTime = LocalDateTime(value, localTime) }

    override var localTime get() = super.localTime
        set(value) { localDateTime = LocalDateTime(localDate, value) }

    operator fun plusAssign(duration: Duration) { now += duration }

    operator fun minusAssign(duration: Duration) { now -= duration }
}

[kevincianfarini]

I am very much against the idea of adding TimeZone into the Clock interface. One of the best parts of kotlinx-datetime is the way that there is a stark separation between the world of instants and the world of local representations of those instants. Coming from Klock i can tell you that the blurring of the line leads to lots of confusion. When you start having to do things like x.local.local something is wrong.

I don't see how this proposal to expose a time zone from the Clock interface blurs the line between Instant and LocalDateTime. If anything, the suggestion you raised does that because it requires instances of DateTimeSource implement localDateTime, localDate, and localTime.

[dalewking]

Because Clock is a well-defined interface that has a single well-contained purpose. There are many uses for the current Clock interface that has no need for anything to do with local time, such as measuring timeouts, collecting UTC timestamps of events. There is no reason to shoehorn timezone into it instead of just making a sub interface to add timezone to it.

My DateTimeSource does not require instances to implement localDateTime, localDate, and localTime since it contains default implementations, but that is a good design point that rather than being in the interface many of the methods would be better as extension functions since there really isn't a reason to use a non-default implementation.

So renaming, refactoring to extension methods, and dropping the TimeSource part of it:

interface LocalClock : Clock {
    val timeZone: () -> TimeZone

    class Standard(
        val clock: Clock = Clock.System,
        val timeZone: () -> TimeZone = TimeZone.Companion::currentSystemDefault,
    ) : Clock by clock
}

val LocalClock.localDateTime get() = toLocalDateTime(now())

val LocalClock.localDate get() = localDateTime.date

val LocalClock.localTime get() = localDateTime.time

fun LocalClock.toLocalDateTime(instant: Instant) = instant.toLocalDateTime(timeZone())

fun LocalClock.fromLocalDateTime(dateTime: LocalDateTime) = dateTime.toInstant(timeZone())

class TestClock(
    var now: Instant = Clock.System.now(),
    override var timeZone: () -> TimeZone = TimeZone.Companion::currentSystemDefault,
) : LocalClock {
    override fun now() = now
}

var TestClock.localDateTime get() = toLocalDateTime(now())
    set(value) { now = fromLocalDateTime(value) }

var TestClock.localDate get() = localDateTime.date
    set(value) { localDateTime = LocalDateTime(value, localTime) }

var TestClock.localTime get() = localDateTime.time
    set(value) { localDateTime = LocalDateTime(localDate, value) }

operator fun TestClock.plusAssign(duration: Duration) { now += duration }

operator fun TestClock.minusAssign(duration: Duration) { now -= duration }

[JakeWharton]

Because Clock is a well-defined interface that has a single well-contained purpose. There are many uses for the current Clock interface that has no need for anything to do with local time, such as measuring timeouts, collecting UTC timestamps of events.

I wouldn't measure timeouts with a clock that can jump forwards or backwards. You're going to want a TimeSource for that, a mechanism we already have in the stdlib. I suppose you can make a Clock into a TimeSource if you're okay with jumps being included in measurement.

As to getting UTC timestamps, nothing really changes there. You quite simply ignore the time zone. Not sure what the problem is there.

[dalewking]

How would what I am suggesting cause jumps forwards or backwards? The jumps would only be for tests to allow the test to control the passage of time. For example we have a cache that uses Clock to check for expiration so you have a test that sets the clock just before expiration, verifies it is not expired, moves the clock to when it expires and verifies that it is now expired.

The reason I am sensitive on this subject comes from using the Klock library for the last 4 years. It doesn't have this rigid separation between UTC time and local time. Klock blurs the line considerably. It has DateTime class which internally is equivalent to Instant except that it also includes date and time instead of being strictly a timestamp. They also do not always represent UTC time and can represent local time. Conversion between UTC time and local time is very error-prone.

Twice per year around the DST changes, we waste many hours trying to trace down test failures because someone did not do the conversion correctly. Sometimes there are issues with dates whether one is getting the local date or the date of UTC which vary depending on timezone. I can tell you that over 4 years the amount of time spent on these kinds of issues can be measured in man-weeks.

That is one of the best parts of kotlinx-datetime that it makes this very rigid distinction between Instants and local dates and times and the only way to convert between the two is via a Timezone. Anything that is trying to blur that barrier between the two scares me because I have lived the extreme version of this with Klock. That is why i am against the idea of adding time zone to Clock. Clock has a well-defined job that only involves Instants. I agree that there should be an interface that also adds Timezone, but prefer it to be separate from Clock to keep that separation in place. We also are sensitive to this because in our case multiple timezones can be involved, not just the local device's time zone.

[JakeWharton]

Jumps would occur when I change the system time on my machine. This could be manual, or due to automatic systems like NTP sync.

My wife's MacBook can't keep time for shit. She can't visit a single website because the time is always off until it's manually changed after boot. If your program is using instants to measure something when I jump the clock two years into the present you aren't going to have an accurate measurement.

[kevincianfarini]

The reason I am sensitive on this subject comes from using the Klock library for the last 4 years. It doesn't have this rigid separation between UTC time and local time. Klock blurs the line considerably. It has DateTime class which internally is equivalent to Instant except that it also includes date and time instead of being strictly a timestamp. They also do not always represent UTC time and can represent local time. Conversion between UTC time and local time is very error-prone.

This proposal is not advocating for blurring the lines between Instant and LocalDateTime. That's one of my favorite parts about kotlinx-datetime, too. To Jake's point, you could ignore the system timezone exposed through a Clock.

From the above patchfile, the only convenience function that's exposed is the following:

-public fun Clock.todayAt(timeZone: TimeZone): LocalDate =
+public fun Clock.today(timeZone: TimeZone = timeZone()): LocalDate =
         now().toLocalDateTime(timeZone).date

And with the current API that could be invoked like the following and would have the same result, except now there's a static dependency on the system timezone.

val date: LocalDate = myClock.todayAt(TimeZone.SYSTEM)

There are currently no functions exposed on the Clock interface that allow you to directly acquire a LocalDateTime or LocalTime; you have to acquire those values using transformations on an Instant. I expect that the inclusion of a timezone on the Clock interface wouldn't change that, and therefore circumvents your concerns.

[dalewking]

Considering #382 asking to move Instant and Clock to the standard library without timezone related code, does this proposal still make any sense?

[JakeWharton]

Of course. The problem still exists, and a ZonedClock : Clock would be a welcome design after upstreaming of Clock.

[kevincianfarini]

The description of #382 seems to help justify the inclusion of an interface ZonedClock : Clock more.

With this change, the kotlinx-datetime library would provide calendar- and timezone-aware operations, whereas Instant, Duration, Clock, and TimeSource would work with just "the flow of time" as seen by the computer.

[dalewking]

Of course. The problem still exists, and a ZonedClock : Clock would be a welcome design after upstreaming of Clock.

Which is what I have argued for quite strongly in this thread. I have no problem with a sub-interface of Clock that added Timezone. My issue was with the notion that Timezone should be added to the Clock interface as i said before:

I also understand that there is a great need for a single interface for dealing with timezones along with instants for having a way to inject the sense of current time to code to control tests. But that should not be done by polluting the Clock interface, but instead should be done by extending the Clock interface.

I think it might be a good idea to close this issue and re-open a new ticket to avoid the confusion between the original proposal here to add TimeZone into clock

[JakeWharton]

That seems entirely unnecessary given the title and description of the issue focus on the problem and not a proposed solution, and that Clock has not moved.