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

Jump to bottom

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

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

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

ghost opened this issue Jul 29, 2020 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@ghost
Copy link

ghost 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 commented Jul 29, 2020 •
edited

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

@ghost
Copy link
Author

ghost commented Jul 29, 2020 •
edited by ghost

@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

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

/cc @robpike @ianlancetaylor for time.

@cagedmantis cagedmantis added this to the Backlog milestone Jul 30, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 30, 2020
@jwebb jwebb mentioned this issue Oct 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@cagedmantis @agnivade @ulikunitz