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

Open
bep opened this issue May 31, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@bep
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

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 this to the Go1.13 milestone May 31, 2019

@bep

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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.

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