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

[titpetric]

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 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]

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]

That’s a fair observation but I vet isn’t going to optimise for this rare case. Just move your method to a pointer receiver and vet will be happy.

On 15 Aug 2018, at 22:19, Tit Petric notifications@github.com wrote:

TBH, if you have a (S) or (_ S), any value of S is un-assignable 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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[titpetric]

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]

@mvdan

[mvdan]

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.

[titpetric]

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]

I don’t think this should be fixed in vet. It is a bug in user code that vet has identified.

Copying the receiver, even in the extreme case of omitting a local name for the receiver is a potential data race — the atomic variable that sync uses is copied without using an atomic load — which pushes the program into the realm of undefined behaviour.

I think that this check should remain as is and its warning heeded.

On 16 Aug 2018, at 05:59, Tit Petric notifications@github.com wrote:

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 ;)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[titpetric]

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