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

@@ -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

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

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