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

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

Closed
minond opened this issue Aug 16, 2018 · 4 comments
Closed

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

minond opened this issue Aug 16, 2018 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Unplanned

Comments

@minond
Copy link

minond commented Aug 16, 2018 •
edited

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.
@minond minond mentioned this issue Aug 16, 2018
Closed
@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
Copy link
Member

ALTree 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.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 16, 2018
@ianlancetaylor
Copy link
Contributor

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.

@agnivade
Copy link
Contributor

agnivade commented Oct 8, 2019

Hi @minond - do you have any comments on Ian's feedback ? To add to what has been already said, the testing/quick package is frozen and is unlikely to modify its behavior.

@agnivade agnivade added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 8, 2019
@minond
Copy link
Author

minond commented Oct 12, 2019

@agnivade the reasoning makes sense, and these updates definitely belong elsewhere. Closing this issue. Thanks for reaching out!

@minond minond closed this as completed Oct 12, 2019
@golang golang locked and limited conversation to collaborators Oct 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Unplanned
Development

Successfully merging a pull request may close this issue.

testing/quick: skip struct fields that cannot be set by Value function
5 participants
@minond @agnivade @ianlancetaylor @ALTree @gopherbot