time: Round(0), Truncate(0) strip monotonic clock readings but documentation still says it returns t unchanged

[glycerine]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.9rc2 darwin/amd64

this does not happen, for contrast, with go1.8.3 darwin/amd64

This change is documented at the top of the new go1.9rc2 time/time.go file:

// For debugging, the result of t.String does include the monotonic                                             
// clock reading if present. If t != u because of different monotonic clock readings,                           
// that difference will be visible when printing t.String() and u.String().                                     

This is actually a rather drastic proposed change. I refer not the use of the monotone clock, but the display of it in timestamps formated via String(). Many of us use String() for many purposes other than debugging. For debugging, a separate new StringWithMonotone() should be provided.

What operating system and processor architecture are you using (go env)?

go env:
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jaten/go"
GORACE=""
GOROOT="/usr/local/go1.9rc2"
GOTOOLDIR="/usr/local/go1.9rc2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/6s/zdc0hvvx7kqcglg5yqm3kl4r0000gn/T/go-build055854381=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Here code to reproduce the issue, serialize_time.go. It is extracted from the time.Time serialization in a popular msgpack serialization package. e.g. getunix() and putunix() are here https://github.com/tinylib/msgp/blob/ad0ff2e232ad2e37faf67087fb24bf8d04a8ce20/msgp/integers.go#L113

package main

import (
	"fmt"
	"time"
)

func main() {

	// time to bytes
	t := time.Now()
	fmt.Printf("original  t='%s'\n", t.String())
	t = t.UTC()
	fmt.Printf("original  t='%s' in UTC\n", t.String())
	var b [12]byte
	putUnix(b[:], t.Unix(), int32(t.Nanosecond()))

	// bytes to time:
	sec, nsec := getUnix(b[:])
	t2 := time.Unix(sec, int64(nsec)).Local()

	fmt.Printf("restored t2='%s'\n", t2.String())
}

/* output of main:

original  t='2017-08-16 19:35:41.958759249 -0400 EDT m=+0.000241395'
original  t='2017-08-16 23:35:41.958759249 +0000 UTC' in UTC
restored t2='2017-08-16 19:35:41.958759249 -0400 EDT'

*/

func getUnix(b []byte) (sec int64, nsec int32) {
	sec = (int64(b[0]) << 56) | (int64(b[1]) << 48) |
		(int64(b[2]) << 40) | (int64(b[3]) << 32) |
		(int64(b[4]) << 24) | (int64(b[5]) << 16) |
		(int64(b[6]) << 8) | (int64(b[7]))

	nsec = (int32(b[8]) << 24) | (int32(b[9]) << 16) | (int32(b[10]) << 8) | (int32(b[11]))
	return
}

func putUnix(b []byte, sec int64, nsec int32) {
	b[0] = byte(sec >> 56)
	b[1] = byte(sec >> 48)
	b[2] = byte(sec >> 40)
	b[3] = byte(sec >> 32)
	b[4] = byte(sec >> 24)
	b[5] = byte(sec >> 16)
	b[6] = byte(sec >> 8)
	b[7] = byte(sec)
	b[8] = byte(nsec >> 24)
	b[9] = byte(nsec >> 16)
	b[10] = byte(nsec >> 8)
	b[11] = byte(nsec)
}

In summary, in go1.9rc2, time.Time.String() may produce:

2017-08-16 18:41:39.184829495 -0400 EDT m=+0.057013769

whereas in go1.83, the String() applied to the same value produces always:

2017-08-16 18:41:39.184829495 -0400 EDT

While not a language consistency issue, this is a backwards-compatibility issue for programs that expected one thing from their time.Time values. For example, my tests in a fork of the tinylib/msgp lib cited above are suddenly broken under go1.9rc2. So these changes may break many programs unexpectedly when moving from go1.8.3 to go1.9.

I'm glad for the new monotonic time feature under the covers, as it solves bugs with walltime going through leap seconds, but does it have to surface itself into the string representation of time in a non-backcompatible way? For the new info, how about adding a separate stringification method if one needs both timestamps? Perhaps: StringWithMonotone() alongside the old fashioned String().

In summary, while useful, features meant for debugging the new time.Time values should not break backwards compatibility, when they can trivially be provided alongside in a separate new method.

[ianlancetaylor]

As the comment says, this is an intentional change, because without it you can have two time.Time values that are not == but print the same.

It would be hard to change it at this point in the release process. At the least we would want to hear whether anybody else is reporting a breakage. This is the first problem report I've seen on this.

CC @rsc

[glycerine]

without it you can have two time.Time values that are not == but print the same.

Ah.

I think I should probably start serializing the monotone part as well....since the typically check on serialization correctness is to compare for equality with the original value after serialization and deserializing... hmm. Or start inserting StripMono() calls.

Since it looks like its inevitable, this would be helpful for patching code that wants to use 1.9: Perhaps stripMono() could be exported? I've developed the following workaround, but its a rather elaborate and an extra function call when stripMono() could simply be StripMono().

func StripMono(tm time.Time) time.Time {

  // If we're going to print t, but
  // we don't want to show the
  // m+0.057013769 monotone timestamp at the end...

  // Since t.stripMono() is private at the moment,
  // we have to	do the mystical incantation:

  return tm.In(tm.Location())

  // While it looks like the above should be a no-op, it has
  // the effect of calling t.stripMono() and so getting
  // rid of the monotone part of the String() representation.
  // Update: ugly and fragile b/c it depends on unspecified
  // behavior in the time API.
}

Actually it looks like Truncate(0) might be a single function call to
accomplish the same thing. But the docs (below) should probably be updated
to note that t is not returned unchanged, but rather has had the
monotonic timestamp portion stripped, and so will no longer be
equal to the input if the input had a non-zero monotone part.

// Truncate returns the result of rounding t down to a multiple of d (since the zero time).                     
// If d <= 0, Truncate returns t unchanged.                                                                     
//                                                                                                              
// Truncate operates on the time as an absolute duration since the                                              
// zero time; it does not operate on the presentation form of the                                               
// time. Thus, Truncate(Hour) may return a time with a non-zero                                                 
// minute, depending on the time's Location.                                                                    
func (t Time) Truncate(d Duration) Time {
    t.stripMono()
    if d <= 0 {
        return t
    }
    _, r := div(t, d)
    return t.Add(-r)
}

Or not. Actually, I'm reconsidering. It would make more sense to keep Truncate(0) API backwards compatible (move the stripMono after the test rather than before). After all, this is otherwise a change in the API. I'm sure there is code that depends upon its claim of If d <= 0, Truncate returns t unchanged that will break.

And just simply export StripMono.

[dmitshur]

Time.String documentation mentions:

for a stable serialized representation, use t.MarshalText, t.MarshalBinary, or t.Format with an explicit format string.

fmt.Stringer documentation says:

Stringer is implemented by any value that has a String method, which defines the “native” format for that value. The String method is used to print values passed as an operand to any format that accepts a string or to an unformatted printer such as Print.

Importantly, time package documentation says in multiple places that the Time.Equal method should be used to test whether two time.Time values represent the same time instant. Using == operator or String method has documented pitfalls.

I don't be think you can rely on Stringer output not changing between any Go versions. That would be akin to using reflect.DeepEqual and expecting tests not to break. It's not viable. Instead, alternatives that are guaranteed to produce stable marshaling or equality comparisons should be used.

@dsnet gave a very informative talk on this subject at this year's GopherCon, see https://www.youtube.com/watch?v=OuT8YYAOOVI.

[glycerine]

I see that the go1.9rc2 time API documentation does recommend using Round(0) to strip the mono part; at https://github.com/golang/go/blob/master/src/time/time.go#L46

// The canonical way to strip a monotonic clock reading is to use t = t.Round(0).

However the documentation for Round itself belies this, also claiming at https://github.com/golang/go/blob/master/src/time/time.go#L1403 that

// If d <= 0, Round returns t unchanged.

[glycerine]

Meta: The Round(0) choice just seems like poor API design; to overload the meaning of Round(0), when StripMono() would be clear and does exactly and only what it says. The former requires a trip to the documentation to read up what on earth Round(0) means by every new user. The later does not. Also then the meaning of Round(0) becomes constrained and cannot be changed to mean something more logical/relevant in the future.

[dmitshur]

On the topic of Round(0) choice, see relevant issue #18991 for prior discussion.

[dsnet]

This issue of String's output changing was brought up in #20876, and the decision there is to be more firm that String is intended for human consumption only, in which case there is no guarantees about stability.

As @shurcooL mentioned, MarshalText, MarshalBinary, or Format should be used for stable representations of time.

[rsc]

@glycerine

Meta: The Round(0) choice just seems like poor API design; to overload the meaning of Round(0), when StripMono() would be clear and does exactly and only what it says. The former requires a trip to the documentation to read up what on earth Round(0) means by every new user. The later does not. Also then the meaning of Round(0) becomes constrained and cannot be changed to mean something more logical/relevant in the future.

All this is true, but if you write Round(0) then it will work in Go 1.8, so we avoid bifurcating the world of compiling Go programs.

[glycerine]

we avoid bifurcating the world of compiling Go programs.

@rsc Very good rationale. Thanks for pointing that out.

[glycerine]

@dsnet This bug points out two important documentation fixes that should be made. Since you seem in a hurry to close it, would you mind opening a new bug to note the needed doc fixes to time.Round and time.Truncate? Both claim they are unchanged with a zero argument, but that is no longer true in 1.9.

[dmitshur]

I'll mention https://talks.godoc.org/github.com/davecheney/go-1.9-release-party/presentation.slide#26 here too, since it's relevant to the topic in the original issue. /cc @davecheney

[gopherbot]

Change https://golang.org/cl/56690 mentions this issue: time: fix documentation of Round, Truncate behavior for d <= 0

[glycerine]

@shurcooL the CL looks nice, thanks for doing it. It is probably worth reiterating one additional note in the time.Round doc as well, since it is buried in the wall of text up front and I missed it on first pass:

The canonical way to strip a monotonic clock reading is to use t = t.Round(0).

[dmitshur]

@glycerine Ive considered doing that, but saw that it wouldn't be helpful. If the user doesn't already know Round(0) is the canonical way to strip mono, they won't be able to jump to its documentation to learn that. It's better to leave the rest of time package docs to say Round(0) is canonical. Other ways are valid too.

[glycerine]

What will happen is this: user sees call to Round(0) in some code they haven't written themselves. Question occurs to user: what the heck is this Round(0) call doing? It doesn't make any sense!?! Let's look at the documentation for Round! User positions cursor over the Round(0) call and presses F12 (or whatever godef is bound to), and jumps to the documentation for Round. Now does the user get a clue? They could, if we help them out here. Or we could just leave them mystified.

I'm not saying remove anything. I'm just saying we should add to the Round documentation that one sentence to explain Round(0); in addition to the preamble stuff.

Come on. Let's not be stingy with docs. This is crappy API, as rsc agreed. Let's help all we can.

[dmitshur]

what the heck is this Round(0) call doing? ... jumps to the documentation for Round. Now does the user get a clue?

Yes, the docs will say:

If d <= 0, Round returns t stripped of any monotonic clock reading but otherwise unchanged.

So the user will know that Round(0) was used to strip a monotonic clock reading.

[dsnet]

Re-opening for cherry-pick.

[rsc]

CL 56690 OK for Go 1.9.2 (doc fix).

[gopherbot]

Change https://golang.org/cl/70846 mentions this issue: [release-branch.go1.9] time: fix documentation of Round, Truncate behavior for d <= 0

[rsc]

go1.9.2 has been packaged and includes:

• CL 56690

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:07 UTC