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

testing/quick: quick.Value should check if a struct field can be set before doing so #27017

Open
minond opened this issue Aug 16, 2018 · 2 comments · May be fixed by #27018

Comments

@minond
Copy link

commented Aug 16, 2018

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

go version go1.10.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/marcosmindon/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/marcosmindon/code/go"
GORACE=""
GOROOT="/Users/marcosmindon/.go1.10.1"
GOTMPDIR=""
GOTOOLDIR="/Users/marcosmindon/.go1.10.1/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
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/wk/2ffhr3wn0qq4t3rrrl4pr1mh0000gp/T/go-build311818715=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Using quick.Value to create a struct with randomly generated values, however a panic occurs if the function tries to set a field it cannot write to, such as an un-exported field. See https://play.golang.org/p/IlQgp7vxDk8 for an example.

What did you expect to see?

I would expect un-exported fields to be ignored and a struct to be successfully generated.

What did you see instead?

reflect.Value.Set is called on a field that cannot be set and a panic is triggered.

@ALTree ALTree changed the title quick.Value should check if a struct field can be set before doing so testing/quick: quick.Value should check if a struct field can be set before doing so Aug 16, 2018

@ALTree ALTree added this to the Unplanned milestone Aug 16, 2018

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

This limitation is explicitly documented in the Value docs:

Note: To create arbitrary values for structs, all the fields must be exported.

so this probably wasn't an oversight. It would be interesting to understand if this limitation was introduced for some "deep" reason, and if it is effectively safe to remove. Digging into the git history didn't help, the change is very old (878d0e1) and I couldn't even find a page with the review comments.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

I think it would be misleading for testing/quick to simply ignore unexported fields. That would produce the misleading impression that the struct was being tested when it fact it is not.

It would be nicer to return an error if possible rather than panic, but I don't think we should start ignoring unexported fields. testing/quick is not a general purpose tool, and to be honest it probably shouldn't be in the standard library. It's a specific tool for specific purposes, and for structs it requires all fields to be exported.

The more general version of testing/quick is fuzzing, and discussion about that is over at #19109.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.