proposal: time/v2: make Duration safer to use

[bcmills]

time.Duration is currently very prone to accidental misuse, especially when interoperating with libraries and/or wire formats external to Go (c.f. http://golang.org/issue/20678).

Some common errors include:

• Unchecked conversions from floating-point seconds.
• Unchecked overflow when multiplying time.Duration values.
• Conversion from floating-point to time.Duration before scaling instead of after.
• Accidentally scaling an already-scaled value during conversion.

https://play.golang.org/p/BwwVO5DxTj illustrates some of these issues. The bugs are unsurprising once detected, but subtle to casual readers of the code — and all of the errors currently produce unexpected values silently. (Some but not all of them would be caught by #19624.)

For Go 2, I believe we should revisit the Duration part of the time API.

---

A quick survey of other languages shows that Go's Duration type is unusually unsafe. Among modern languages, only Swift appears to be prone to the same bugs as Go.

Out-of-range conversion and overflow

Exceptions:

• The Rust constructor panics on out-of-range conversions and provides checked variants of arithmetic operations.
• C# raises OverflowException or ArgumentException for out-of-range arguments (to FromMilliseconds and friends).
• Java doesn't appear to have an explicit conversion operator, but raises ArithmeticException on overflow to its arithmetic methods.
• Python's timedelta raises OverflowError.
• Standard ML raises the Time exception if the argument is out of range.

Floating-point or arbitrary-precision representations:

• C++11 allows the use of floating-point representations; I would expect that overflow with integer representations is undefined.
• The only OCaml time package I could find uses a floating-point representation.
• The Haskell constructor takes an arbitrary-precision Integer argument, but it does not appear to check ranges on floating-point to integer conversions.

Double-scaling

• Rust, Java, Python, C++11, and OCaml all have asymmetric scaling or multiplication operations: you can scale a duration by a float or an integer, but you cannot scale a duration by a duration.
• The Haskell type system will reject attempts at double-conversion: converting an Integer to a TimeInterval is an explicit function call, not just a reinterpretation of the type. However, it won't stop you from erroneously multiplying a TimeInterval by a TimeInterval.
• C# and Standard ML do not support multiplying time intervals at all, requiring a round-trip conversion through a primitive number type in user code.

[ianlancetaylor]

It would be nice to have an actual proposal here.

[bradfitz]

One actual proposal:

package time
type Duration struct { ns int64 }

That is, hide the representation, and make it impossible to use the normal math operators on it.

That's kinda how I read this bug originally.

[bcmills]

I think the best fit depends on the outcome of a number of other decisions for Go 2, particularly #21130 (const struct literals), #19624 (checked overflow) and #15292 (generic programming).

It also depends upon whether we want to make the time.Time API itself less prone to overflow (#20678).

Without assuming any of those, I'd propose an API like the one @bradfitz suggests, with the top-level constants replaced by functions.

type Duration struct { ns int64 }

func Nanoseconds(int64) Duration
func Microseconds(int64) (Duration, bool)
func MustMicroseconds(int64) Duration
[…]

func (Duration) ScaleBy(float64) (Duration, bool)
func (Duration) MustScaleBy(float64) Duration
func (Duration) DivideBy(Duration) float64

func (Duration) Add(Duration) (Duration, bool)
func (Duration) MustAdd(Duration) Duration
func (Duration) Sub(Duration) (Duration, bool)
func (Duration) MustSub(Duration) Duration

// And keep the existing time.Duration methods.
[…]

[bcmills]

Or perhaps get rid of the Must variants and use a floating-point representation instead, although floating-point for time makes me wary.

type Duration struct { ns float64 }

func Nanoseconds(int64) Duration
func Microseconds(int64) Duration
[…]

func (Duration) ScaleBy(float64) Duration
func (Duration) DivideBy(Duration) float64

func (Duration) Add(Duration) Duration
func (Duration) Sub(Duration) Duration

// And keep the existing time.Duration methods.
[…]

[ianlancetaylor]

A lot of existing code will break if time.Second and friends are no longer const. Go 2 is permitted to break code but we need a big big benefit to break so much code.

[bcmills]

A lot of existing code will break if time.Second and friends are no longer const.

To me, that sounds like a good argument for also adding struct constants, although I admit that that leads down a long and winding path toward generalized compile-time evaluation.

The other alternative is to move those constants into some compatibility package (go1time.Second?) and have the rewriting rules know to look for constants and insert explicit calls to convert them to the new time.Duration as appropriate. For example (https://play.golang.org/p/8PqNKRbLcgZ):

package main

import (
    "fmt"
    "time"
)

const (
    SamplingDuration = 10 * time.Second
    SamplingPeriod   = time.Second
)

type Samples [SamplingDuration / SamplingPeriod]int64

func main() {
    var samples Samples
    for i := range samples {
        samples[i] = time.Now().Unix()
        time.Sleep(SamplingPeriod)
    }
    fmt.Printf("%v\n", samples)
}

would be rewritten (automatically) to:

package main

import (
    "fmt"
    "go1time"
    "time"
)

const (
    SamplingDuration = 10 * go1time.Second
    SamplingPeriod   = go1time.Second
)

type Samples [SamplingDuration / SamplingPeriod]int64

func main() {
    var samples Samples
    for i := range samples {
        samples[i] = time.Now().Unix()
        time.Sleep(SamplingPeriod.Go2Duration())
    }
    fmt.Printf("%v\n", samples)
}

[tandr]

The other alternative is to move those constants into some compatibility package (go1time.Second?)

or make a new package time2 with some functionality (adapters) to use/convert items from time to time2 as needed, and have less concerns about compatibility/documentation/old-examples changes. This way existing code don't need to be touched, new code can use time2 and be happy campers.

[bcmills]

@tandr If you don't touch existing code, you severely limit the impact of any improvements.

If the new types are assignable to the existing types, then it's easy to accidentally convert to the existing types and write the existing bug-patterns.

On the other hand, if the new types are not assignable to the existing types, then anyone who calls a package that uses the new types has to do their own (explicit) conversions, and they will have a strong incentive to stick to the old package (and its bug-prone API).

[tandr]

@bcmills Bryan, thank you very much for the explanation.

I understand the value of "lets all move forward", but realistically, it is not always possible. Fresh example being yesterday with vendored code that we are using, and cannot upgrade due to conflicts with another library. And this code came from another team, not even from 3rd party outside (well, breaking changes were introduced by 3rd party, and other team does not have time to deal with it, and decided to stay on an older dependency).

The assumption that we can run "go fix" on everything is a bit too brave, sorry - it changes "constness" of code and its surroundings (libraries etc). The moment we change that code, this assumption of fixed and tested dependencies that they were using goes out of the window from the original maintainer's prospective (if he/she/them are even still around), sorry. "It works for us", "You break it - you keep it" (you touched it - you are fixing it) is far more common in development world that we are willing to admit due to lack of time, resources, or just desire to deal with "someone else's" problems.

Sorry for the long rant.

TL;DR I still would prefer a new "namespace" for breaking changes on standard library.

[bcmills]

Here's another fun example: time.Sub and time.Add can silently fail to be inverses, because time.Sub saturates rather than failing explicitly on overflow.

https://play.golang.org/p/QX-MnDNiIFt

[taralx]

The multiplication problem is an argument for units in the type system. That plus checked integer types should IIUC make this safe to use without making the API clunky.

[bcmills]

The multiplication problem is an argument for units in the type system.

I agree, but that would be a much larger proposal than this one. (Proper units in type systems require deep parametricity, but Go's type system doesn't have parametricity at all today.)

That plus checked integer types should IIUC make this safe to use without making the API clunky.

Checked integer types are (much to my chagrin) not a foregone conclusion. (#30209 is my current proposal. #30613 is @ianlancetaylor's current counter-proposal. There may be others.)

[taralx]

Proper units in type systems require deep parametricity

Do they though? We don't allow functions to be parametric over the current family of numeric types, and AFAIK nobody has asked for even that much yet.

[anjmao]

I actually never had any issue with time package and time.Duration. Maybe because I never wrote code like shown in the examples. While I agree that in some cases unexpected bugs can occur but I always felt that go constants use including time.Duration is genius invention.

[yawaramin]

A lot of existing code will break if time.Second and friends are no longer const.

@ianlancetaylor has there been any proposal or discussion of 'unboxing' or 'unwrapping' single-field structs? Imagining a thought experiment where that's done, single-field struct values could be unwrapped and treated as consts if the field is a const-able type. And it would make @bradfitz 's suggestion above preserve time.Second etc. as const: const Second = Duration{ns:...}. OCaml has a similar concept with the @unboxed attribute.

I am happy to write something separately in more detail if this has not been considered yet.

[ianlancetaylor]

@yawaramin I can't recall anything along those lines. It sounds a bit complicated, and it's not obvious to me how it would help with the problems mentioned in the original post on this issue. If we automatically unwrap, then it seems that those problems occur. If don't automatically unwrap, then we break existing code.