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: ParseDuration fails to parse values consisting of multiple zeros #14209

Closed
iand opened this issue Feb 3, 2016 · 11 comments
Closed

time: ParseDuration fails to parse values consisting of multiple zeros #14209

iand opened this issue Feb 3, 2016 · 11 comments

Comments

@iand
Copy link
Contributor

@iand iand commented Feb 3, 2016

time.ParseDuration returns an error when passed a string consisting of more than one zero but no unit: "time: missing unit in duration 00"

See http://play.golang.org/p/_UklkYQ0I1

I'd expect this to have the same behaviour as parsing a single zero, i.e. return a duration of 0 and no error. Leading zeros are ignored as expected when the supplied string has a unit.

go version go1.5.2 linux/amd64

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Feb 4, 2016
@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 4, 2016

It looks like the issue is here and here.

Zero alone is a special case, and there's no further parsing done for it. All the real parsing requires a format. The simplest fix would be at the second location to add a check if the captured number is equal to zero.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 4, 2016

Hmm, flipside of how the parser works, it accepts crazy values like 3s1m2s: http://play.golang.org/p/kcLlE4bX7p

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 4, 2016

Is accepting values like 1s1s1s1s considered part of the Go 1.0 API guarantee? If not, it would be fun to rewrite ParseDuration to do proper lexing/parsing with a documented format.

@minux
Copy link
Member

@minux minux commented Feb 4, 2016

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 4, 2016

:-(

I just got done typing up an EBNF:

duration = [sign] hour
sign = "-" | "+"
hour = number "h" [minute] | minute
minute = number "m" [second] | second
second = number "s" [milli] | milli
milli = number "ms" [] | 
micro = number microsign [nano] | nano
microsign = "us" | "μs"
nano = number "ns" | zero
zero = { "0" } [ "." { "0" } ]
number = { digit } [ "."  { digit } ]
digit = "0" | "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9"
@robpike
Copy link
Contributor

@robpike robpike commented Feb 5, 2016

If it's a bug, we can fix it. The value 00 not being the same as 0 seems like a bug to me, especially for a time value, where 00 is often seen.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 5, 2016

Okay. This seems like a pretty simple bug to fix. I've never submitted a Go patch before so I'll have to read all the contributor docs and whatnot first, but I'll try reading them this weekend and see how far I can get.

One question: should I also have it accept 0.0? Seems like we should for consistency with the other zero values we accept.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 5, 2016

Note to self, here's a minimal change that seems to fix it: http://play.golang.org/p/L36TxtuMcY

@minux
Copy link
Member

@minux minux commented Feb 6, 2016

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 8, 2016

CL https://golang.org/cl/19314 mentions this issue.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@quentinmit quentinmit added the NeedsFix label Oct 5, 2016
@rsc
Copy link
Contributor

@rsc rsc commented Oct 18, 2016

Based on CL 19314, I think this is not a bug.
Unitless "0" is a special case.
Let's stop the special there instead of letting it spread.

@rsc rsc closed this Oct 18, 2016
@golang golang locked and limited conversation to collaborators Oct 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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