Skip to content
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

Jump to bottom

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
Closed

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

tve opened this issue Dec 8, 2017 · 4 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@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
Copy link
Contributor

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.

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 8, 2017
@tve
Copy link
Author

tve commented Dec 8, 2017 •
edited

Hmmm, two answers:

  1. the purpose of https://go-review.googlesource.com/c/go/+/23561 (initially raised as time: AddDate() should use UTC if loc is nil  #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
Copy link
Contributor

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.

@bradfitz bradfitz added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Dec 8, 2017
@tve
Copy link
Author

tve commented Dec 8, 2017

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

@tve tve closed this as completed 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.
Assignees
No one assigned
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bradfitz @tve @gopherbot