CL https://golang.org/cl/38002 mentions this issue.

@sbuss note that the "docs" you linked to are comments on an unexported field of time.Time, not part of the public documentation of the type.

I've also noticed this in the following context: if you have a test that creates a timestamp with time.Now(), round-trips it through, say, JSON, and then compares the result to the original timestamp using == (or more commonly via reflect.DeepEquals where the timestamp is buried in some other structure), the test will pass unless the local tz offset is +0 (because the non-nil location round-trips to nil).

In this case the fix is to not write tests that use == on time.Times. But it is an interesting gotcha.

I'm not persuaded. I don't think the time package needs to cater to people who explicitly set time.Local = time.UTC. And I don't think the time package makes any promises about the unexported loc field for the case where the local timezone happens to be UTC.

If we're going to make this change, it needs a better rationale than I see so far in this description. For example, it needs an explanation in terms of something else that matters and can be clearly understood, like #15716.

I don't think the time package needs to cater to people who explicitly set time.Local = time.UTC

I included time.Local = time.UTC in the play link because I thought it'd be better to be explicit. But this also happens if your system timezone is set to UTC, like the golang playground: https://play.golang.org/p/sP141RpCuF

I've also noticed this in the following context: if you have a test that creates a timestamp with time.Now(), round-trips it through, say, JSON, and then compares the result to the original timestamp using == (or more commonly via reflect.DeepEquals where the timestamp is buried in some other structure), the test will pass unless the local tz offset is +0 (because the non-nil location round-trips to nil).

This is exactly how I discovered this issue, in fact. I was storing and retrieving some data to Google Cloud Datastore and doing a reflect.DeepEqual on the before-and-after. This manifests as the time you put into datastore not equaling the time you get from datastore. I know that users are supposed to use time.Equal, and we've since updated our tests, but the simple patch in the linked CL does fix some surprising behavior.

@ianlancetaylor well, more or less the same justification as #15716 applies here (it could be phrased as "Time.MarshalJSON and Time.UnmarshalJSON aren't symmetrical if the local timezone is UTC"):

package main

import (
	"fmt"
	"log"
	"time"
)

func main() {
	t0 := time.Now()
	s := t0.Format(time.RFC3339Nano)
	t1, err := time.Parse(time.RFC3339Nano, s)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Printf("t0==t1: %t\n", t0 == t1) // true, unless the location is UTC
	fmt.Printf("t0: %#v\n", t0)
	fmt.Printf("t1: %#v\n", t1)
}

https://play.golang.org/p/xx0fU4gn45

The commit message for 5fbf35d that fixed #15716 is relevant.

/cc @rsc

@cespare Thanks, I'm fine with fixing that problem, but then let's update the CL to be about that problem, with an appropriate test, rather than a test that explicitly sets Local = UTC.

but then let's update the CL to be about that problem, with an appropriate test, rather than a test that explicitly sets Local = UTC.

SGTM I will do this tomorrow

While trying to write the test you recommended, I discovered another potential bug in the time marshaling functions (#19502). I'll wait until learning more in that bug before revising my patch for this issue.

Edit: I figured out that that was intended behavior and know how to do this correctly. Should have a new patch soon.

I can't write a test which uses MarshalXYZ or UnmarshalXYZ because I have to disable the monotonic clock via AddDate(0, 0, 0), which calls setLoc and effectively masks the bug. So I can't meet your ask exactly, @ianlancetaylor, but the code snippet from @cespare is close enough that I'll go with that.

Actually, I can't use @cespare's code snippet, either, because of the same issue with the monotonic clock. You'll always get different structs if the monotonic clock isn't disabled, but the act of disabling it hides the bug. @ianlancetaylor Is there something else I could do to the patch or tests to make you more in favor of it?

Note that this issue doesn't show up on the golang playground yet because it's running a version of go that doesn't include the monotonic clock.

If we consider the test example, in Go 1.9 it will fail in all timezones; there's nothing special about UTC. So the motivation here kind of goes away. Perhaps we should just leave it alone.

I believe you can use t.Round(0) to get the same value without a monotonic time and without calling setLoc.

I believe you can use t.Round(0) to get the same value without a monotonic time and without calling setLoc.

Yup, that did it. Thanks for the pointer!

I just posted an updated patch which tests round trips through the Marshal/Unmarshal functions explicitly. Without the fix to Now and unixTime, the new test fails like this:

--- FAIL: TestDefaultLocNow (0.00s)
        time_test.go:1275: Now() and Now().UTC() behave differently for MarshalBinaryRoundTrip
        time_test.go:1275: Now() and Now().UTC() behave differently for GobEncodeRoundTrip
        time_test.go:1275: Now() and Now().UTC() behave differently for MarshalJSONRoundTrip
        time_test.go:1275: Now() and Now().UTC() behave differently for MarshalTextRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for MarshalBinaryRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for GobEncodeRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for MarshalJSONRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for MarshalTextRoundTrip

rather than a test that explicitly sets Local = UTC

I don't know how I'd affect the local timezone of the test runner without explicitly setting Local = UTC. Do you have a suggested way of doing this?

For testing purposes I suspect we need a new function like initTestingZone that sets the local location name to "UTC", just as initLocal does.

Seems like another way to fix this problem is to change MarshalJSON and friends to not use RFC3339Nano but instead use "2006-01-02T15:04:05.999999999-0700".

I do not believe we support packages overwriting package time's locations. That is, clients are not expected to do:

time.Local = time.UTC

That's not OK. It may cause arbitrary breakage (as it does here), and that breakage may change from release to release.

@rsc setting time.Local = time.UTC is a red herring; it was just how @sbuss was demonstrating the issue.

If I run this code (1.8 or tip):

package main

import (
	"encoding/json"
	"fmt"
	"log"
	"time"
)

func main() {
	t0 := time.Now().Round(0)
	b, err := json.Marshal(t0)
	if err != nil {
		log.Fatal(err)
	}

	var t1 time.Time
	if err := json.Unmarshal(b, &t1); err != nil {
		log.Fatal(err)
	}

	fmt.Printf("t0 == t1: %t\n", t0 == t1)
	fmt.Printf("t0.Equal(t1): %t\n", t0.Equal(t1))
	fmt.Printf("t0: %#v\n", t0)
	fmt.Printf("t1: %#v\n", t1)
}

on my local machine in PDT, I get:

t0 == t1: true
t0.Equal(t1): true
t0: time.Time{wall:0x370fff54, ext:63627184829, loc:(*time.Location)(0x586220)}
t1: time.Time{wall:0x370fff54, ext:63627184829, loc:(*time.Location)(0x586220)}

and if I run on a server in UTC, I get

t0 == t1: false
t0.Equal(t1): true
t0: time.Time{wall:0x2d79e290, ext:63627184833, loc:(*time.Location)(0x586220)}
t1: time.Time{wall:0x2d79e290, ext:63627184833, loc:(*time.Location)(nil)}

So the issue is that we got a timestamp that round-trips through JSON in PDT but not in UTC. (The same applies to other encodings as @sbuss's patch shows.)

Maybe we don't care about people using == to compare time.Times (commonly done by accident using reflect.DeepEqual), but your change 5fbf35d seems to indicate that we do care a little bit.

@cespare Do you know of any reason we can't change MarshalJSON as I suggested above?

@ianlancetaylor that might be okay, though I suppose on the margin it could break (strange) code that expects JSON-encoded UTC timestamps to use Z. Wouldn't we need to do similar fixes for the other encodings too? (MarshalText MarshalBinary/gob).

I guess my concern is that 5fbf35d purports to introduce the invariant that the UTC time.Times are only ever represented using a nil loc:

In the zero Time, the (not user visible) nil *Location indicates UTC.
In the result of t.UTC() and other ways to create times in specific
zones, UTC is indicated by a non-nil *Location, specifically &utcLoc.
This creates a representation ambiguity exposed by comparison with ==
or reflect.DeepEqual or the like.

Change time.Time representation to use only nil, never &utcLoc,
to represent UTC. This eliminates the ambiguity.

This bug is highlighting two cases -- time.Now and time.Unix -- where this doesn't hold. Now this invariant isn't part of the public API, but it seems to me that the reasoning given before should apply in these cases as well.

From this perspective, changing the JSON and other encodings seems to be papering over an underlying ambiguity.

I don't believe we can change MarshalJSON at this point, and I don't believe there is an actual problem here. All time formattings are lossy. It may well be that t0!=t1 but after a round trip serialization they are equal. For example if they differ only in the final nanosecond bit but the time only records microseconds, non-equal times become equal. The same thing (but different) is happening here. JSON cannot distinguish "UTC 00:00" from "Local 00:00" the way Go does.

This is a dup of #17875 / #17885.

@rsc Since you added setLoc to Parse, Time.UTC, Time.Local, Time.In, and Time.UnmarshalBinary in 5fbf35d to preserve the invariant that UTC Times have a nil loc, I don't understand why the same reasoning doesn't apply to Now and Unix (this issue). But I'll stop pursuing this now.

Change 5fbf35d was about making sure that the internal location pointer in a time.Time has a consistent internal representation for UTC times, namely nil and not &utcLoc.

The problem in the program in #19486 (comment) is different. Go always maintains a distinction between local and UTC times, even when TZ=UTC in the environment. Given that, the problem is that JSON encoding does not preserve this distinction, so it can't round trip back to the same representation:

$ cat /tmp/x.go
package main

import (
	"encoding/json"
	"fmt"
	"time"
)

func main() {
	t := time.Unix(1e9, 0)
	fmt.Printf("local %s\n", js(t.Local()))
	fmt.Printf("utc   %s\n", js(t.UTC()))
}

func js(x interface{}) string {
	js, _ := json.Marshal(x)
	return string(js)
}
$ go run /tmp/x.go
local "2001-09-08T21:46:40-04:00"
utc   "2001-09-09T01:46:40Z"
$ TZ=UTC go run /tmp/x.go
local "2001-09-09T01:46:40Z"
utc   "2001-09-09T01:46:40Z"
$ 

There's no way JSON unmarshaling can decode that pair of strings back to two different time.Times. One must be mapped to the other.