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 golang/go#6729.

R=adg, r, dsymonds
CC=golang-dev
https://golang.org/cl/23420043

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
+}

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

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 :(

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)