interface-receiver: Clarify why value receiver can be called #170
Conversation
The guidance in this section was a bit confusing because it didn't quite explain why we could only call Read, not Write, on the values stored in the map. Resolves uber-go#163
style.md
Outdated
| @@ -216,17 +216,22 @@ func (s *S) Write(str string) { | |||
| s.data = str | |||
| } | |||
|
|
|||
| // We cannot get pointers to value stored in maps. | |||
There was a problem hiding this comment.
| // We cannot get pointers to value stored in maps. | |
| // We cannot get pointers to values stored in maps. |
I'm +1 on this, it's a source of many common mistakes. |
Co-authored-by: Matt Way <[email protected]>
| // You can call both Read and Write using a pointer | ||
| // You can call both Read and Write if the map stores pointers, | ||
| // because pointers are intrinsically addressable. | ||
| sPtrs[1].Read() | ||
| sPtrs[1].Write("test") | ||
| ``` |
There was a problem hiding this comment.
This may compile but it is not safe. sPtrs[1] will return a nil value if 1 is not in the map, and s.Write does not guard against a nil method receiver. (Nor should it: methods should be able to assume their receivers are valid, in general.)
Expressions of the form m[key].Method() which chain map value lookups with method calls on those values are almost never safe. It would be great if this example could be removed from the guide.
There was a problem hiding this comment.
Agreed that this is an unsafe form and m[key].Method() should generally be avoided.
However, I think this case was specifically demonstrating addressability of values -- the value stored in the map not being addressable, and therefore, not usable for the method.
I don't immediately have an alternative suggestion for how else to demonstrate addressable versus non-addressable right after the prior value-in-map example.
But again, I agree that we don't want to encourage m[key].Method(), so, short of a different way to make the point this is making, maybe a comment would suffice? Something like "This sample is just to demonstrate the non-addressable nature of values stored in maps. We don't recommend calling methods on map values directly. See [link]" -- possibly accompanied by a new style guide entry that discourages m[key].Method() -- although we'll probably want to discuss it with the maintainers in a new issue. I think there's some nuance to it besides "don't call methods on map items directly," e.g. "unless you're absolutely certain" or "unless you control all keys". Perhaps "be wary" is better than "don't", but I'll defer that to the discussion.
Edit: Opened #178
The guidance in this section was a bit confusing
because it didn't quite explain why we could only call Read, not Write,
on the values stored in the map.
Resolves #163