New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: time: make Duration safer to use (Go 2) #20757

Open
bcmills opened this Issue Jun 22, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@bcmills
Copy link
Member

bcmills commented Jun 22, 2017

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Feb 20, 2018

It would be nice to have an actual proposal here.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Feb 20, 2018

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

This comment has been minimized.

Copy link
Member

bcmills commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Member

bcmills commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Member

bcmills commented Feb 21, 2018

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

This comment has been minimized.

Copy link

tandr commented Mar 21, 2018

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

This comment has been minimized.

Copy link
Member

bcmills commented Mar 21, 2018

@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

This comment has been minimized.

Copy link

tandr commented Mar 21, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment