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

reflect: add Value.Equal, Value.Comparable #46746

Closed
ianlancetaylor opened this issue Jun 14, 2021 · 40 comments
Closed

reflect: add Value.Equal, Value.Comparable #46746

ianlancetaylor opened this issue Jun 14, 2021 · 40 comments
Assignees
Labels
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jun 14, 2021

In Go 1.17 we have introduced a conversion that can panic (#395). This is the first case in which a conversion can panic. This means that code that calls reflect.Type.ConvertibleTo and then, if that returns true, calls reflect.Value.Convert, can see an unexpected panic. (See #46730.)

Separately, for a long time now it has been possible for a comparison to panic, when comparing two interface values that have the same dynamic type but for which the dynamic type is not comparable. Therefore, for a long time code that calls reflect.Type.Comparable and then, if that returns true, uses the == operator can see an unexpected panic. (This is a fairly uncommon case as the problem only arises when working with indirectly accessed interface types, such as pointers to interfaces.)

I propose adding two new methods to reflect.Value.

// ConvertibleTo reports whether v can be converted to type t.
// If this reports true then v.Convert(t) will not panic.
func (v Value) ConvertibleTo(t Type) bool

// Comparable reports whether the type of v is comparable.
// If the type of v is an interface, this checks the dynamic type.
// If this reports true then v.Interface() == x will not panic for any x.
func (v Value) Comparable() bool
@josharian
Copy link
Contributor

josharian commented Jun 14, 2021

One very minor observation:

// If this reports true then v.Interface() == x will not panic for any x.

I'm not sure this is quite right. x could be an interface containing a dynamic type that isn't comparable. In the common case, comparing two reflect.Values, you need to call Comparable on both of them. I don't have better wording to suggest.

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Jun 14, 2021

I think the statement is still true, because if the dynamic types of v.Interface() and x are different, then the comparison is false, and it doesn't matter whether either or both of the dynamic types are not comparable. In other words, if the dynamic type of v.Interface() is comparable, then either x has a different dynamic type and the result of v.Interface() == x is false, or x has the same dynamic type and the comparison will be run without panicking.

@josharian
Copy link
Contributor

josharian commented Jun 14, 2021

Ah, indeed. Thanks. The relevant sentence from the spec is:

A comparison of two interface values with identical dynamic types causes a run-time panic if values of that type are not comparable.

@rsc
Copy link
Contributor

rsc commented Jun 16, 2021

We have:

func (v Value) Addr() Value
func (v Value) CanAddr() bool

func (v Value) Interface() interface{}
func (v Value) CanInterface() bool

So it sounds like we want to add the second one of these:

func (v Value) Convert(t Type) Value
func (v Value) CanConvert(t Type) bool

And maybe:

func (v Value) Equal(u Value) bool
func (v Value) Comparable(u Value) bool

Comparable seems like a better name than CanEqual here, but Equal seems better than Compare (compare bytes.Compare, bytes.Equal).

We probably want CanConvert at least for Go 1.17.

@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 16, 2021
@rsc
Copy link
Contributor

rsc commented Jun 16, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc changed the title proposal: reflect: add Value.ConvertibleTo(Value) and Value.Comparable(Value) proposal: reflect: add Value.Convert, Value.CanConvert, Value.Equal, Value.Comparable Jul 14, 2021
@rsc rsc changed the title proposal: reflect: add Value.Convert, Value.CanConvert, Value.Equal, Value.Comparable proposal: reflect: add Value.CanConvert, Value.Equal, Value.Comparable Jul 14, 2021
@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jul 14, 2021
@twmb
Copy link
Contributor

twmb commented Jul 14, 2021

If a Value is Comparable, does this mean the return from Interface() can be used as a key in a map? I don't think reflect currently has a way to answer this question.

(this is just a clarifying question, not a comment against the proposal)

@josharian
Copy link
Contributor

josharian commented Jul 14, 2021

We probably want CanConvert at least for Go 1.17.

Is this still true? The window for doing this is pretty small now.

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Jul 14, 2021

If a Value is Comparable, does this mean the return from Interface() can be used as a key in a map? I don't think reflect currently has a way to answer this question.

Assuming you have a map[interface{}]T (for some value type T), then yes: if v.Comparable() returns true, you can use v.Interface() as a key value for that map, and no panic will occur.

@gopherbot
Copy link

gopherbot commented Jul 14, 2021

Change https://golang.org/cl/334669 mentions this issue: reflect: add Value.CanConvert

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Jul 14, 2021

I sent https://golang.org/cl/334669 in case we do want this in 1.17.

@rsc
Copy link
Contributor

rsc commented Jul 21, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: reflect: add Value.CanConvert, Value.Equal, Value.Comparable reflect: add Value.CanConvert, Value.Equal, Value.Comparable Jul 21, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jul 21, 2021
gopherbot pushed a commit that referenced this issue Jul 21, 2021
For #395
For #46746

Change-Id: I4bfc094cf1cecd27ce48e31f92384cf470f371a6
Reviewed-on: https://go-review.googlesource.com/c/go/+/334669
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Sep 21, 2022

CC @rsc

@dsnet
Copy link
Member

dsnet commented Sep 21, 2022

I think it's fine to return false.

Seems a bit counter-intuitive that:

x := reflect.ValueOf([]int{1, 2, 3})
x.Equal(x) // reports false

It's unfortunate to lose the reflexive property of equality, but I guess that's already the case for NaNs.

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Sep 26, 2022

@dsnet Fair point. As far as I can tell the only other option is to panic, or to change the signature of Equal.

@bcmills
Copy link
Member

bcmills commented Sep 26, 2022

Given that the language itself panics for equality comparisons on incomparable types, I think it would make sense for Equal to panic for incomparable types, perhaps with a CanEqual method (akin to CanConvert, CanAddr, CanInterface, etc.) that can be used to easily avoid the panic.

@dsnet
Copy link
Member

dsnet commented Sep 26, 2022

Isn't Value.CanEqual more or less identical to Value.Comparable?

@bcmills
Copy link
Member

bcmills commented Sep 26, 2022

Maybe? I don't know: the proposed doc comment is “Comparable reports whether the type of v is comparable”, but for struct types whether Equal panics depends on the (deep) value, not just its (shallow) type.

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Sep 26, 2022

...which is why Comparable is a method on Value, not Type.

@go101
Copy link

go101 commented Sep 27, 2022

@ianlancetaylor

which is why Comparable is a method on Value, not Type.

It is actually the inverse?

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Sep 27, 2022

@go101 The Value.Comparable method is new on tip, for 1.20, for #46746.

@bcmills
Copy link
Member

bcmills commented Sep 27, 2022

...which is why Comparable is a method on Value, not Type.

In that case, I think the documentation is confusing — it explicitly states that it reports (emphasis mine) “whether the type of v is comparable”, which on its face seems equivalent to v.Type().Comparable().

(The second sentence does state that “v.Interface() == x will not panic for any x”, but I would prefer that the behavior of the method be described accurately in the first sentence as well.)

@gopherbot
Copy link

gopherbot commented Sep 27, 2022

Change https://go.dev/cl/435277 mentions this issue: reflect: clarify that Value.Comparable checks the value

gopherbot pushed a commit that referenced this issue Sep 27, 2022
For #46746

Change-Id: Ic7a31ddf7cd6bf6dd0db6b9eb3fee68fc180f72e
Reviewed-on: https://go-review.googlesource.com/c/go/+/435277
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

We definitely explicitly chose above to call CanEqual Comparable instead (because it matches the language spec term).

If CanConvert returns false and you call Convert anyway, that panics.
(Same for Addr, Set, Float, and so on.)
So if Comparable returns false and you call Equal anyway, it seems like that should panic too.

In any other package panicking would probably not be the answer, but panicking is how reflect signals these kinds of conditions. It would be odd not to panic in this one function.

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 5, 2022

Sounds like this function should panic. Moving to likely accept for that decision.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@go101
Copy link

go101 commented Oct 7, 2022

Are EqualTo and ComparableWith better method names?

@gopherbot
Copy link

gopherbot commented Oct 7, 2022

Change https://go.dev/cl/440037 mentions this issue: reflect: panic when Value.Equal using uncomparable value

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

@go101 we already decided the names. This was reopened for the panic behavior.

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@go101
Copy link

go101 commented Oct 13, 2022

OK. It is just that Comparable listens like a package-level function name for value comparisons. Not a big matter though.

gopherbot pushed a commit that referenced this issue Oct 17, 2022
Assuming the two values are valid and non-comparable, Equal should panic.

	x := reflect.ValueOf([]int{1, 2, 3})
	x.Equal(x) // can not report false, should panic

Assuming one of them is non-comparable and the other is invalid, it should
always report false.

	x := reflect.ValueOf([]int{1, 2, 3})
	y := reflect.ValueOf(nil)
	x.Equal(y) // should report false

For #46746.

Change-Id: Ifecd77ca0b3de3019fae2be39048f9277831676c
Reviewed-on: https://go-review.googlesource.com/c/go/+/440037
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
Assuming the two values are valid and non-comparable, Equal should panic.

	x := reflect.ValueOf([]int{1, 2, 3})
	x.Equal(x) // can not report false, should panic

Assuming one of them is non-comparable and the other is invalid, it should
always report false.

	x := reflect.ValueOf([]int{1, 2, 3})
	y := reflect.ValueOf(nil)
	x.Equal(y) // should report false

For golang#46746.

Change-Id: Ifecd77ca0b3de3019fae2be39048f9277831676c
Reviewed-on: https://go-review.googlesource.com/c/go/+/440037
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

gopherbot commented Nov 4, 2022

Change https://go.dev/cl/447798 mentions this issue: reflect: rewrite value.Comparable to avoid allocations

@gopherbot
Copy link

gopherbot commented Nov 4, 2022

Change https://go.dev/cl/447798 mentions this issue: reflect: rewrite value.Equal to avoid allocations

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Nov 4, 2022

Everything here has been implemented.

gopherbot pushed a commit that referenced this issue Nov 4, 2022
For #46746

Change-Id: I75ddb9ce24cd3394186562dae156fef9fe2d55d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/447798
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Status: Accepted
Development

No branches or pull requests