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

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

Open
dsnet opened this Issue Apr 1, 2019 · 3 comments

Comments

Projects
None yet
4 participants
@dsnet
Copy link
Member

dsnet commented Apr 1, 2019

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

@gopherbot gopherbot added the Proposal label Apr 3, 2019

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Apr 3, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

griesemer commented Apr 3, 2019

@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 label Apr 12, 2019

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.