Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(43)

Issue 23420043: code review 23420043: cmd/vet: check for mutexes used as a value receivers (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by josharian
Modified:
10 years, 3 months ago
Reviewers:
r, adg, dfc, bradfitz
CC:
adg, r, dsymonds, golang-dev
Visibility:
Public.

Description

cmd/vet: check for sync types being copied during function calls Using a type containing a sync type directly in a function call (whether as a receiver, a param, or a return value) is an easy way to accidentally copy a lock or other sync primitive. Check for it. The test as implemented does not provide 100% coverage; see the discussion near the bottom of testdata/copylock.go for shortcomings. Fixes issue 6729.

Patch Set 1 #

Patch Set 2 : diff -r 4b37e5d8385f https://code.google.com/p/go.tools #

Patch Set 3 : diff -r 4b37e5d8385f https://code.google.com/p/go.tools #

Total comments: 2

Patch Set 4 : diff -r 4b37e5d8385f https://code.google.com/p/go.tools #

Patch Set 5 : diff -r 4b37e5d8385f https://code.google.com/p/go.tools #

Total comments: 17

Patch Set 6 : diff -r 17c8fe23290a https://code.google.com/p/go.tools #

Total comments: 10

Patch Set 7 : diff -r e3de190d1ada https://code.google.com/p/go.tools #

Patch Set 8 : diff -r e3de190d1ada https://code.google.com/p/go.tools #

Total comments: 16

Patch Set 9 : diff -r f81c7877d377 https://code.google.com/p/go.tools #

Total comments: 4

Patch Set 10 : diff -r f81c7877d377 https://code.google.com/p/go.tools #

Unified diffs Side-by-side diffs Delta from patch set Stats (+200 lines, -7 lines) Patch
A cmd/vet/copylock.go View 1 2 3 4 5 6 7 8 9 1 chunk +100 lines, -0 lines 0 comments Download
M cmd/vet/main.go View 1 2 3 4 5 6 7 8 9 3 chunks +9 lines, -6 lines 0 comments Download
A cmd/vet/testdata/copylock.go View 1 2 3 4 5 6 7 8 9 1 chunk +89 lines, -0 lines 0 comments Download
M cmd/vet/types.go View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 26
josharian
Hello adg@golang.org, r@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools
10 years, 4 months ago (2013-11-08 03:50:44 UTC) #1
dsymonds
Can you make this a bit more generic? There are some other types that fall ...
10 years, 4 months ago (2013-11-08 03:58:11 UTC) #2
adg
On 8 November 2013 14:58, David Symonds <dsymonds@golang.org> wrote: > Can you make this a ...
10 years, 4 months ago (2013-11-08 04:05:30 UTC) #3
adg
I wonder if this can be generalized to catch any function argument that would be ...
10 years, 4 months ago (2013-11-08 04:06:34 UTC) #4
adg
https://codereview.appspot.com/23420043/diff/2/cmd/vet/copylock.go File cmd/vet/copylock.go (right): https://codereview.appspot.com/23420043/diff/2/cmd/vet/copylock.go#newcode17 cmd/vet/copylock.go:17: // inadvertently cause copying of a lock. It currently ...
10 years, 4 months ago (2013-11-08 04:10:01 UTC) #5
josharian
> I wonder if this can be generalized to catch any function argument that > ...
10 years, 4 months ago (2013-11-08 05:37:22 UTC) #6
adg
On 8 November 2013 16:37, <josharian@gmail.com> wrote: > You're right, though, it should check all ...
10 years, 4 months ago (2013-11-08 05:45:28 UTC) #7
josharian
This now checks all function i/o, including receiver, params, and return values. (Thanks for suggesting ...
10 years, 4 months ago (2013-11-08 17:21:13 UTC) #8
adg
Doe this catch func F(sync.Mutex) ? https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go File cmd/vet/copylock.go (right): https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode18 cmd/vet/copylock.go:18: // whether any ...
10 years, 4 months ago (2013-11-11 00:54:53 UTC) #9
josharian
> Doe this catch > func F(sync.Mutex) > ? Yep; added a test for it. ...
10 years, 4 months ago (2013-11-11 16:04:28 UTC) #10
adg
LGTM Rob? https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go File cmd/vet/copylock.go (right): https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go#newcode60 cmd/vet/copylock.go:60: func pathString(path []types.Type) string { If you ...
10 years, 4 months ago (2013-11-12 03:09:43 UTC) #11
r
https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go File cmd/vet/copylock.go (right): https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go#newcode6 cmd/vet/copylock.go:6: // via value receivers. receivers aren't the whole story, ...
10 years, 4 months ago (2013-11-12 23:22:59 UTC) #12
r
10 years, 4 months ago (2013-11-12 23:23:05 UTC) #13
josharian
Sorry for the delay; traveling. Reworked to check for Lock methods instead of sync structs. ...
10 years, 4 months ago (2013-11-16 02:01:12 UTC) #14
josharian
ping
10 years, 4 months ago (2013-11-21 16:44:26 UTC) #15
josharian
On 2013/11/21 16:44:26, josharian wrote: > ping ping
10 years, 3 months ago (2013-12-05 01:36:10 UTC) #16
r
https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go File cmd/vet/copylock.go (right): https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newcode5 cmd/vet/copylock.go:5: // This file contains a test to check that ...
10 years, 3 months ago (2013-12-09 20:46:31 UTC) #17
josharian
PTAL https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go File cmd/vet/copylock.go (right): https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newcode5 cmd/vet/copylock.go:5: // This file contains a test to check ...
10 years, 3 months ago (2013-12-10 00:23:31 UTC) #18
r
https://codereview.appspot.com/23420043/diff/150001/cmd/vet/main.go File cmd/vet/main.go (right): https://codereview.appspot.com/23420043/diff/150001/cmd/vet/main.go#newcode210 cmd/vet/main.go:210: tpkg *types.Package not a fan of this name. now ...
10 years, 3 months ago (2013-12-10 00:36:30 UTC) #19
josharian
PTAL https://codereview.appspot.com/23420043/diff/150001/cmd/vet/main.go File cmd/vet/main.go (right): https://codereview.appspot.com/23420043/diff/150001/cmd/vet/main.go#newcode210 cmd/vet/main.go:210: tpkg *types.Package On 2013/12/10 00:36:30, r wrote: > ...
10 years, 3 months ago (2013-12-10 02:20:07 UTC) #20
r
LGTM came out nicely
10 years, 3 months ago (2013-12-10 04:13:53 UTC) #21
r
*** Submitted as https://code.google.com/p/go/source/detail?r=7e10342eb528&repo=tools *** cmd/vet: check for sync types being copied during function calls ...
10 years, 3 months ago (2013-12-10 04:14:39 UTC) #22
dfc
\o/ On Tue, Dec 10, 2013 at 3:14 PM, <r@golang.org> wrote: > *** Submitted as ...
10 years, 3 months ago (2013-12-10 06:36:52 UTC) #23
adg
We've seen this catch a ton of mistakes already. Nice job Josh! Andrew
10 years, 3 months ago (2013-12-10 07:09:36 UTC) #24
bradfitz
Oh? Where? Good to hear, though. On Tue, Dec 10, 2013 at 11:09 AM, Andrew ...
10 years, 3 months ago (2013-12-10 07:45:07 UTC) #25
josharian
10 years, 3 months ago (2013-12-10 16:03:04 UTC) #26
> We've seen this catch a ton of mistakes already. Nice job Josh!

Thanks. I had lots of help. :)

-josh
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b