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

go/ast: mention strconv.Unquote in BasicLit documentation #39590

Closed
tenntenn opened this issue Jun 15, 2020 · 7 comments
Closed

go/ast: mention strconv.Unquote in BasicLit documentation #39590

tenntenn opened this issue Jun 15, 2020 · 7 comments

Comments

@tenntenn
Copy link
Contributor

@tenntenn tenntenn commented Jun 15, 2020

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

$ go version
go version go1.14.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tenntenn/Library/Caches/go-build"
GOENV="/Users/tenntenn/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/tenntenn/Documents/gopath"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wm/76fqhrc52ls25p4cmjrjbdqh0000gp/T/go-build969893907=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://golang.org/pkg/go/ast/#BasicLit

What did you expect to see?

I propose improve the document of ast.BasicLit.

When BasicLit.Kind is token.STRING, we must call strconv.Unquote to get unquoted string value.

So I think the document of ast.BasicLit should refer to strconv.Unquote.

What did you see instead?

https://golang.org/pkg/go/ast/#BasicLit

@gopherbot gopherbot added this to the Proposal milestone Jun 15, 2020
@ALTree
Copy link
Member

@ALTree ALTree commented Jun 15, 2020

Thanks for the report.

A note from the Proposal Process doc:

Significant changes to the language, libraries, or tools must be first discussed, and sometimes formally documented, before they can be implemented.

Stress on "significant changes". I don't think a suggestion like this one has to go through the heavyweight Proposal Process, so I've changed this to a regular issue with a needsDecision label.

@ALTree ALTree changed the title proposal: go/ast: improve document of ast.BasicLit go/ast: improve document of ast.BasicLit Jun 15, 2020
@ALTree ALTree removed the Proposal label Jun 15, 2020
@ALTree ALTree removed this from the Proposal milestone Jun 15, 2020
@ALTree ALTree changed the title go/ast: improve document of ast.BasicLit go/ast: mention strconv.Unquote in BasicLit documentation? Jun 15, 2020
@tenntenn
Copy link
Contributor Author

@tenntenn tenntenn commented Jun 15, 2020

@ALTree
I got it! Thank you.

@mvdan
Copy link
Member

@mvdan mvdan commented Jun 15, 2020

The strconv docs do bring this up:

Unquote and UnquoteChar unquote Go string and rune literals.

However, I agree that it's not really an answer, because many developers new to go/ast will not know the strconv package all that well. A quick mention in the ast package sounds good to me.

@mvdan
Copy link
Member

@mvdan mvdan commented Jun 15, 2020

I think you want to mention Unquote for string literals, UnquoteChar for rune literals, ParseInt for integer literals, and ParseFloat for floating point literals. I think that covers all the basic literal kinds, minus IMAG, which I'm not sure about.

@ALTree
Copy link
Member

@ALTree ALTree commented Jun 15, 2020

you want to mention Unquote for string literals, UnquoteChar for rune literals, ParseInt for integer literals, and ParseFloat for floating point literals.

This could be okay for completeness' sake but to add a datapoint: I was bitten a couple times by the fact that, for strings, Value holds "aaaa" including the " chars; on the other hand I never had a problem with e.g. ints.

Mostly because you see that the interface holds a string, so you imagine that for an int you'll get a string with the number in it: 1, and you'll have to call ParseInt to get a Go value; on the other hand for strings we have this gotcha that the literal is "aaaa", so go/ast will return a 6-chars string with "s in it.

So just documenting the string-literal special case with a suggestion to call Unquote would help, even if we don't do the same for all the other types. But helping with all of them is fine, too.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 27, 2020

Change https://golang.org/cl/244960 mentions this issue: go/ast: note that in BasicLit CHARs and STRINGs are quoted

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 6, 2020

Agreed. We don't need to be exhaustive here. Will review now.

@ALTree ALTree changed the title go/ast: mention strconv.Unquote in BasicLit documentation? go/ast: mention strconv.Unquote in BasicLit documentation Sep 6, 2020
@ALTree ALTree added NeedsFix and removed NeedsDecision labels Sep 6, 2020
@gopherbot gopherbot closed this in 5cc030a Sep 6, 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.