Instant.until(Instant, DateTimeUnit.TimeBased) and until(Instant, DateTimeUnit, TimeZone) maybe shouldn't be overloads

[kluever]

The following two APIs on Instant aren't really overloads:

• until(Instant, DateTimeUnit.TimeBased)
• until(Instant, DateTimeUnit, TimeZone)

One takes timezones into account and the other one doesn't. That doesn't seem like a great pair of APIs to be overloads of one another. Perhaps the time-based one should have a longer, scarier name to discourage use? (e.g., timeUntil() or timeUnitsUntil())

Same issue for the analogous plus() and minus() overloads on Instant.

[dkhalanskyjb]

That doesn't seem like a great pair of APIs to be overloads of one another.

Semantically they are more or less overloads. One could implement until(Instant, DateTimeUnit.TimeBased) like this:

fun Instant.until(other: Instant, unit: DateTimeUnit.TimeBased) = until(other, unit, TimeZone.UTC)

Though such an implementation would throw more often than what we have now.

Perhaps the time-based one should have a longer, scarier name to discourage use?

I don't see why we would want to discourage its use. The reason it doesn't take time zones into account is that it doesn't matter in which time zone to add 15 seconds to an Instant, the result is the same, so if the programmer knows that the unit they're dealing with is time-based, they can safely omit the time zone.

[kluever]

it doesn't matter in which time zone to add 15 seconds to an Instant, the result is the same

Unless of course you're adding 15 seconds during a DST transition...

From an internal "best practices" guide (emphasis my own):

While overloads of a method are independent, they should be thought of as a single method with multiple signatures. Use overloading only when the methods serve the exact same purpose, with differences limited to the interactions between the caller and implementation. There should be no behavioral differences between the overloads aside from the natural consequences of the differing signatures.

I still don't think these should be overloads: they have slightly different behaviors (the fact that the behaviors are the same 99.9% of the time is actually even worse -- users won't know they have a bug until their code has some weird DST bug.

[dkhalanskyjb]

Unless of course you're adding 15 seconds during a DST transition...

Nope, DST transitions don't affect this operation.

Functionally this is true because we even implement until(Instant, DateTimeUnit, TimeZone) via until(Instant, DateTimeUnit.TimeBased) if we determine that the passed unit is time-based: https://github.com/Kotlin/kotlinx-datetime/blob/master/core/jvm/src/Instant.kt#L163

Semantically this is true because Instant represents a moment in time. If we record a moment when some concert started and add 2 hours and 33 minutes to that moment, then we get a moment 2 hours and 33 minutes into the concert, no question about it, time zones don't factor into this at all. Of course, in some regions, the clock readings could have changed by some other value than 2 hours and 33 minutes due to transitions, but the clock readings are represented with LocalDateTime, not Instant.

We need to know the time zone to add date-based units to Instants because when we need to add 2 days, that is, go from 21:30 of July 15 to 21:30 of July 17, the difference between the two moments may not always be 48 hours due to transitions.

[netdpb]

I think the behavior is well-defined, but the worry is that it's easy to mistake one for the other. In fact, if you're implementing until(Instant, DateTimeUnit, TimeZone) to ignore the time zone when the unit is time based, that feels even more surprising to the user.

val until1 = instant1.until(instant2, HOUR * 24)
val until2 = instant1.until(instant2, HOUR * 24, TimeZone.of("America/New_York"))
val until3 = instant1.until(instant2, DAY, TimeZone.of("America/New_York"))

It's not immediately obvious that until2 and until3 may be different from each other if the instants cross a DST change, or that until1 and until2 won't ever be even if they do.

One thing you could do to disambiguate is to require a DateBased unit when accepting a time zone. Another is to give them slightly different names to make clear that they have different semantics.

[dkhalanskyjb]

it's easy to mistake one for the other

Could you elaborate on this? If the client code operates on a unit that's known to be time-based, there is no reason to pass the time zone as it would not be used anyway, and so the shorthand version is available and even preferred. If the unit is not known to be time-based, it's impossible to call the shorthand version, so there is no potential for confusion.

if you're implementing until(Instant, DateTimeUnit, TimeZone) to ignore the time zone when the unit is time based, that feels even more surprising to the user.

Is there a defensible implementation of until that somehow utilizes the time zone for time-based units?

require a DateBased unit when accepting a time zone

What do you propose to do in the polymorphic case where it's unknown whether the unit is date-based or time-based? Not providing an until overload that accepts a DateTimeUnit seems odd, as this forces the user to always case on the type of their DateTimeUnit values. If we do provide an until that accepts a DateTimeUnit, then it must also accept a time zone to support date-based time units, and then it's unclear what's the use in having DateTimeUnit.DateBased.

give them slightly different names to make clear that they have different semantics.

Which functions are you talking about here? Instant.until(Instant, DateTimeUnit.TimeBased) and Instant.until(Instant, DateTimeUnit, TimeZone)? If so, their return values are the same for time-based units.

[netdpb]

it's easy to mistake one for the other

Could you elaborate on this? If the client code operates on a unit that's known to be time-based, there is no reason to pass the time zone as it would not be used anyway, and so the shorthand version is available and even preferred. If the unit is not known to be time-based, it's impossible to call the shorthand version, so there is no potential for confusion.

I agree there's no reason to use the time zone for time-based units. But having a method that sometimes ignores one parameter based on the value of another parameter will lead to people writing subtle bugs.

For example, it's possible for someone to use a time-based unit that seems like a day-based unit:

val DAYS = HOURS * 24

val lengthInDays = start.until(end, DAYS, timeZone) // I expect it to consider the time zone, but it won't

I know that there's already a day-based DAY constant, but people will do something like this, even if it's not in such a simple form.

if you're implementing until(Instant, DateTimeUnit, TimeZone) to ignore the time zone when the unit is time based, that feels even more surprising to the user.

Is there a defensible implementation of until that somehow utilizes the time zone for time-based units?

I'm not sure. I think I would consider excluding that by having no until method that takes the base DateTimeUnit as a parameter.

require a DateBased unit when accepting a time zone

What do you propose to do in the polymorphic case where it's unknown whether the unit is date-based or time-based? Not providing an until overload that accepts a DateTimeUnit seems odd, as this forces the user to always case on the type of their DateTimeUnit values. If we do provide an until that accepts a DateTimeUnit, then it must also accept a time zone to support date-based time units, and then it's unclear what's the use in having DateTimeUnit.DateBased.

I think the fact that the behavior is subtly but meaningfully different in that a whole parameter is either required or not applicable means that polymorphism in this case may be more confusing than helpful. People will write bugs because the type system silently ignores information it can't use rather than making the programmer be explicit.

If you change that to Instant.until(Instant,.DateTimeUnit.DayBased, TimeZone) then users will be forced to think about whether they want the calculation to take time zones into account. If they want to consider time zones then they must have a day-based unit; if they don't want to consider time zones they can't use a day-based unit. I think making programmers be explicit about which logic they want will help them avoid subtle, hard to find bugs for their users.

give them slightly different names to make clear that they have different semantics.

Which functions are you talking about here? Instant.until(Instant, DateTimeUnit.TimeBased) and Instant.until(Instant, DateTimeUnit, TimeZone)? If so, their return values are the same for time-based units.

Sure, but that might be surprising to users who don't expect the time zone, a required parameter, to be ignored.

[dkhalanskyjb]

Ok, I see your point now. Thank you for the thorough response!

True, the compiler can prevent some mistakes if we process DateTimeUnit.TimeBased and DateTimeUnit.DateBased in separate functions to denote that adding time units to the representation of a moment in time as Instant doesn't depend on the time zone and that advancing a day (a month, a year) does.

That said, there are trade-offs to this beyond just inconveniencing the user by forcing them to case on their units. The problem is, making this distinction would be inconsistent with the existence of DateTimePeriod, which allows one to advance three months, go back three days, and then add three seconds. In essence, DateTimePeriod could be represented as a group of some amount of DateTimeUnit.MONTH, DateTimeUnit.DAY, and DateTimeUnit.NANOSECOND, and behaves as such. If we changed Instant.until to reflect the difference in handling dates and times, consistency would require that we also get rid of DateTimePeriod and instead introduce TimePeriod (we already have DatePeriod), working with which doesn't require passing a time zone.

However, Java Time already tried this with Period and Duration, causing much confusion in the end. Judging from the number of questions asked about how to specify seconds in a Period, why there is a day in both Period and Duration, how to get the "full" period between two instants, and many other things, and the number of blog posts and tutorials explaining this, the users do want a combined representation of the seemingly simple concept of waiting for two days, and then for two hours.

True, in the edge case when the only nonzero components of a DateTimePeriod are time-based, the passed time zone is not needed, but we certainly don't want to go down the path of the whole Period/Duration dichotomy.

The overall point is, the change you're proposing is not free and would lead to some difficult decisions.

Also, I would think that people who do understand the dangers of DST transitions would also research what semantics the different DateTimeUnit types have. The people who don't could just as easily make the following bug:

val DAY = DateTimeUnit.HOUR * 24 // not DateTimeUnit.DAY so the compiler shuts up about the time zones
val lengthInDays = start.until(end, DAY)

[dkhalanskyjb]

In the end, this doesn't seem worth pursuing. One important point is that now, kotlinx.datetime.Instant is about to become kotlin.time.Instant. This means that kotlin.time.Instant and kotlin.time.Duration (which was promoted to stable since 2020) are what most users will be exposed to, with kotlinx-datetime only being used in the more specialized scenarios. Adding a DateTimeUnit to Instant is no longer the primary means of doing arithmetics.

Also, after looking through how people are using kotlinx-datetime, it doesn't seem like this particular point is error-prone. Given that changing this would make the API less convenient, the tradeoff just doesn't seem worth it.