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: bugs in parsing time string with GMT as time zone. #40472

Open
leafbebop opened this issue Jul 29, 2020 · 4 comments
Open

time: bugs in parsing time string with GMT as time zone. #40472

leafbebop opened this issue Jul 29, 2020 · 4 comments
Milestone

Comments

@leafbebop
Copy link

@leafbebop leafbebop commented Jul 29, 2020

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

Go Playground,
Go 1.14.6

Does this issue reproduce with the latest release?

Yes

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

go env Output
Go Playground.

What did you do?

https://play.golang.org/p/UJyx6fMkbJW

var (
	tests = [][2]string{
		{"Tue Jun 11 2019 13:26:45 GMT+08", "Mon Jan 02 2006 15:04:05 MST-07"},
		{"Tue Jun 11 2019 13:26:45 GMT+0800", "Mon Jan 02 2006 15:04:05 MST-0700"},
		{"Tue Jun 11 2019 13:26:45 GMT+0000", "Mon Jan 02 2006 15:04:05 MST-0700"},
		{"Tue Jun 11 2019 13:26:45 MST+0000", "Mon Jan 02 2006 15:04:05 MST-0700"},
		{"Tue Jun 11 2019 13:26:45 MST+08", "Mon Jan 02 2006 15:04:05 MST-07"},
	}
)

func demo() {
	for _, vl := range tests {
		value, layout := vl[0], vl[1]
		_, err := time.Parse(layout, value)
		fmt.Printf("time.Parse: %q - error: %v\n", value, err)
	}
	fmt.Println("--------------")
}

What did you expect to see?

All tests passed without error.

What did you see instead?

The GMT ones except "GMT+0800" failed with cannot parse "" as "-0700" or cannot parse "" as "-07".

Details:

When parsing time zones, the Parse function calls parseTimeZone, which makes a special case for parsing GMT time zone (which I don't understand why), calling and returning results from parseGMT, as bytes to consume by Parse function.

go/src/time/format.go

Lines 1047 to 1059 in 85afa2e

case stdTZ:
// Does it look like a time zone?
if len(value) >= 3 && value[0:3] == "UTC" {
z = UTC
value = value[3:]
break
}
n, ok := parseTimeZone(value)
if !ok {
err = errBad
break
}
zoneName, value = value[:n], value[n:]

go/src/time/format.go

Lines 1209 to 1212 in 85afa2e

if value[:3] == "GMT" {
length = parseGMT(value)
return length, true
}

The parseGMT function tries to return more than 3 bytes, which is the "GMT" string, as in other time zone format. You can see in the playground link, that it returns 8 bytes for "GMT+0000", and leaving nothing to match with "-0700" in the layout string.

go/src/time/format.go

Lines 1247 to 1280 in 85afa2e

// parseGMT parses a GMT time zone. The input string is known to start "GMT".
// The function checks whether that is followed by a sign and a number in the
// range -23 through +23 excluding zero.
func parseGMT(value string) int {
value = value[3:]
if len(value) == 0 {
return 3
}
return 3 + parseSignedOffset(value)
}
// parseSignedOffset parses a signed timezone offset (e.g. "+03" or "-04").
// The function checks for a signed number in the range -23 through +23 excluding zero.
// Returns length of the found offset string or 0 otherwise
func parseSignedOffset(value string) int {
sign := value[0]
if sign != '-' && sign != '+' {
return 0
}
x, rem, err := leadingInt(value[1:])
// fail if nothing consumed by leadingInt
if err != nil || value[1:] == rem {
return 0
}
if sign == '-' {
x = -x
}
if x < -23 || 23 < x {
return 0
}
return len(value) - len(rem)
}

Interestingly (and confusingly) it does so by checking whether the following string is a signed integer which is in range from -23 to +23, excluding leading zeroes - but it considers "+0800" as 800, which makes it invalid, so parseGMT returns 3, which makes Parse functions without an error.

However, all other values showed in the tests ("+0000", "+08") are considered valid, and making the Parse function consumes too much.

@ulikunitz
Copy link
Contributor

@ulikunitz ulikunitz commented Jul 29, 2020

The layout MST matches GMT-3, GMT-03 or GMT-003. GMT-03 is the time zone with a difference in hours from GMT.

The layout MST-0700 requests two time zones and according to the documentation the numeric value has precedence. So your examples would be correct with GMT+08+0800 for layout MST-0700.

I fixed your examples in https://play.golang.org/p/ZxqWuhnEI28

@leafbebop
Copy link
Author

@leafbebop leafbebop commented Jul 29, 2020

@ulikunitz I was unaware of "MST" and "-0700" is redundant, that part of code makes much more sense now.

However, this behaviour of GMT format is really confusing and works in an unintended way most of the time, (as "+/-0X00" where X is not zero is dominant (mis-)use-case). And I am not sure if any user would expect it to work that way. (I am not sure about what to expect either, for I rarely touch time zones.)

I think at least we need a documentation of how GMT format is parsed.

EDIT:

And maybe make a vet warning about cases of "GMT+0800"? Although the parsed value is "correct", time.Time.Zone() will return "GMT" instead of "GMT+08", which is a difference.

@ulikunitz
Copy link
Contributor

@ulikunitz ulikunitz commented Jul 29, 2020

I believe the author of the time package must decide, whether he wants to document the GMT behavior.

It should also be noted that Go doesn't handle time zone abbreviations unless it is the abbreviation of the local time zone or the location provided to ParseInLocation. If that is not the case the time is always interpreted as UTC. Note that GMT+X is actually handled correctly because it gives a difference to UTC:

So numerical offsets should always be preferred over time zone abbreviations as in RFC3339 / ISO 8601, which is the official International Standard to write times.

In general the term UTC (Coordinated Universal Time) should be used instead of GMT, because GMT is ambiguous. GMT is used in navigation for UT0 and not UTC and astronomers calculated GMT starting at noon in the past. UTC is comparison is precisely defined.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Jul 30, 2020

/cc @robpike @ianlancetaylor for time.

@cagedmantis cagedmantis added this to the Backlog milestone Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.