time: add Duration.Abs

[earthboundkid]

#50770 adds Time.Compare, which is good, however, as noted in the issue, because times have nanosecond precision, it is unusual for times to be exactly equal to each other unless they've been truncated somehow. One might think, oh, no big deal, just do | t1 - t2 | < delta. It isn't such a big deal, but there is a subtle bug here (which bit me, naturally). time.Durations are measured as nanoseconds and can only measure 260 years or so. If t1 is the zero time and t2 is not, t1.Sub(t2) produces a Duration that cannot be converted from negative to positive because it is math.MinInt64.

See playground:

func withinBad(t1, t2 time.Time, delta time.Duration) bool {
	diff := t1.Sub(t2)
	if diff < 0 {
		diff *= -1
	}
	return diff < delta
}

func withinGood(t1, t2 time.Time, delta time.Duration) bool {
	if t2.Before(t1) {
		t1, t2 = t2, t1
	}
	diff := t2.Sub(t1)
	return diff < delta
}

func main() {
	t1 := time.Time{}
	t2 := time.Now()
	fmt.Println(withinBad(t1, t2, 1*time.Second)) // true !!
	fmt.Println(withinBad(t2, t1, 1*time.Second)) // false
	fmt.Println(withinGood(t1, t2, 1*time.Second)) // false
	fmt.Println(withinGood(t2, t1, 1*time.Second)) // false
}

Anyhow, getting this right seems subtle enough to be worth adding a helper method to time.Time.

[mpx]

I think it would be better to provide an "absolute subtraction" or "difference" method instead (if anything):

// AbsSub returns abs(t-u). Always non-negative. If the result is too large it returns the maximum value.
// FIXME: naming
func (t Time) AbsSub(u Time) Duration

This allows people to do the delta comparison if they like, or something else.

For my use cases I find:

• Times are relatively close together
• The larger value is mostly clear due to the passage of time (eg, t1.Sub(t0)).

I agree comparison with the zero time increases the likelihood of getting this wrong (the bug/risk is non-obvious).

If AbsSub is added some reference should be added to time.Sub for people who only want non-negative numbers. There is a risk that most people who should use the new API won't know it exists (time.Time has a huge number of methods).

Observation: If time.Time{}.Sub(time.Now()) returned math.MinUint64+1 this wouldn't be an issue.
..but it would be a breaking change.

[earthboundkid]

That’s a good suggestion. It could be called AbsDiff or plain old Diff.

[rsc]

It sounds like today you would write:

d := t1.Sub(t2); -1*time.Minute <= d && d <= 1*time.Minute

and the suggestion is to instead be able to write:

t1.Diff(t2) <= 1*time.Minute

?

I'm not sure I would expect Diff to be absolute value of Sub, but maybe I'm unfamiliar with usage elsewhere?

[earthboundkid]

It sounds like today you would write:

d := t1.Sub(t2); -1*time.Minute <= d && d <= 1*time.Minute

The problem is that it is very easy to write this buggy code instead:

d := t1.Sub(t2)
if d < 0 {
  d *= -1
}
if d < 1 *time.Minute { // ...

I'm fine with the name AbsSub instead of Diff if the implication of being an absolute difference isn't clear enough from the other name.

[jimmyfrasche]

bit of a mouthful but DurationBetween would be descriptive name for this distance function. It's kind of redundant but naming it just Duration or just Between seems like it's missing something

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

It may be easy to write that buggy code but it seems easier to write the correct code (-1*time.Minute <= d && d <= 1*time.Minute), since it requires fewer statements. Still, I understand your point about having this footgun lying around with Sub returning math.MinInt64 in common cases.

Hacker's Delight refers to max(x-y, 0) as "difference or zero", suggesting that x-y (not |x-y|) is a typical meaning of "difference".

Perhaps we should instead add Duration.Abs, defined that time.Duration(math.MinInt64).Abs() == math.MaxInt64.
Then people can use t.Sub(t2).Abs().

[bcmills]

I don't think Abs really addresses the problem of Sub unexpectedly returning math.MinInt64, since it has surprising behavior in other ways. (Compare #20757 (comment).)

That is: it seems to me that this proposal would address one specific failure mode, but leave many of the other time.Duration footguns in place. Long-term, I would like to see a more holistic solution. (Short-term, maybe it's worth addressing this particular case pointwise? I really don't know.)

[earthboundkid]

Methods Duration.Absolute() Duration and Duration.Saturated() bool would be helpful, but only to those who think to use them…

In retrospect, if Duration were struct{ nanoseconds int } it could have safe arithmetic methods instead of unsafe + etc. I'm not sure how we'd get from here to there though.

[beoran]

If the current time.Duration turns out to have many infelicities, then a new, better designed package could perhaos help?

[earthboundkid]

Even if there is a time2go package at some point in the future, it would be good to make improvements to the existing time package for projects stuck on old time.

Ideas proposed in this issue:

• Time.Within(Time, Duration) bool
• Time.AbsSub(Time) Duration
• Duration.Absolute() Duration
• Duration.Saturated() bool

Do people have a strong sense that we should do all of these? Some of these? None of these? My feeling is that Absolute and Saturated are improvements to safety, so they should definitely happen, and then Within is convenient so it should maybe happen, and AbsSub is sort of redundant if Duration.Absolute exists, so it can be left off.

[rsc]

time is a very low-level package. We don't want to dump all possible APIs into it.
It sounds like we can address this specific problem with just func (Duration) Abs() Duration,
where time.Duration(math.MinInt64).Abs() == time.Duration(math.MaxInt64).

If people feel strongly about func (Duration) IsInf(sign int) (like math.IsInf), we could talk about that too,
but we should probably keep this one to Abs.

[earthboundkid]

Floating point numbers stay saturated once they hit infinity, but Duration will wrap, so IsInf seems less useful unless we also added ScaleBy, DivideBy, Add, and Sub from #20757. Or add saturating arithmetic operators to int (don't know the issue number for that, but I'm sure it exists). Abs is probably enough for now, and the other safety methods can be revisited later.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[gopherbot]

Change https://go.dev/cl/393515 mentions this issue: time: add Duration.Abs

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group