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

strconv: document exact grammar of Parse{Float,Int,Uint} #31197

Open
dsnet opened this issue Apr 1, 2019 · 5 comments
Open

strconv: document exact grammar of Parse{Float,Int,Uint} #31197

dsnet opened this issue Apr 1, 2019 · 5 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Apr 1, 2019 •
edited

strconv.Parse{Float,Int,Uint} is frequently used in the implementation of other grammars (e.g., JSON), which may be a subset or superset of what strconv.ParseX currently does today. However, what strconv.Parse does today is not well-specified. What exactly is the input grammar that is accepted?

For example, the documentation for ParseFloat only says "If s is well-formed", but does not specify what "well-formed" means. I suspect that the answer to this is whether the input is well-formed according to the grammar in the Go specification. If so, this needs to be documented.

Furthermore, this also opens the question for whether the input grammar is stable. Apparently, this is not the case since strconv.ParseX was recently augmented to support the new number literal grammars (CL/160241 and CL/160244). This change is going to silently break a number of use-cases that were assuming that the input of ParseX was constrained to the grammar prior to Go1.13. If so, the instability of the input grammar should also be documented.

(BTW, I think the change in semantics is entirely reasonable and I support it; but I see a lot of code silently broken by this change. Documenting this better would help dissuade future abuse.)

\cc @cybrcodr, @griesemer, @rsc
@dsnet dsnet added this to the Go1.13 milestone Apr 1, 2019
@andybons andybons changed the title strconv: document exact grammar of Parse{Float,Int,Uint} proposal: strconv: document exact grammar of Parse{Float,Int,Uint} Apr 3, 2019
@griesemer
Copy link
Contributor

The proposed Go 1.13 number literal changes are language changes, obviously the accepted grammar of the respective strconv routines had to be adjusted. Though for the integer parsing functions, the more flexible grammar (underscores) is only permitted if the base argument is set to 0.

Well-formed here does mean it follows the Go language spec for these literals.

@dsnet
Copy link
Member Author

dsnet commented Apr 3, 2019

@andybons, I'm a little confused. Is there a reason this is marked as a proposal?

obviously the accepted grammar of the respective strconv routines had to be adjusted

The fact that strconv follows the Go specification was not obvious at all to me. The misuse of this function inside Google seems to suggest that this is not obvious to users also.

the more flexible grammar (underscores) is only permitted if the base argument is set to 0.

Behavior of like this is surprising and all the more shows that the documentation around these functions is lacking.

@griesemer
Copy link
Contributor

@dsnet I've been imprecise: The integer parse routines, if they specify a non-zero base, do not follow the language spec (since the language spec doesn't pemit, say a hex literal without the 0x prefix). They also haven't changed. What has changed is the integer parse routines for base 0, and the float parse routines.

In any case, this shouldn't be a proposal (I fixed that). This is a just a documentation issue.

@griesemer griesemer changed the title proposal: strconv: document exact grammar of Parse{Float,Int,Uint} strconv: document exact grammar of Parse{Float,Int,Uint} Apr 3, 2019
@griesemer griesemer removed the Proposal label Apr 3, 2019
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gopherbot
Copy link

Change https://golang.org/cl/191078 mentions this issue: strconv: document _ grouping in digits for Parse(Float, *Int)

gopherbot pushed a commit that referenced this issue Aug 22, 2019
@odeke-em @griesemer
strconv: update documentation
d9b1323 
Fixes #33750.
Updates #31197.

Change-Id: I26f63cef57e5f0eec85b84554c82f6d47b4f41a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/191078
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/191168 mentions this issue: [release-branch.go1.13] strconv: update documentation

gopherbot pushed a commit that referenced this issue Aug 22, 2019
@odeke-em @andybons
[release-branch.go1.13] strconv: update documentation
bd2e281 
Fixes #33750.
Updates #31197.

Change-Id: I26f63cef57e5f0eec85b84554c82f6d47b4f41a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/191078
Reviewed-by: Robert Griesemer <gri@golang.org>
(cherry picked from commit d9b1323)
Reviewed-on: https://go-review.googlesource.com/c/go/+/191168
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@odeke-em @t4n6a1ka
strconv: update documentation
1e7c7ab 
Fixes golang#33750.
Updates golang#31197.

Change-Id: I26f63cef57e5f0eec85b84554c82f6d47b4f41a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/191078
Reviewed-by: Robert Griesemer <gri@golang.org>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@rsc @andybons @bcmills @dsnet @griesemer @gopherbot