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: AddDate() should use UTC if loc is nil #15852

Closed
KilledKenny opened this issue May 26, 2016 · 5 comments
Closed

time: AddDate() should use UTC if loc is nil #15852

KilledKenny opened this issue May 26, 2016 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@KilledKenny
Copy link
Contributor

I added all my env info at the end since everything is reproducible on play.golang.org

What did you do?

I called time's AddDate whit a time object that was missing a location

https://play.golang.org/p/S-ZZKabdyL

package main

import (
    "time"
)

func main() {
    time.Time{}.AddDate(0,0,0)
}

What did you expect to see?

I expected that AddDate would follow the behavior specified by loc documentation that it should use the default UTC as location when calling Date

Only the zero Time has a nil Location.
In that case it is interpreted to mean UTC.

Ref: https://golang.org/src/time/time.go?s=2261:2352

The expected behavior can be seen here: https://play.golang.org/p/-9Bp0rjphc

What did you see instead?

panic: time: missing Location in call to Date

goroutine 1 [running]:
panic(0xd6660, 0x1030a078)
    /usr/local/go/src/runtime/panic.go:481 +0x700
time.Date(0x1, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    /usr/local/go/src/time/time.go:1030 +0xa0
time.Time.AddDate(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    /usr/local/go/src/time/time.go:658 +0x100
main.main()
    /tmp/sandbox596209771/main.go:8 +0x60

My suggested solution

$ git diff
diff --git a/src/time/time.go b/src/time/time.go
index d9dbd34..6ead1ce 100644
--- a/src/time/time.go
+++ b/src/time/time.go
@@ -652,7 +652,8 @@ func Since(t Time) Duration {
 func (t Time) AddDate(years int, months int, days int) Time {
        year, month, day := t.Date()
        hour, min, sec := t.Clock()
-       return Date(year+years, month+Month(months), day+days, hour, min, sec, int(t.nsec), t.loc)
+       location := t.Location()
+       return Date(year+years, month+Month(months), day+days, hour, min, sec, int(t.nsec), location)
 }

 const (

Dose the fix look good? Should I submit it for review?

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

$ go version
go version go1.6.1 linux/amd64

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

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/simonr/go"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label May 26, 2016
@quentinmit
Copy link
Contributor

It looks like this panic has been in place since at least 2011; it's a simple change but I think it's probably too late for Go 1.7.

@KilledKenny Can you please submit your patch via Gerrit? (see https://golang.org/doc/contribute.html)

@rsc @bradfitz You seem to own the time package, do you think we should change this and do you agree we should wait for Go 1.8?

@quentinmit quentinmit added this to the Go1.8 milestone May 26, 2016
@KilledKenny
Copy link
Contributor Author

@quentinmit Yep, I'm aware of the contribute guidelines, I will attempt to submit the fix during the weekend.

@rsc
Copy link
Contributor

rsc commented May 27, 2016

It should probably be fixed, but let's wait for Go 1.8. It's clearly not critical since it has gone this long. Thanks.

@KilledKenny
Copy link
Contributor Author

@quentinmit I have submitted it now: https://go-review.googlesource.com/23561

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
4 participants
@rsc @quentinmit @KilledKenny @gopherbot