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

time: fix for "AddDate() should use UTC if loc is nil" does not seem to actually fix the issue #23048

Closed
tve opened this Issue Dec 8, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@tve
Copy link

tve commented Dec 8, 2017

The fix for issue #15852 time: AddDate() should use UTC if loc is nil does not seem to actually fix things. See the following snipped in golang play:

package main

import "fmt"
import "time"

func main() {
	t1 := time.Date(2017, 1, 1, 0, 0, 0, 0, time.UTC)
	fmt.Printf("%+v\n", struct{s time.Time}{t1})
	t2 := t1.AddDate(0,0,0)
	fmt.Printf("%+v\n", struct{s time.Time}{t2})
}

results in

{s:{wall:0 ext:63618825600 loc:<nil>}}
{s:{wall:0 ext:63618825600 loc:<nil>}}

See https://play.golang.org/p/InvEKjzB5b

Supposedly this was fixed in https://go-review.googlesource.com/c/go/+/23561
Am I missing something?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 8, 2017

@tve, what's the problem?

You added 0 years, 0 months, and 0 days to t1 and got the same time.

As far as our API promises, we did exactly what you asked.

Discussing internal state is out of scope, unless it impacts the package's ability to deliver on its documented API. And it's not even clear what internal state you want to see, since you deleted the issue template which asks that question.

@tve

This comment has been minimized.

Copy link

tve commented Dec 8, 2017

Hmmm, two answers:

  1. the purpose of https://go-review.googlesource.com/c/go/+/23561 (initially raised as #15852) was specifically to make an application of AddDate(0,0,0) to a time object cause the location field to be non-nil. As far as I can tell, even though a fix was merged and a test written for it, it doesn't accomplish what was stated. You may say "we cared to fix it back then but we no longer care", fine.

  2. The reason I ended up stumbling into this is because I use reflect.DeepEqual in tests to compare actual vs. expected for deeply nested structures and I end up with a discrepancy due to some time objects having nil in the location. And so far I don't manage to create a simple static time object that has a UTC zone and doesn't have nil in the location field, I'm sure I'm missing something obvious. E.g. time.Date(2017, 1, 1, 0, 0, 0, 0, time.UTC) results in a nil location. Doing simple things like .AddDate(0,0,0) doesn't fix the nil. I assume you'll say "don't use DeepEqual"...

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 8, 2017

the purpose of https://go-review.googlesource.com/c/go/+/23561 (initially raised as #15852) was specifically to make an application of AddDate(0,0,0) to a time object cause the location field to be non-nil.

What text in either that code review or that bug makes you think this is about changing the internal values? As I read it, that bug was about making a nil Location mean the same thing as UTC, and not crashing on a nil pointer dereference.

The reason I ended up stumbling into this is because I use reflect.DeepEqual in tests
...
I assume you'll say "don't use DeepEqual"...

Sorry, but yes. This is just one of the traps of DeepEqual, and is why we're increasingly moving towards using https://github.com/google/go-cmp instead.

@tve

This comment has been minimized.

Copy link

tve commented Dec 8, 2017

Thanks for the responses! Time to look at go-cmp.

@tve tve closed this Dec 8, 2017

@golang golang locked and limited conversation to collaborators Dec 8, 2018

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