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

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 Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. Proposal v2 A language change or incompatible library change
Milestone
Proposal

Comments

@dsnet
Copy link
Member

dsnet commented Jan 31, 2017 •
edited

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

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

gopherbot pushed a commit that referenced this issue Feb 1, 2017
@dsnet
reflect: adjust documentation on Value
048b8ce 
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

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 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 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 v2 A language change or incompatible library change label Feb 1, 2017
@bradfitz
Copy link
Contributor

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 dominikh mentioned this issue Feb 2, 2017
Open
@dominikh
Copy link
Member

dominikh commented Feb 2, 2017 •
edited

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 modified the milestone: Unplanned 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
@paulzhol
compare NameMapper and origMapper properly
4db2bce 
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
@anthonyfok anthonyfok mentioned this issue Jul 24, 2017
Merged
@ianlancetaylor ianlancetaylor added the Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. label Jan 23, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Jan 23, 2018
@dominikh dominikh mentioned this issue Jan 30, 2021
Open
@gopherbot
Copy link

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
@randall77
go/tools: add vet check for direct comparion of reflect.Values
b261fe9 
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>
@dsnet dsnet mentioned this issue Apr 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. Proposal v2 A language change or incompatible library change
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
5 participants
@bradfitz @dominikh @ianlancetaylor @dsnet @gopherbot