diff --git a/cmd/vet/copylock.go b/cmd/vet/copylock.go new file mode 100644 index 00000000..b9df0c85 --- /dev/null +++ b/cmd/vet/copylock.go @@ -0,0 +1,100 @@ +// 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 the code to check that locks are not passed by value. + +package main + +import ( + "bytes" + "code.google.com/p/go.tools/go/types" + "fmt" + "go/ast" +) + +// checkCopyLocks checks whether a function might +// inadvertently copy a lock, by checking whether +// its receiver, parameters, or return values +// are locks. +func (f *File) checkCopyLocks(d *ast.FuncDecl) { + if !vet("copylocks") { + return + } + + 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]); path != nil { + f.Warnf(expr.Pos(), "%s passes Lock by value: %v", d.Name.Name, path) + } + } + + if d.Type.Params != nil { + for _, field := range d.Type.Params.List { + expr := field.Type + if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr]); path != nil { + f.Warnf(expr.Pos(), "%s passes Lock by value: %v", d.Name.Name, path) + } + } + } + + if d.Type.Results != nil { + for _, field := range d.Type.Results.List { + expr := field.Type + if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr]); path != nil { + f.Warnf(expr.Pos(), "%s returns Lock by value: %v", d.Name.Name, path) + } + } + } +} + +type typePath []types.Type + +// pathString pretty-prints a typePath. +func (path typePath) String() string { + n := len(path) + var buf bytes.Buffer + for i := range path { + if i > 0 { + fmt.Fprint(&buf, " contains ") + } + // The human-readable path is in reverse order, outermost to innermost. + fmt.Fprint(&buf, path[n-i-1].String()) + } + return buf.String() +} + +// lockPath returns a typePath describing the location of a lock value +// contained in typ. If there is no contained lock, it returns nil. +func lockPath(tpkg *types.Package, typ types.Type) typePath { + if typ == nil { + return nil + } + + // We're only interested in the case in which the underlying + // type is a struct. (Interfaces and pointers are safe to copy.) + styp, ok := typ.Underlying().(*types.Struct) + if !ok { + return nil + } + + // We're looking for cases in which a reference to this type + // can be locked, but a value cannot. This differentiates + // embedded interfaces from embedded values. + if plock := types.NewPointer(typ).MethodSet().Lookup(tpkg, "Lock"); plock != nil { + if lock := typ.MethodSet().Lookup(tpkg, "Lock"); lock == nil { + return []types.Type{typ} + } + } + + nfields := styp.NumFields() + for i := 0; i < nfields; i++ { + ftyp := styp.Field(i).Type() + subpath := lockPath(tpkg, ftyp) + if subpath != nil { + return append(subpath, typ) + } + } + + return nil +} diff --git a/cmd/vet/main.go b/cmd/vet/main.go index 3763cc8d..e8a66492 100644 --- a/cmd/vet/main.go +++ b/cmd/vet/main.go @@ -40,6 +40,7 @@ var report = map[string]*bool{ "atomic": flag.Bool("atomic", false, "check for common mistaken usages of the sync/atomic package"), "buildtags": flag.Bool("buildtags", false, "check that +build tags are valid"), "composites": flag.Bool("composites", false, "check that composite literals used field-keyed elements"), + "copylocks": flag.Bool("copylocks", false, "check that locks are not passed by value"), "methods": flag.Bool("methods", false, "check that canonically named methods are canonically defined"), "nilfunc": flag.Bool("nilfunc", false, "check for comparisons between functions and nil"), "printf": flag.Bool("printf", false, "check printf-like invocations"), @@ -200,12 +201,13 @@ func doPackageDir(directory string) { } type Package struct { - path string - idents map[*ast.Ident]types.Object - types map[ast.Expr]types.Type - values map[ast.Expr]exact.Value - spans map[types.Object]Span - files []*File + path string + idents map[*ast.Ident]types.Object + types map[ast.Expr]types.Type + values map[ast.Expr]exact.Value + spans map[types.Object]Span + files []*File + typesPkg *types.Package } // doPackage analyzes the single package constructed from the named files. @@ -435,6 +437,7 @@ func (f *File) walkFuncDecl(d *ast.FuncDecl) { f.walkMethod(d.Name, d.Type) } f.prepStringerReceiver(d) + f.checkCopyLocks(d) } // prepStringerReceiver checks whether the given declaration is a fmt.Stringer diff --git a/cmd/vet/testdata/copylock.go b/cmd/vet/testdata/copylock.go new file mode 100644 index 00000000..db42d638 --- /dev/null +++ b/cmd/vet/testdata/copylock.go @@ -0,0 +1,89 @@ +// 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. + +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/types.go b/cmd/vet/types.go index 41ce6442..8a478e62 100644 --- a/cmd/vet/types.go +++ b/cmd/vet/types.go @@ -29,7 +29,8 @@ func (pkg *Package) check(fs *token.FileSet, astFiles []*ast.File) error { Values: pkg.values, Objects: pkg.idents, } - _, err := config.Check(pkg.path, fs, astFiles, info) + typesPkg, err := config.Check(pkg.path, fs, astFiles, info) + pkg.typesPkg = typesPkg // update spans for id, obj := range pkg.idents { pkg.growSpan(id, obj)