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: Parsing of GMT+XX offsets is broken #56392

Open
jwebb opened this issue Oct 24, 2022 · 3 comments
Open

time: Parsing of GMT+XX offsets is broken #56392

jwebb opened this issue Oct 24, 2022 · 3 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jwebb
Copy link

jwebb commented Oct 24, 2022

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

1.19.1

Does this issue reproduce with the latest release?

Yes

What did you do?

https://go.dev/play/p/e5jcM1fIJm9?v=gotip (case 4 is the bug)

What did you expect to see?

GMT+2, if it is parsed at all, ought to be treated the same as +0200 (or -0200, according to Posix).

What did you see instead?

It causes the time to be parsed as UTC, but be labelled as though an offset were applied.

When applying a known zone abbreviation the time is shifted at https://cs.opensource.google/go/go/+/refs/tags/go1.19.2:src/time/format.go;l=1365;drc=85196fc982ead65ea56c377c2e381eabff329773;bpv=0;bpt=1 - it seems the same should be done at L1373.

However, this GMT+XX format parsing has a bunch of special-case code, supported by a single test, which checks the wrong input, and it's been this way forever without anyone noticing apparently. And as noted in #40472, it's undocumented and interacts confusingly with the ISO/RFC822 style. Further, as noted in my case 3 above, the direction of the offset in this notation has conflicting conventions. So I'd suggest just removing this from the parser entirely.

(I encountered this only because I was reading the code looking for a way to detect my case 5 and turn it into an error - which sadly seems impossible in general without separately parsing zoneinfo.)

@bcmills
Copy link
Contributor

bcmills commented Oct 24, 2022

(CC @dsnet, who has been cleaning up time parsing lately)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 24, 2022
@bcmills bcmills added this to the Backlog milestone Oct 24, 2022
@bcmills
Copy link
Contributor

bcmills commented Oct 24, 2022

Looking at the layout documentation in https://pkg.go.dev/time#pkg-constants, I don't see any description of what actual format the MST abbreviation is supposed to match, but it seems that this case was added explicitly in CL 12922043 (CC @robpike), so presumably it should be fixed to use the correct offset instead of treating GMT+2 as an error.

@jwebb
Copy link
Author

jwebb commented Oct 25, 2022

And another case - numeric offsets without the GMT prefix work differently again: time.Parse("2006-01-02 15:04:05 MST", "2022-08-24 00:00:00 +02") is accepted as valid but the +02 is treated the same as XXX, with no offset recorded. Also, +2 is surprisingly not accepted.

So to expand this issue: +0200 (using Z0700 format), and GMT+2 and +02 (using MST format) all ought logically to do the same thing, but in fact do three different things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants