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: vet is reporting lock value copying because of a Lock method unrelated to mutexes #18451

Closed
dlecorfec opened this issue Dec 28, 2016 · 11 comments

Comments

Projects
None yet
7 participants
@dlecorfec
Copy link

commented Dec 28, 2016

go version go1.7.3 linux/amd64

A method named Lock() on a struct is enough to trigger the vet lock copy warning. See https://play.golang.org/p/V2v2w7DPAM

What did you expect to see?

$ go vet
$

What did you see instead?

$ go vet
main.go:11: assignment copies lock value to b: main.A
main.go:12: assignment copies lock value to _: main.A
exit status 1
$

@dhananjay92

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

vet checks for any (*T).Lock(). Perhaps we should limit it to a handful of known types (sync.Mutex, sync.RWMutex)?

/cc @robpike

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

This is related to #8005, which made the decision to embed a noCopy struct with a Lock method, which vet checks for. Russ' comment from Mar 1, 2016 explains the current status.

@dhananjay92

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

Ah, okay. Thanks for the context.
Where does vet check for noCopy? Couldn't find it with grep "noCopy" -r src/cmd/vet.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

It doesn't check for noCopy, I think it just checks for the Lock method, which the noLock struct has. The point being, we made Lock a special "no copy" method. Personally, I think it is worth revisiting whether noCopy.Lock was the right approach or if we should expand "no copy" detection to be more general to things other than locks.

From this issue report, it seems that there are types with Lock methods that okay to copy. From #8005, it seems that there is a clear demand for declaring types (other than locks) as "no copy". Thus, it feels strange to me that having a Lock method == "no copy".

@josharian

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

For history, this decision was from Rob's comment on the original copylocks vet CL.

This is the first complaint about this in three years, and having Lock == "no copy" was actually a really useful solution to #8005, so I think it is worth understanding a bit more before altering this.

@dlecorfec can you provide some context about what Lock means in your program?

@dhananjay92

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

I see. From what I understand,

Set A = all types that transitively embed sync.noCopy
Set B = all types with (*T).Lock()

cmd/vet should warn on A. But it uses B as a proxy for A, and ends up warning for types in B-A set too.

How difficult is it to determine whether type T transitively embeds type V? If it's not difficult, we should do that. Otherwise, leave it as is on the ground of this being infrequent false-positive.

@minux

This comment has been minimized.

Copy link
Member

commented Dec 29, 2016

@dlecorfec

This comment has been minimized.

Copy link
Author

commented Dec 29, 2016

Context: a Job struct (for a video encoding job), with a Lock() method which inserts a SQL row in a video_encoding_lock table in order to prevent multiple workers (on multiple servers) from processing the same job.

Notwithstanding this possibly dubious architecture (legacy stuff, slightly older than Go itself), I can easily rename this method if this cmd/vet behaviour is intended.

Being new to this video code base, I was confused by the warning because the struct didn't contain a mutex and I didn't see its Lock method at first sight :)

@josharian

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2016

Thanks, @dlecorfec. The lock is external, making it ok to copy; makes sense.

I think that we should take this as a data point, recommend renaming the Lock method for now, and wait to see whether this issue arises again.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

Agree with @josharian. Good data point, but for now you need to rename your Lock method. Sorry.

@rsc rsc closed this Jan 4, 2017

@dlecorfec

This comment has been minimized.

Copy link
Author

commented Jan 4, 2017

Done, no problem, thanks all for your time!

@golang golang locked and limited conversation to collaborators Jan 4, 2018

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