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

cmd/vet: function with no-var receiver raises 'passes lock by value' #27001

Closed
titpetric opened this issue Aug 15, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@titpetric
Copy link

commented Aug 15, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.10

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import (
        "fmt"
        "sync"
)

type S struct {
        sync.Mutex
}

func (S) First() *S {
        return &S{sync.Mutex{}}
}

func (_ S) Second() *S {
        return &S{sync.Mutex{}}
}

func main() {
        s1 := S{}.First()
        s2 := S{}.Second()
        fmt.Println("mutexes", s1, s2)
}

What did you expect to see?

Nothing. The functions don't copy the receiver (not-assignable).

What did you see instead?

# go vet
# app
./main.go:12: First passes lock by value: main.S
./main.go:16: Second passes lock by value: main.S
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

This is working as expected. The error from vet is correct, the method receiver is always a copy of the value the method is called on because a method is just syntactic sugar for a function whose implicit first parameter is the receiver.

In most cases, when the method is declared on a pointer type, the receiver is a copy of the pointer. However in this case the receiver is a value type S, so the expression (rewritten from your example)

var s1 S
s1.First()

Makes a copy of s1 and uses that copy as the receiver for First(). Vet is complaining because any change to the embedded mutex in the copy of s1 will be invisible to other callers.

@titpetric

This comment has been minimized.

Copy link
Author

commented Aug 15, 2018

TBH, if you have a (S) or (_ S), any value of S is un-assignable (no way to access it) and a copy isn't being made, or even produced in the final go assembly. Go asm for the case reports the same output code as func New() *S for both of these receiver methods.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

@titpetric

This comment has been minimized.

Copy link
Author

commented Aug 15, 2018

Will you at least field a PR if I optimize for this rare case? If that's a non-starter, please feel free to close the issue :)

@agnivade

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

I agree with Dave that this is technically correct, but in practice it does seem like a false positive. I'd personally be fine with a fix for this particular edge case, but it would need to cover all other cases, such as when a function receives _ S as a parameter other than the receiver.

On the other hand, this seems like such a niche example - I can't help but wonder if there's any real reason why one would want to do this, avoiding the pointer receiver.

Let's leave this open for a bit, to see if others have any compelling arguments.

@mvdan mvdan added the NeedsDecision label Aug 15, 2018

@mvdan mvdan added this to the Unplanned milestone Aug 15, 2018

@titpetric

This comment has been minimized.

Copy link
Author

commented Aug 15, 2018

The main reason is guaranteed immutability of the receiver. Even if I hide the function signature behind an interface and check that it's implemented as a value receiver, I can't really enforce that somebody may not declare func (s S) Something() S { at that point (thereby exposing himself to the issue that vet reports against). Obviously *S resolves the vet reported issue, but it throws immutability out of the window.

It's not a high priority for me, but I thank you for your time so far, it gave me the opportunity to think about it more and how I would work around this issue. I'd still like to give a stab at fixing copylock at some point, if the discussion will move in that direction. Thank you all ;)

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

@titpetric

This comment has been minimized.

Copy link
Author

commented Aug 16, 2018

I defer to your better judgement and I'm closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.