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

time: Parse inappropriately sets Location() to Local in some circumstances #19750

Closed
lupine opened this issue Mar 28, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@lupine
Copy link

commented Mar 28, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8 darwin/amd64

I can also see this on go1.6.2 darwin/amd64 and go.7.4 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/lupine/.gvm/pkgsets/go1.8/global"
GORACE=""
GOROOT="/Users/lupine/.gvm/gos/go1.8"
GOTOOLDIR="/Users/lupine/.gvm/gos/go1.8/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/lh/4xp9ym5137vdbpg81jw192dm0000gn/T/go-build598903070=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Here's an example on the playground: https://play.golang.org/p/ND8CEYOKcT

The correct behaviour is actually seen when this example is run on playground, so I've included the output I see on my own machine.

What did you expect to see?

time.Parse("-0700", "+0000") should return a time.Time with a Location() with an offset of +0000 in all cases

What did you see instead?

time.Local was used instead, resulting in the timezone changing from +0000 to +0100 on my local machine.

Adding the year to the parse string is enough to get the desired behaviour

@lupine

This comment has been minimized.

Copy link
Author

commented Mar 28, 2017

Just to be clear, my timezone is Europe/London, aka. BST, aka. +0100

@ianlancetaylor ianlancetaylor changed the title `time.Parse` inappropriately sets Location() to Local in some circumstances time: Parse inappropriately sets Location() to Local in some circumstances Mar 28, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.9 milestone Mar 28, 2017

@bradfitz bradfitz added the NeedsFix label May 24, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 24, 2017

I can reproduce on Linux or Mac, with:

$ TZ=Europe/London go run repro.go
UTC: 0x528aa0 UTC
Local: 0x52ae80 Europe/London
tl1: Format: "+0000" Location: 0x52ae80 Europe/London
tl2: Format: "+0000" Location: 0xc42009c060
@bradfitz

This comment has been minimized.

Copy link
Member

commented May 24, 2017

Here's the relevant code:

        if zoneOffset != -1 {  
                t := Date(year, Month(month), day, hour, min, sec, nsec, UTC)  
                t.addSec(-int64(zoneOffset))  
  
                // Look for local zone with the given offset.                                                                                                             
                // If that zone was in effect at the given time, use it.                                                                                                  
                name, offset, _, _, _ := local.lookup(t.unixSec())  
                if offset == zoneOffset && (zoneName == "" || name == zoneName) {  
                        t.setLoc(local)  
                        return t, nil  
                }  
  
                // Otherwise create fake zone to record offset.                                                                                                           
                t.setLoc(FixedZone(zoneName, zoneOffset))  
                return t, nil  
        }  

It's trying to map the timezone to your local timezone if your local timezone was applicable for that time. In this case, it's looking to see whether Unix time -62167219200 (year 0) was a valid British Summer Time. It then fails to find a valid timezone at year 0 and returns some zero values (including offset == 0), and then helpfully matches the zone you specified! So it matches your local timezone.

Looking at a fix.

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 24, 2017

More particularly, Unix time -62167219200 in Europe/London was GMT, but somewhere before Unix time 0 (sometime in the following 1970 years), daylight savings time was invented and it was now British Summer Time, with offset 3600 seconds forward, which didn't match your +0000, so your local time zone didn't apply then.

I'm tempted to ignore this rule before December 1, 1847-ish, when time zones first started being used. Or I'll see what the earliest recorded zone in the Olson database is. Oh, that's just from 1970. Easy enough.

@gopherbot

This comment has been minimized.

Copy link

commented May 24, 2017

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 24, 2017

Moving discussion from the CL back to the issue.

I said:

It's not entirely obvious to me that the behavior reported in the issue is wrong. It does seem to match the documentation of time.Parse.

@bradfitz replied:

The docs say "if the offset corresponds to a time zone used by the current location (Local)", so the question is what the definition of "corresponds" is. I'd argue that in year 0, there weren't timezones yet, so +0000 mapping to Local because the user today is in London is kinda weird.

To which I say:

Yes, it is weird, but an arbitrary cutoff at a point in historical time is also weird, whether the cutoff is 1970 (start of Unix time) or 1847 (invention of timezones).

I think the behavior of time.Parse in trying to set the location to time.Local when possible is kind of weird, but maybe I just don't get it.

But the current behavior does seem to match the docs, so if we change this, we should change the docs. And if we approach it from that perspective, what do we want the docs to say?

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 24, 2017

Yeah, the more I think about it, the more I dislike an arbitrary cutoff and more docs too.

The current behavior is documented and there's a workaround for people who really care: time.ParseInLocation.

I'm happy closing this.

@bradfitz bradfitz closed this Jun 7, 2017

@golang golang locked and limited conversation to collaborators Jun 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.