Inconsistency: Are days 24 hours or not?

[FeldrinH]

There seems to be some inconsistency in the treatment of days in spans. In particular, TryFrom<Span> for Duration assumes that days are 24 hours, whereas Timestamp::saturating_add rejects days as units that can't be resolved without a specific timezone.

Personaly I think the assumption that days are 24 hours is dangerous and shouldn't be made (because it is readily violated by DST), but more than that I think all functions should at least agree on whether days are assumed to always be 24 hours or not.

[BurntSushi]

It is true that sometimes days are treated as always 24 hours and sometimes days are treated as more or less than 24 hours, depending on time zone transitions. In contexts where the meaning of a day is ambiguous, it's banned, as in the case of Timestamp::checked_add (and Timestamp::saturating_add). In cases where it could be either, like in the Span APIs, whether a day is 24 hours or not is determined by whether 1) a relative date is provided and 2) whether that relative date is a Zoned or not.

The shape of when days are allowed or not, and when they are 24 hours or otherwise, was taken straight from Temporal. For example, see its Instant.add API.

I think it would be better to be very concrete here. If you're working with a Zoned, for example, it is impossible to accidentally treat "1 day" as "always 24 hours." The APIs won't get it wrong precisely because a Zoned is involved. But when you're working with civil time, you want to be able to operate on days, yet civil time is unaware of time zones. So days are always 24 hours.

While I agree that this can all seem very inconsistent and arbitrary upon initial inspection, the thing I really care about is whether you're committing bugs in your code because of it. Do you have any code you've written where you got bit by days being 24 hours when you didn't want them to be?

Personaly I think the assumption that days are 24 hours is dangerous and shouldn't be made (because it is readily violated by DST)

You really can't blanket reject the assumption. Because sometimes you want it. And if you sometimes you want it, you really can't have all functions agree on whether days are uniform or not. Like, if all functions agreed that days were non-uniform, then what would adding 1 day to civil::DateTime do? It doesn't have a time zone, so it can't choose anything other than "1 day is 24 hours." So in that context, it is definitionally uniform. There is no other possible interpretation that I can see. And once you have that, it flows through to other APIs, like Span.

[FeldrinH]

I guess I could have been clearer. I specifically have a problem with TryFrom<Span> for Duration and Span::total making that assumption. I think the behavior of civil::DateTime is fine, because if you use it you more-or-less explicitly choose to ignore DST and other timezone oddities. However, the conversion to Duration and getting the total duration of a Span in absolute units are general APIs and I think making the assumption that days are 24 hours there lends itself to misuse when combined with zoned datetimes.

[BurntSushi]

I specifically have a problem with TryFrom<Span> for Duration making that assumption.

Ah I see. That follows from the behavior of Span::round, Span::total, Span::compare, Span::checked_add and Span::checked_sub. And that's all consistent with the corresponding Temporal.Duration APIs as well. Namely, days are always treated as 24 hours unless a relative time zone aware datetime is given.

An alternative design is that these APIs return an error when non-zero day units are given and no relative date is given. But if a relative date is given, then whether it is civil or time zone aware would determine the length of the day.

I think this Temporal issue sketches out the shape of why Temporal went with this design decision. There is even more background here and here, but it gets very dense very quickly.

I am not opposed to diverging from Temporal. I specifically did not set a goal that Jiff must match Temporal. Jiff is merely heavily inspired by Temporal. With that said, I've generally deferred to Temporal on these kinds of decisions, because they quite literally reflect the result of multiple experts investing person years of effort into the design. It doesn't mean they're always right, but there's got to be something compelling IMO to push me towards reversing one of their decisions.

and I think making the assumption that days are 24 hours there lends itself to misuse when combined with zoned datetimes.

Can you give a concrete example?

[FeldrinH]

Currently not near my computer, but will give an example when I get back (probably tomorrow).

[FeldrinH]

The first problematic example that I stumbled upon is this (I actually didn't realize this code was incorrect until you mentioned that Span::round assumes 24 hour days):

I wanted to enforce that some input spans should only contain absolute units and I figured that a good way to do this would be to normalize the span to contain seconds as the largest unit. My assumption was that any ambiguity about the absolute duration of the span would produce an error:

let normalized_span = input_span.round(SpanRound::new().largest(Unit::Second)).unwrap();

This works fine in most cases, but will unexpectedly change the effective duration of the span when the span contains days and DST is involved, whereas I expected it to produce an error:

let input_span = 2.days();
let normalized_span = input_span.round(SpanRound::new().largest(Unit::Second)).unwrap();

let datetime = date(2024, 11, 3).at(0, 0, 0, 0).intz("America/New_York").unwrap();
println!("{}", &datetime + input_span);     // 2024-11-05T00:00:00-05:00[America/New_York]
println!("{}", &datetime + normalized_span) // 2024-11-04T23:00:00-05:00[America/New_York]

[BurntSushi]

But where did you get the input Span from? Getting a Span from two zoned datetimes will already return a Span with units no bigger than 1 hour. You'll only get bigger units if you explicitly ask for them.

[FeldrinH]

The span comes from external data (specifically some data dumps that store durations with the ISO 8601 format).

[BurntSushi]

The other problem with your suggestion is that, as of today, all of the Span APIs can be used correctly, without a relative date, using any Span returned by subtracting two datetimes in the default configuration. Your suggestion would make that not the case for spans from the difference between two DateTimes or two Dates.

[BurntSushi]

FWIW, I do find your suggestion compelling.

[FeldrinH]

The other problem with your suggestion is that, as of today, all of the Span APIs can be used correctly, without a relative date, using any Span returned by subtracting two datetimes in the default configuration. Your suggestion would make that not the case for spans from the difference between two DateTimes or two Dates.

Based on my (admittedly relatively surface level) understanding, I would propose this:
The difference between two DateTimes could use hours as the largest unit, which would match Zoned.
The difference between two Dates could be made to work by adding a special case that allows adding or subtracting two spans as long as they both contain only one unit and it is the same unit.

This would almost certainly be an ergonomic hit in some cases that I haven't thought of, but my biased opinion is that the extra strictness and correctness is worth it.

[FeldrinH]

PS: There's probably a good chance that this proposal breaks some part of the API that I haven't though of yet.

Also the proposed special case for spans is problematic in that it breaks the invariant that you can always add absolute units to a span. That tradeoff may or may not be worth it.

[BurntSushi]

Also the proposed special case for spans is problematic in that it breaks the invariant that you can always add absolute units to a span.

I believe this is not true today. If you're adding two spans and either of those spans have non-uniform units bigger than days, then you need a relative date. Because the lower units could add up and overflow into bigger units. I think the key thing today is that, under the default configuration for computing spans between any two datetimes, yes, you can always add absolute units to the span returned. But the change to avoiding implicitly assuming days are always 24 hours would indeed break that for DateTime and Date. The change to hours for DateTime would restore it there, but yes, I don't see a way to fix it for Date, which is unfortunate.

[FeldrinH]

Honestly, I don't know. Like you said, the discussion gets very dense very quickly and the tradeoffs are hard to evaluate. I would keep the issue open for now, in case someone else wants to weigh in with more practical experience or proposals.

[BurntSushi]

@FeldrinH Can you say more about your higher level use case here? Like, what is the higher level problem you're trying to solve? This is what I understand so far:

• You have some ISO 8601 durations in serialized form somewhere.
• You want to parse them and convert them into durations with units no bigger than seconds. Or more generally, a duration with uniform units.
• You want an error to appear if there is any ambiguity about what the units might mean.
• You demonstrate the problem by comparing the duration before and after rounding with a time zone aware datetime.

I don't mean to ask for these details as a way to dismiss your concern, but I do think it's important to connect the threads a bit more here. In particular, I am wondering about the connection here between the time zone aware datetime after the fact instead of providing it to the rounding routine. Or perhaps that is part of the problem. I could see, for example, not realizing that you need to provide a relative datetime to round, trying it and then relying on it to produce an error if you're doing something wrong. Since it's silent about days, you assume it's okay and go on to do something with those durations and time zone aware datetimes. But had round given you an error, I think your remedy would have been to use a zoned relative datetime with round and thereby get correct spans. Is that right?

If so, yeah, I can see how the failure mode is effectively a way of pushing you towards using the APIs correctly and preventing misuse.