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: regression in time.Parse from Go 1.11 to 1.12 #32358

Closed
bep opened this issue May 31, 2019 · 8 comments
Closed

time: regression in time.Parse from Go 1.11 to 1.12 #32358

bep opened this issue May 31, 2019 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bep
Copy link
Contributor

bep commented May 31, 2019

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

package main

import (
	"log"
	"time"
)

func main() {

	// The date string is on time.RubyDate format
	_, err := time.Parse(time.UnixDate, "Fri Jan 01 03:01:00 +0000 2016")
	if err == nil {
		log.Fatal("no error")
	}
}

The above runs fine on Go 1.11 (time.Parse returns an error), but fails in Go 1.12 and later.

@ianlancetaylor
Copy link
Contributor

I think that this is due to htps://golang.org/cl/130696 which was part of the fixes for #26032.

It's not obvious to me that the new behavior is wrong. It's a change, but the resulting date does seem to match the input. Why is this wrong?

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 31, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone May 31, 2019
@bep
Copy link
Contributor Author

bep commented May 31, 2019

Why is this wrong?

I found this while debugging an issue in a library I didn't write. This library depends on the error behavior of time.Parse. Which is also the best answer I can provide to the existential "why".

But to me, Fri Jan 01 03:01:00 +0000 2016 does not match the layout Mon Jan _2 15:04:05 MST 2006. I don't see how the numeric offset +0000 be seen as a timezone abbreviation.

@ALTree
Copy link
Member

ALTree commented May 31, 2019

We accept numeric-looking zone names when matching against MST because not every timezone has a three-letter abbreviation. Many timezones has an abbreviation that looks like a number.

If this works:

time.Parse(time.UnixDate, "Fri Jan 01 03:01:00 +03 2016")

then why it shouldn't with +00 ?

And if we accept +00 we should also accept +0000, since the 4-digits format for the numeric abbreviated timezone name is used for timezones with non-integral UTC offset.

@bep
Copy link
Contributor Author

bep commented May 31, 2019

then why it shouldn't with +00 ?

+00 is in none of my examples. To be clear, I would expect this to return an error:

package main

import (
	"log"
	"time"
)

func main() {
	_, err := time.Parse(time.UnixDate, time.Now().UTC().Format(time.RubyDate))
	if err == nil {
		log.Fatal("no error")
	}
}

As it is now you have a situation where time.Parse errors depending on if you're on holiday somewhere or not (+0000 works, +0002 does not).

I understand the complexity of time and timezones and appreciate all the work that has been done on this. I'm just reporting a real issue found in the wild, I'm not saying it has to be fixed.

@ALTree
Copy link
Member

ALTree commented May 31, 2019

I would expect this to return an error

I don't think it should. It uses +0000 and we'll need to accept timezones abbreviations of form +XXXX going forward, to fix the aforementioned #26032. Otherwise we would be rejecting valid abbreviations like +0430 which is the Asia/Kabul one.

As it is now you have a situation where time.Parse errors depending on if you're on holiday somewhere or not (+0000 works, +0002 does not).

You're right, but my point is that we should accept both (not error on both); so what we do now on +0000 is correct and the fact that we reject things like +0430 is #26032, which we should fix sooner or later.

So in the end I think your snippet is WAI and the inconsistency between +0000 and +0430 is #26032.

@bep
Copy link
Contributor Author

bep commented May 31, 2019

@ALTree I understand the complexity, but I still think it's the wrong thing to mix timezone abbreviations with timezone offsets. Esp. when you see who they are treated so fundamentally different in time.Parse.

@ALTree
Copy link
Member

ALTree commented May 31, 2019

it's the wrong thing to mix timezone abbreviations with timezone offsets

Do you have a concrete suggestion on how to handle matching MST with timezones abbreviations?

Suppose I have this format string:

Mon Jan 2 15:04:05 2006 MST

Now I zdump the current time in Rome and Singapore:

$ zdump Europe/Rome Asia/Singapore

Europe/Rome     Fri May 31 23:17:35 2019 CEST
Asia/Singapore  Sat Jun  1 05:17:35 2019 +08

Should the Singapore timestamp be rejected by Parse? But then we're back at square 1: the timestamp is accepted or not depending on where we are.

"MST" doesn't mean "a three letter abbreviation". It just means "whatever the tzdatabase uses as a shorthand for the timezone name". If the shorthand for "Asia/Singapore" is +08, I don't think we can reject it.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@ALTree
Copy link
Member

ALTree commented Sep 7, 2020

For the reason outlined in #32358 (comment) I don't think this is a bug. Closing this issue.

@ALTree ALTree closed this as completed Sep 7, 2020
@golang golang locked and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

6 participants