|
|
Descriptioncmd/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 #
MessagesTotal messages: 26
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
Sign in to reply to this message.
Can you make this a bit more generic? There are some other types that fall into this category, like sync.WaitGroup.
Sign in to reply to this message.
On 8 November 2013 14:58, David Symonds <dsymonds@golang.org> wrote: > Can you make this a bit more generic? There are some other types that > fall into this category, like sync.WaitGroup. > This catches all of the sync types.
Sign in to reply to this message.
I wonder if this can be generalized to catch any function argument that would be a sync.Foo value or a struct that embeds sync.Foo. On 8 November 2013 14:50, <josharian@gmail.com> wrote: > Reviewers: adg, r, > > Message: > 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 > > > Description: > cmd/vet: check for mutexes used as a value receivers > > Using a mutex as a value receiver is an easy way > to accidentally copy a lock. Check for it. > > The test as implemented does not provide 100% > coverage; see the discussion near the bottom of > testdata/copylock.go for the shortcomings. > > Fixes issue 6729. > > Please review this at https://codereview.appspot.com/23420043/ > > Affected files (+165, -0 lines): > A cmd/vet/copylock.go > M cmd/vet/main.go > A cmd/vet/testdata/copylock.go > > >
Sign in to reply to this message.
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 s/. It currently does this /,/
Sign in to reply to this message.
> I wonder if this can be generalized to catch any function argument that > would be a sync.Foo value or a struct that embeds sync.Foo. It already recurses down through struct fields. You're right, though, it should check all function args, not just the receiver. I'll do that. -josh
Sign in to reply to this message.
On 8 November 2013 16:37, <josharian@gmail.com> wrote: > You're right, though, it should check all function args, not just the > receiver. I'll do that. > Cool. I think that's more likely to catch mistakes, as it's more likely to be the type's user (not author) that makes this mistake.
Sign in to reply to this message.
This now checks all function i/o, including receiver, params, and return values. (Thanks for suggesting this -- it just uncovered a bug in another project of mine. Eep!) I also updated the commit comments to be clearer, I hope. PTAL. 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 On 2013/11/08 04:10:02, adg wrote: > s/. It currently does this /,/ Done, with extra re-wording.
Sign in to reply to this message.
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 parameters, receivers, or return types s/any.+/its receiver, parameters, or return values/ https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode19 cmd/vet/copylock.go:19: // contain structs from the sync package. s/structs/values of types/ https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode26 cmd/vet/copylock.go:26: params := make([]ast.Expr, 0) // pre-calculating the capacity here is more trouble than its worth var params []ast.Expr lose the comment maybe call this "types" https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode32 cmd/vet/copylock.go:32: if d.Type.Params != nil && d.Type.Params.List != nil { no need for the second nil check. the range will be a no-op in that case https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode38 cmd/vet/copylock.go:38: if d.Type.Results != nil && d.Type.Results.List != nil { ditto https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode44 cmd/vet/copylock.go:44: for _, param := range params { for _, expr := range types { https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode46 cmd/vet/copylock.go:46: // pointer receivers are ok s/ receivers/s/ https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode53 cmd/vet/copylock.go:53: if path != nil { i'd put this part in a function too func pathString([]types.Type) string
Sign in to reply to this message.
> Doe this catch > func F(sync.Mutex) > ? Yep; added a test for it. Thanks for the thorough review, always appreciated. Please take another look. 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 parameters, receivers, or return types On 2013/11/11 00:54:54, adg wrote: > s/any.+/its receiver, parameters, or return values/ Done. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode19 cmd/vet/copylock.go:19: // contain structs from the sync package. On 2013/11/11 00:54:54, adg wrote: > s/structs/values of types/ Done as s/structs/values of structs/ "struct" instead of "type" here is intentional; it's safe to contain a sync.Locker (or other interfaces). https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode26 cmd/vet/copylock.go:26: params := make([]ast.Expr, 0) // pre-calculating the capacity here is more trouble than its worth On 2013/11/11 00:54:54, adg wrote: > var params []ast.Expr > lose the comment > maybe call this "types" Done, but used "exprs" instead of "types"; "types" would shadow the go/types import. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode26 cmd/vet/copylock.go:26: params := make([]ast.Expr, 0) // pre-calculating the capacity here is more trouble than its worth On 2013/11/11 00:54:54, adg wrote: > var params []ast.Expr > lose the comment > maybe call this "types" Done, but used "exprs" instead of "types"; "types" would shadow the go/types import. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode32 cmd/vet/copylock.go:32: if d.Type.Params != nil && d.Type.Params.List != nil { On 2013/11/11 00:54:54, adg wrote: > no need for the second nil check. the range will be a no-op in that case Thanks! I always learn something useful. Done. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode38 cmd/vet/copylock.go:38: if d.Type.Results != nil && d.Type.Results.List != nil { On 2013/11/11 00:54:54, adg wrote: > ditto Done. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode44 cmd/vet/copylock.go:44: for _, param := range params { On 2013/11/11 00:54:54, adg wrote: > for _, expr := range types { Done. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode46 cmd/vet/copylock.go:46: // pointer receivers are ok On 2013/11/11 00:54:54, adg wrote: > s/ receivers/s/ Done. https://codereview.appspot.com/23420043/diff/70001/cmd/vet/copylock.go#newcode53 cmd/vet/copylock.go:53: if path != nil { On 2013/11/11 00:54:54, adg wrote: > i'd put this part in a function too > > func pathString([]types.Type) string Done.
Sign in to reply to this message.
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 want to be cute, you could create the type type typePath []types.Type and this function becomes a fmt.Stringer impl func (path typePath) String() string { and the syncPath function returns a typePath: func syncPath(typ types.Type) typePath then your warning becoems f.Warnf(expr.Pos(), passed type contains sync struct: %v", path) :-)
Sign in to reply to this message.
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, surely. https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go#newcode52 cmd/vet/copylock.go:52: path := syncPath(typ) why not ask for a Lock method here instead? https://codereview.appspot.com/23420043/diff/90001/cmd/vet/main.go File cmd/vet/main.go (right): https://codereview.appspot.com/23420043/diff/90001/cmd/vet/main.go#newcode42 cmd/vet/main.go:42: "copylocks": flag.Bool("copylocks", false, "check that types containing locks are not used as value receivers"), s/"check.*receivers"/"check that sync.Mutex variables are not passed by value"/ https://codereview.appspot.com/23420043/diff/90001/cmd/vet/testdata/copylock.go File cmd/vet/testdata/copylock.go (right): https://codereview.appspot.com/23420043/diff/90001/cmd/vet/testdata/copylock.... cmd/vet/testdata/copylock.go:82: // not have the opportunity to discover that. i do not understand this comment. you have the types, you have the fields, you have the methods. what am i missing?
Sign in to reply to this message.
Sorry for the delay; traveling. Reworked to check for Lock methods instead of sync structs. This necessitated keeping a handle to the types.Package; see main.go and types.go. I expect there'll probably be another round trip review on this. So: PTAL! 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. On 2013/11/12 23:22:59, r wrote: > receivers aren't the whole story, surely. Yep, stale comment; thanks for catching it. Now matches the description in main.go. https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go#newcode52 cmd/vet/copylock.go:52: path := syncPath(typ) On 2013/11/12 23:22:59, r wrote: > why not ask for a Lock method here instead? Done. https://codereview.appspot.com/23420043/diff/90001/cmd/vet/copylock.go#newcode60 cmd/vet/copylock.go:60: func pathString(path []types.Type) string { On 2013/11/12 03:09:43, adg wrote: > If you want to be cute, you could create the type > type typePath []types.Type > and this function becomes a fmt.Stringer impl > func (path typePath) String() string { > and the syncPath function returns a typePath: > func syncPath(typ types.Type) typePath > then your warning becoems > f.Warnf(expr.Pos(), passed type contains sync struct: %v", path) > > :-) Couldn't resist. Done. https://codereview.appspot.com/23420043/diff/90001/cmd/vet/main.go File cmd/vet/main.go (right): https://codereview.appspot.com/23420043/diff/90001/cmd/vet/main.go#newcode42 cmd/vet/main.go:42: "copylocks": flag.Bool("copylocks", false, "check that types containing locks are not used as value receivers"), On 2013/11/12 23:22:59, r wrote: > s/"check.*receivers"/"check that sync.Mutex variables are not passed by value"/ Changed to "check that locks are not passed by value". Work for you? (I've struggled with wording a lot in this CL, so everyone's help beating it into shape has been much appreciated.) https://codereview.appspot.com/23420043/diff/90001/cmd/vet/testdata/copylock.go File cmd/vet/testdata/copylock.go (right): https://codereview.appspot.com/23420043/diff/90001/cmd/vet/testdata/copylock.... cmd/vet/testdata/copylock.go:82: // not have the opportunity to discover that. On 2013/11/12 23:22:59, r wrote: > i do not understand this comment. you have the types, you have the fields, you > have the methods. what am i missing? Thanks for pushing back. You're entirely right, this was user error: sync.Mutex doesn't Lock, *sync.Mutex does, and I was misinterpreting what had gone wrong in my early attempts.
Sign in to reply to this message.
ping
Sign in to reply to this message.
On 2013/11/21 16:44:26, josharian wrote: > ping ping
Sign in to reply to this message.
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 locks // This file contains the code to check that locks are not passed by value. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:19: // that are locks. s/that // also rejustify the text please. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:25: // build the set of all exprs to check: receiver, params, return values s/b/B/ s/$/./ https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:51: f.Warnf(expr.Pos(), "passed type contains a lock: %v", path) the word "passed" here doesn't read well in isolation. %s passes or returns Lock by value: where %s is the function name if available. (otherwise say "closure") https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:62: for i := 0; i < n; i++ { // The human-readable path is in reverse order, outermost to innermost. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:65: return strings.Join(components, " -> ") s/->/contains/ ? not sure that's better. opinions welcome https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:65: return strings.Join(components, " -> ") if you looped in the other direction, you could print it as you go using a bytes.buffer and avoid the join. at least do this: for i := range path { components[n-i-1] = path[i].String() } but i prefer the (untested) var b bytes.Buffer for i := range path { if i > 0 { fmt.Fprint(&buf, " -> ") } fmt.Fprint(&buf, path[n-i-1].String()) } return buf.String() https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:87: if plock != nil && lock == nil { you only look at lock if plock != nil, which is rare, so refactor to do plock and then, if non-nil, lock.
Sign in to reply to this message.
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 that locks On 2013/12/09 20:46:32, r wrote: > // This file contains the code to check that locks are not passed by value. Done. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:19: // that are locks. On 2013/12/09 20:46:32, r wrote: > s/that // > also rejustify the text please. Done. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:25: // build the set of all exprs to check: receiver, params, return values On 2013/12/09 20:46:32, r wrote: > s/b/B/ s/$/./ Done. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:51: f.Warnf(expr.Pos(), "passed type contains a lock: %v", path) On 2013/12/09 20:46:32, r wrote: > the word "passed" here doesn't read well in isolation. > > %s passes or returns Lock by value: > > where %s is the function name if available. (otherwise say "closure") Done. Note that closures are not handled, since we only examine ast.FuncDecl, not ast.FuncLit. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:62: for i := 0; i < n; i++ { On 2013/12/09 20:46:32, r wrote: > // The human-readable path is in reverse order, outermost to innermost. Done. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:65: return strings.Join(components, " -> ") On 2013/12/09 20:46:32, r wrote: > s/->/contains/ ? not sure that's better. opinions welcome "contains" is longer but clearer; switched to "contains". https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:65: return strings.Join(components, " -> ") On 2013/12/09 20:46:32, r wrote: > if you looped in the other direction, you could print it as you go using a > bytes.buffer and avoid the join. > > at least do this: > > for i := range path { > components[n-i-1] = path[i].String() > } > > but i prefer the (untested) > > var b bytes.Buffer > for i := range path { > if i > 0 { > fmt.Fprint(&buf, " -> ") > } > fmt.Fprint(&buf, path[n-i-1].String()) > } > > return buf.String() Done. Much nicer, thanks. https://codereview.appspot.com/23420043/diff/130001/cmd/vet/copylock.go#newco... cmd/vet/copylock.go:87: if plock != nil && lock == nil { On 2013/12/09 20:46:32, r wrote: > you only look at lock if plock != nil, which is rare, so refactor to do plock > and then, if non-nil, lock. Done.
Sign in to reply to this message.
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 about typesPkg? https://codereview.appspot.com/23420043/diff/150001/cmd/vet/testdata/copylock.go File cmd/vet/testdata/copylock.go (right): https://codereview.appspot.com/23420043/diff/150001/cmd/vet/testdata/copylock... cmd/vet/testdata/copylock.go:34: func BadFunc(FieldMutex, int) {} // ERROR "BadFunc passes or returns Lock by value: testdata.FieldMutex contains sync.Mutex" why not spend the time to distinguish whether it's a pass or a return? it's pretty easy.
Sign in to reply to this message.
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: > not a fan of this name. now about typesPkg? Done. https://codereview.appspot.com/23420043/diff/150001/cmd/vet/testdata/copylock.go File cmd/vet/testdata/copylock.go (right): https://codereview.appspot.com/23420043/diff/150001/cmd/vet/testdata/copylock... cmd/vet/testdata/copylock.go:34: func BadFunc(FieldMutex, int) {} // ERROR "BadFunc passes or returns Lock by value: testdata.FieldMutex contains sync.Mutex" On 2013/12/10 00:36:30, r wrote: > why not spend the time to distinguish whether it's a pass or a return? it's > pretty easy. Absent-mindedness, not difficulty. :) Thanks for pointing it out.
Sign in to reply to this message.
LGTM came out nicely
Sign in to reply to this message.
*** 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 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. R=adg, r, dsymonds CC=golang-dev https://codereview.appspot.com/23420043 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
\o/ On Tue, Dec 10, 2013 at 3:14 PM, <r@golang.org> wrote: > *** 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 > > 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. > > R=adg, r, dsymonds > CC=golang-dev > https://codereview.appspot.com/23420043 > > Committer: Rob Pike <r@golang.org> > > > > https://codereview.appspot.com/23420043/ > > -- > > ---You received this message because you are subscribed to the Google Groups > "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
We've seen this catch a ton of mistakes already. Nice job Josh! Andrew
Sign in to reply to this message.
Oh? Where? Good to hear, though. On Tue, Dec 10, 2013 at 11:09 AM, Andrew Gerrand <adg@golang.org> wrote: > We've seen this catch a ton of mistakes already. Nice job Josh! > > Andrew > > -- > > --- > You received this message because you are subscribed to the Google Groups > "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. >
Sign in to reply to this message.
|