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.

Sign up for GitHub

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

interface-receiver: Clarify why value receiver can be called #170

Merged
merged 3 commits into from
Mar 3, 2023
+26 −6
Merged

interface-receiver: Clarify why value receiver can be called #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
140827a
interface-receiver: Clarify why value receiver can be called
abhinav Mar 3, 2023
0e359bd
Apply suggestions from code review
abhinav Mar 3, 2023
6387abd
make style.md
abhinav Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 2023-03-03

- Receivers and Interfaces: Clarify subtlety with pointer receivers and values.

# 2022-10-18

- Add guidance on managing goroutine lifecycles.
Expand Down
14 changes: 11 additions & 3 deletions src/interface-receiver.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,25 @@ func (s *S) Write(str string) {
s.data = str
}

// We cannot get pointers to values stored in maps, because they are not
// addressable values.
sVals := map[int]S{1: {"A"}}

// You can only call Read using a value
// We can call Read on values stored in the map because Read
// has a value receiver, which does not require the value to
// be addressable.
sVals[1].Read()

// This will not compile:
// We cannot call Write on values stored in the map because Write
// has a pointer receiver, and it's not possible to get a pointer
// to a value stored in a map.
//
// sVals[1].Write("test")

sPtrs := map[int]*S{1: {"A"}}

// 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")
```
Comment on lines -33 to 44
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@abhinav abhinav Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
14 changes: 11 additions & 3 deletions style.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,17 +216,25 @@ func (s *S) Write(str string) {
s.data = str
}

// We cannot get pointers to values stored in maps, because they are not
// addressable values.
sVals := map[int]S{1: {"A"}}

// You can only call Read using a value
// We can call Read on values stored in the map because Read
// has a value receiver, which does not require the value to
// be addressable.
sVals[1].Read()

// This will not compile:
// We cannot call Write on values stored in the map because Write
// has a pointer receiver, and it's not possible to get a pointer
// to a value stored in a map.
//
// sVals[1].Write("test")

sPtrs := map[int]*S{1: {"A"}}

// 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")
```
Expand Down