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

proposal: reflect: make Value uncomparable #18871

Open
dsnet opened this issue Jan 31, 2017 · 7 comments
Open

proposal: reflect: make Value uncomparable #18871

dsnet opened this issue Jan 31, 2017 · 7 comments
Labels
Go2Cleanup Go2 Proposal
Milestone

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Jan 31, 2017

Comparing two Value type does not do what the user expects. The type is documented as such:

Using == on two Values does not compare the underlying values they represent, but rather the contents of the Value structs. To compare two Values, compare the results of the Interface method.

However, it is still common to see users accidentally make this mistake. What's worse is that it "seems to work", giving a false sense of security.

s1, s2 := "hello", "goodbye"

v1a := reflect.ValueOf(&s1)
v1b := reflect.ValueOf(&s1)
v2 := reflect.ValueOf(&s2)

fmt.Println(v1a == v1a) // Prints true
fmt.Println(v1a == v1b) // Prints true
fmt.Println(v1a == v2) // Prints false

This is because the Value struct contains a flag field whose value may be different for each Value and is entirely an implementation detail of the reflect package.

Perhaps we should add a uncomparable type to prevent this misuse:

type Value struct {
	_ [0]func() // Prevents Value from being comparable and occupies 0 bytes
	typ *rtype
	ptr unsafe.Pointer
	flag
}

\cc @ianlancetaylor

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 31, 2017

CL https://golang.org/cl/36018 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 1, 2017
Make the documentation more explicit that it is not safe to directly
compare Value. Get straight to the point on how to do it correctly.

Updates #18871

Change-Id: I2aa3253f779636b2f72a1aae8c9bb45d3c32c902
Reviewed-on: https://go-review.googlesource.com/36018
Reviewed-by: Keith Randall <khr@golang.org>
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 1, 2017

Adding [0]func() is a clever idea, but unfortunately it breaks the Go 1 guarantee. We can't actually modify reflect.Value so that == does not work, as that will cause currently working programs to stop compiling.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 1, 2017

What @ianlancetaylor said. It would break things like collecting a set of reflect.Values in a map.

@dsnet
Copy link
Member Author

@dsnet dsnet commented Feb 1, 2017

It would break things like collecting a set of reflect.Values in a map.

Is that so bad? It's funny that you bring up reflect.Values in a map. That was exactly what someone tried doing the other day and it "seemed to work".

However, I do agree that we probably can't do this in Go 1.

@dsnet dsnet added the Go2 label Feb 1, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 1, 2017

A map is fine in some cases. (e.g. using it as a set of things to process, and ranging over it to get the reflect.Values back)

Maybe worth a static analysis checker?

@dominikh
Copy link
Member

@dominikh dominikh commented Feb 2, 2017

In the subset of the Go corpus v0.01 that actually compiles, there are 33 instances of using == or != with two reflect.Value.

Of these, 12 compare against the zero value reflect.Value{}, as it is returned by, for example, MethodByName. 12 compare a reflect.Value against the result of reflect.Zero. The remaining 9 compare two generic reflect.Values.

Of the 21 incorrect comparisons, 5 compare reflect.Values derived from function values, which would not be comparable if converted to interface values first.

I've attached the list of matches. [fn] denotes a comparison of functions, [zero] denotes a comparison with the zero value, [zv] denotes a comparison with a value produced by reflect.Zero, and the rest are normal, though sometimes oddly written, comparisons.

[fn] github.com/Masterminds/glide/vendor/github.com/Masterminds/semver/constraints_test.go:47:9
[zero] github.com/cyfdecyf/cow/config.go:320:12
[zero] github.com/cyfdecyf/cow/config.go:348:12
[zero] github.com/cyfdecyf/cow/config.go:629:13
[zero] github.com/docker/docker/vendor/github.com/imdario/mergo/map.go:70:18
[zv] github.com/docker/machine/vendor/github.com/Azure/go-autorest/autorest/validation/validation.go:313:7
[zv] github.com/docker/machine/vendor/github.com/Azure/go-autorest/autorest/validation/validation.go:333:7
[zero] github.com/drone/drone/vendor/gopkg.in/gorp.v1/gorp.go:1763:7
[zero] github.com/ethereum/go-ethereum/vendor/github.com/robertkrimen/otto/type_go_slice.go:71:78
[zero] github.com/ethereum/go-ethereum/vendor/github.com/robertkrimen/otto/type_go_struct.go:41:70
[zero] github.com/ethereum/go-ethereum/vendor/github.com/robertkrimen/otto/type_go_struct.go:45:55
[fn] github.com/git-lfs/git-lfs/vendor/github.com/spf13/cobra/cobra_test.go:949:31
[zv] github.com/go-xorm/xorm/statement.go:418:19
[zv] github.com/go-xorm/xorm/statement.go:655:18
github.com/google/cadvisor/vendor/github.com/stretchr/testify/assert/assertions.go:41:19
github.com/google/cadvisor/vendor/github.com/stretchr/testify/assert/assertions.go:46:77
github.com/iris-contrib/formBinder/formBinder.go:40:14
github.com/iris-contrib/formBinder/formBinder.go:570:33
[zero] github.com/kataras/go-serializer/vendor/github.com/imdario/mergo/map.go:70:18
[zero] github.com/kataras/go-websocket/vendor/github.com/imdario/mergo/map.go:70:18
[zv] github.com/mitchellh/packer/vendor/github.com/gophercloud/gophercloud/params.go:61:46
[zv] github.com/mitchellh/packer/vendor/github.com/gophercloud/gophercloud/params.go:83:46
[zv] github.com/openshift/origin/vendor/github.com/Azure/go-autorest/autorest/validation/validation.go:313:7
[zv] github.com/openshift/origin/vendor/github.com/Azure/go-autorest/autorest/validation/validation.go:333:7
[zero] github.com/openshift/origin/vendor/github.com/imdario/mergo/map.go:70:18
[zv] github.com/prometheus/prometheus/vendor/github.com/Azure/go-autorest/autorest/validation/validation.go:313:7
[zv] github.com/prometheus/prometheus/vendor/github.com/Azure/go-autorest/autorest/validation/validation.go:333:7
[fn] github.com/sosedoff/pgweb/vendor/github.com/jmoiron/sqlx/sqlx.go:37:23
[fn] github.com/spf13/cobra/cobra_test.go:1135:31
[fn] github.com/tj/cobra/cobra_test.go:1136:31
[zero] k8s.io/heapster/vendor/github.com/imdario/mergo/map.go:70:18
[zv] k8s.io/heapster/vendor/github.com/rikatz/goryman/marshal.go:30:29
[zv] k8s.io/heapster/vendor/github.com/rikatz/goryman/marshal.go:93:29

@dsnet dsnet assigned dsnet and unassigned dsnet Feb 2, 2017
@bradfitz bradfitz added this to the Unplanned milestone Mar 21, 2017
@bradfitz bradfitz added this to the Unplanned milestone Mar 21, 2017
@rsc rsc changed the title reflect: make Value uncomparable proposal: reflect: make Value uncomparable Jun 17, 2017
paulzhol added a commit to paulzhol/sqlx that referenced this issue Jul 21, 2017
https://golang.org/pkg/reflect/#Value:
    Using == on two Values does not compare the underlying values they represent,
    but rather the contents of the Value structs.
    To compare two Values, compare the results of the Interface method.

See also golang/go#18871
@ianlancetaylor ianlancetaylor added the Go2Cleanup label Jan 23, 2018
@ianlancetaylor ianlancetaylor removed this from the Unplanned milestone Jan 23, 2018
@ianlancetaylor ianlancetaylor added this to the Proposal milestone Jan 23, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 7, 2021

Change https://golang.org/cl/308209 mentions this issue: go/tools: add vet check for direct comparion of reflect.Values

gopherbot pushed a commit to golang/tools that referenced this issue Apr 8, 2021
Comparing reflect.Values directly is almost certainly not what you want.
That compares reflect's internal representation, not the underlying
value it represents. One compares reflect.Values correctly by comparing
the results of .Interface() calls.

Fixes golang/go#43993
Update golang/go#18871

Change-Id: I6f441d920a5089bd346e5431032780403cefca76
Reviewed-on: https://go-review.googlesource.com/c/tools/+/308209
Trust: Keith Randall <khr@golang.org>
Trust: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Keith Randall <khr@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go2Cleanup Go2 Proposal
Projects
None yet
Development

No branches or pull requests

5 participants