Skip to content

vet: copylock check doesn't detect certain copies #14664

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

Closed
valyala opened this issue Mar 5, 2016 · 12 comments
Closed

vet: copylock check doesn't detect certain copies #14664

valyala opened this issue Mar 5, 2016 · 12 comments
Milestone

Comments

@valyala
Copy link
Contributor

valyala commented Mar 5, 2016

copylock check must emit a warning on copying sync.Mutex or struct containing sync.Mutex. It doesn't detect the following cases:

        var a sync.Mutex
        b := a   // doesn't detect
        var c = a  // doesn't detect
        b = a  // detects

See #8005 for details.

@valyala
Copy link
Contributor Author

valyala commented Mar 6, 2016

Submitted a patch for code review - https://go-review.googlesource.com/20254 .

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/20254 mentions this issue.

@valyala
Copy link
Contributor Author

valyala commented Mar 6, 2016

Added yet another patch for detecting locks' copying inside composite literals:

type A struct {
   L sync.Mutex
}

var m sync.Mutex
a := A{
    L: m,   // now this case is detected
}
b := []sync.Mutex{m, a.L}  // and this one
c := map[string]sync.Mutex{"foo": m}  // and this one

@valyala
Copy link
Contributor Author

valyala commented Mar 6, 2016

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/20258 mentions this issue.

@josharian
Copy link
Contributor

Thanks for working on these. I left some comments and asked Rob to take over reviewing once they're addressed (that's what R=r means).

@valyala
Copy link
Contributor Author

valyala commented Mar 13, 2016

@josharain, I updated the CL according to comments.

@josharian
Copy link
Contributor

Please ping the CL. (Just write a comment that says "Ping.") Feel free to repeat as needed. I'm leaving the remainder of the review to Rob.

@valyala
Copy link
Contributor Author

valyala commented Jun 22, 2016

There is yet another case not covered by cmd/vet:

var a [5]sync.Mutex
b := a // here we copy mutexes by value

Will create a CL for this case.

@valyala
Copy link
Contributor Author

valyala commented Jun 22, 2016

See the CL

@valyala
Copy link
Contributor Author

valyala commented Jun 23, 2016

Yet another case has been caught and covered by the CL mentioned above:

a := sync.Mutex{}
a = sync.Mutex{}  // here a is overwritten by new value

gopherbot pushed a commit that referenced this issue Aug 29, 2016
Updates #14664

Change-Id: I1f7b1116cfe91466816c760f136ce566da3e80a9
Reviewed-on: https://go-review.googlesource.com/24340
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34630 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 24, 2016
This is a follow-up for https://golang.org/cl/24340.

Updates #14664.
Fixes #18374.

Change-Id: I2831556a9014d30ec70d5f91943d18c33db5b390
Reviewed-on: https://go-review.googlesource.com/34630
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Dec 19, 2017
@dmitshur dmitshur added this to the Go1.7 milestone Apr 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants
@josharian @valyala @dmitshur @gopherbot and others