From 2cd071c190dbf3b1aaf805f58bc62dd6054ceff4 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Wed, 24 Sep 2014 12:35:15 -0700 Subject: [PATCH] go.tools/cmd/vet: warn about copying locks in range statements Fixes golang/go#8356. LGTM=r R=r, dsymonds CC=golang-codereviews https://golang.org/cl/145360043 --- cmd/vet/copylock.go | 61 ++++++++++++++++++-- cmd/vet/testdata/copylock_func.go | 90 ++++++++++++++++++++++++++++++ cmd/vet/testdata/copylock_range.go | 67 ++++++++++++++++++++++ 3 files changed, 212 insertions(+), 6 deletions(-) create mode 100644 cmd/vet/testdata/copylock_func.go create mode 100644 cmd/vet/testdata/copylock_range.go diff --git a/cmd/vet/copylock.go b/cmd/vet/copylock.go index 443b987727..48b7e96b4a 100644 --- a/cmd/vet/copylock.go +++ b/cmd/vet/copylock.go @@ -10,6 +10,7 @@ import ( "bytes" "fmt" "go/ast" + "go/token" "code.google.com/p/go.tools/go/types" ) @@ -18,16 +19,25 @@ func init() { register("copylocks", "check that locks are not passed by value", checkCopyLocks, - funcDecl) + funcDecl, rangeStmt) } -// checkCopyLocks checks whether a function might +// checkCopyLocks checks whether node might +// inadvertently copy a lock. +func checkCopyLocks(f *File, node ast.Node) { + switch node := node.(type) { + case *ast.RangeStmt: + checkCopyLocksRange(f, node) + case *ast.FuncDecl: + checkCopyLocksFunc(f, node) + } +} + +// checkCopyLocksFunc checks whether a function might // inadvertently copy a lock, by checking whether // its receiver, parameters, or return values // are locks. -func checkCopyLocks(f *File, node ast.Node) { - d := node.(*ast.FuncDecl) - +func checkCopyLocksFunc(f *File, d *ast.FuncDecl) { if d.Recv != nil && len(d.Recv.List) > 0 { expr := d.Recv.List[0].Type if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil { @@ -54,9 +64,48 @@ func checkCopyLocks(f *File, node ast.Node) { } } +// checkCopyLocksRange checks whether a range statement +// might inadvertently copy a lock by checking whether +// any of the range variables are locks. +func checkCopyLocksRange(f *File, r *ast.RangeStmt) { + checkCopyLocksRangeVar(f, r.Tok, r.Key) + checkCopyLocksRangeVar(f, r.Tok, r.Value) +} + +func checkCopyLocksRangeVar(f *File, rtok token.Token, e ast.Expr) { + if e == nil { + return + } + id, isId := e.(*ast.Ident) + if isId && id.Name == "_" { + return + } + + var typ types.Type + if rtok == token.DEFINE { + if !isId { + return + } + obj := f.pkg.defs[id] + if obj == nil { + return + } + typ = obj.Type() + } else { + typ = f.pkg.types[e].Type + } + + if typ == nil { + return + } + if path := lockPath(f.pkg.typesPkg, typ); path != nil { + f.Badf(e.Pos(), "range var %s copies Lock: %v", f.gofmt(e), path) + } +} + type typePath []types.Type -// pathString pretty-prints a typePath. +// String pretty-prints a typePath. func (path typePath) String() string { n := len(path) var buf bytes.Buffer diff --git a/cmd/vet/testdata/copylock_func.go b/cmd/vet/testdata/copylock_func.go new file mode 100644 index 0000000000..108c044209 --- /dev/null +++ b/cmd/vet/testdata/copylock_func.go @@ -0,0 +1,90 @@ +// Copyright 2013 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains tests for the copylock checker's +// function declaration analysis. + +package testdata + +import "sync" + +func OkFunc(*sync.Mutex) {} +func BadFunc(sync.Mutex) {} // ERROR "BadFunc passes Lock by value: sync.Mutex" +func OkRet() *sync.Mutex {} +func BadRet() sync.Mutex {} // ERROR "BadRet returns Lock by value: sync.Mutex" + +type EmbeddedRWMutex struct { + sync.RWMutex +} + +func (*EmbeddedRWMutex) OkMeth() {} +func (EmbeddedRWMutex) BadMeth() {} // ERROR "BadMeth passes Lock by value: testdata.EmbeddedRWMutex" +func OkFunc(e *EmbeddedRWMutex) {} +func BadFunc(EmbeddedRWMutex) {} // ERROR "BadFunc passes Lock by value: testdata.EmbeddedRWMutex" +func OkRet() *EmbeddedRWMutex {} +func BadRet() EmbeddedRWMutex {} // ERROR "BadRet returns Lock by value: testdata.EmbeddedRWMutex" + +type FieldMutex struct { + s sync.Mutex +} + +func (*FieldMutex) OkMeth() {} +func (FieldMutex) BadMeth() {} // ERROR "BadMeth passes Lock by value: testdata.FieldMutex contains sync.Mutex" +func OkFunc(*FieldMutex) {} +func BadFunc(FieldMutex, int) {} // ERROR "BadFunc passes Lock by value: testdata.FieldMutex contains sync.Mutex" + +type L0 struct { + L1 +} + +type L1 struct { + l L2 +} + +type L2 struct { + sync.Mutex +} + +func (*L0) Ok() {} +func (L0) Bad() {} // ERROR "Bad passes Lock by value: testdata.L0 contains testdata.L1 contains testdata.L2" + +type EmbeddedMutexPointer struct { + s *sync.Mutex // safe to copy this pointer +} + +func (*EmbeddedMutexPointer) Ok() {} +func (EmbeddedMutexPointer) AlsoOk() {} +func StillOk(EmbeddedMutexPointer) {} +func LookinGood() EmbeddedMutexPointer {} + +type EmbeddedLocker struct { + sync.Locker // safe to copy interface values +} + +func (*EmbeddedLocker) Ok() {} +func (EmbeddedLocker) AlsoOk() {} + +type CustomLock struct{} + +func (*CustomLock) Lock() {} +func (*CustomLock) Unlock() {} + +func Ok(*CustomLock) {} +func Bad(CustomLock) {} // ERROR "Bad passes Lock by value: testdata.CustomLock" + +// TODO: Unfortunate cases + +// Non-ideal error message: +// Since we're looking for Lock methods, sync.Once's underlying +// sync.Mutex gets called out, but without any reference to the sync.Once. +type LocalOnce sync.Once + +func (LocalOnce) Bad() {} // ERROR "Bad passes Lock by value: testdata.LocalOnce contains sync.Mutex" + +// False negative: +// LocalMutex doesn't have a Lock method. +// Nevertheless, it is probably a bad idea to pass it by value. +type LocalMutex sync.Mutex + +func (LocalMutex) Bad() {} // WANTED: An error here :( diff --git a/cmd/vet/testdata/copylock_range.go b/cmd/vet/testdata/copylock_range.go new file mode 100644 index 0000000000..f95b0252b6 --- /dev/null +++ b/cmd/vet/testdata/copylock_range.go @@ -0,0 +1,67 @@ +// Copyright 2014 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains tests for the copylock checker's +// range statement analysis. + +package testdata + +import "sync" + +func rangeMutex() { + var mu sync.Mutex + var i int + + var s []sync.Mutex + for range s { + } + for i = range s { + } + for i := range s { + } + for i, _ = range s { + } + for i, _ := range s { + } + for _, mu = range s { // ERROR "range var mu copies Lock: sync.Mutex" + } + for _, m := range s { // ERROR "range var m copies Lock: sync.Mutex" + } + for i, mu = range s { // ERROR "range var mu copies Lock: sync.Mutex" + } + for i, m := range s { // ERROR "range var m copies Lock: sync.Mutex" + } + + var a [3]sync.Mutex + for _, m := range a { // ERROR "range var m copies Lock: sync.Mutex" + } + + var m map[sync.Mutex]sync.Mutex + for k := range m { // ERROR "range var k copies Lock: sync.Mutex" + } + for mu, _ = range m { // ERROR "range var mu copies Lock: sync.Mutex" + } + for k, _ := range m { // ERROR "range var k copies Lock: sync.Mutex" + } + for _, mu = range m { // ERROR "range var mu copies Lock: sync.Mutex" + } + for _, v := range m { // ERROR "range var v copies Lock: sync.Mutex" + } + + var c chan sync.Mutex + for range c { + } + for mu = range c { // ERROR "range var mu copies Lock: sync.Mutex" + } + for v := range c { // ERROR "range var v copies Lock: sync.Mutex" + } + + // Test non-idents in range variables + var t struct { + i int + mu sync.Mutex + } + for t.i, t.mu = range s { // ERROR "range var t.mu copies Lock: sync.Mutex" + } +}