Extend assert.Same to support pointer-like objects #1777
Conversation
Today, assert.Same and assert.NotSame support comparing only literal pointers. This PR generalizes both functions so they also support comparing maps and channels by type and pointer equality. Why these two types? Because the [Go language reference][0] states: > A map or channel value is a reference to the implementation-specific > data structure of the map or channel. That is, it states maps and channels are de-facto pointers. So, my belief is that it would minimize surprise if we allow Same/NotSame to compare any two values that consist of just a reference to some underlying value, regardless if that underlying value is a user-defined object vs some implementation-specific data structure. By this logic, I decided against modifying Same/NotSame to support comparing things like slices, which are not represented via a single reference. [0]: https://go.dev/ref/spec#Representation_of_values
There was a problem hiding this comment.
Your comparison between maps and pointers to structs is a valid one, the assertion has the same semantics in this case.
You are correct in omitting slices, strings, and funcs.
It's preferred to use uintptr(Value.UnsafePointer()) to get the equivalent result.
I wonder why the reflect.Value.Pointer() docs say this. Perhaps it's because when called with a func; the implementation may use unsafe and the Go maintainers prefer that callers be aware of this. The implementation in this PR will not call Pointer() on a value of a func.
I am happy with this change to the behavior of Same/NotSame. I assert that it is a compatible change because they have not accepted maps and channels before, except incorrectly.
| // Both arguments must be pointer variables or something directly coercible | ||
| // to a pointer such as a map or channel. Pointer variable sameness is |
There was a problem hiding this comment.
What's coercible mean? Just say "Both arguments must be of kind pointer, map, or channel".
| c1 := make(chan int) | ||
| c2 := make(chan int) | ||
| if Same(mockT, c1, c2) { | ||
| t.Error("Same should return false") | ||
| } | ||
| if !Same(mockT, c1, c1) { | ||
| t.Error("Same should return true") | ||
| } |
There was a problem hiding this comment.
Encode in the tests that c1 and c2 are Same:
c1 := make(chan int)
c2 := (<-chan int)(s)
| { | ||
| name: "slice disallowed", | ||
| args: args{first: s, second: s}, | ||
| same: False, | ||
| ok: False, | ||
| }, |
There was a problem hiding this comment.
Also test that string and func are disallowed.
Summary
Modify assert.Same and assert.NotSame to support comparing maps and channels by pointer equality.
Changes
samePointerstosameReferencesand generalize it so it will try comparing objects that are represented using a single pointer/reference, as opposed to comparing only literal pointers. We maintain the invariant that the two args must have the same Kind and Type.Test_samePointerstoTest_sameReferencesTest_sameReferences,TestSame, andTestNotSame.Motivation
Today, assert.Same and assert.NotSame support comparing only literal pointers. This PR generalizes both functions so they also support comparing maps and channels by type and pointer equality.
Why these two types? Because the Go language reference states:
That is, all three types are represented by a single reference to some underlying value.
Given this, my belief is it would minimize surprise for users if we handle all three cases in the same way in Same/NotSame: we compare the objects by pointer equality, regardless if the underlying value is a user-defined object vs some implementation-specific data structure.
By this logic, I decided against modifying Same/NotSame to support comparing things like slices, which are not represented via a single reference.
Related issues
There is a related discussion #1732, where (iiuc) the claim was made that we should not support Same/NotSame for maps because we cannot compare map keys and values for sameness in all cases.
However, I found this a somewhat surprising argument: if Same is given a pointer to some user-defined struct, it currently does not attempt to introspect and compare the contents of that struct. So, I don't see why it's necessary or expected to do the same for maps.
Hence, this PR.