cmd/vet: diagnose plain assignment in copylock detector

It went out of its way to look for implicit assignments
but never checked explicit assignments.

This detects the root bug for #12099.

Change-Id: I6a6e774cc38749ea8be7cfd58ba6421247b67000
Reviewed-on: https://go-review.googlesource.com/13646
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/cmd/vet/copylock.go b/src/cmd/vet/copylock.go
index 6c71061b3e645e77a71bd691faa052ef26142de8..8d8399a851b37192d0ea2dac789161d0583251f0 100644 (file)
--- a/src/cmd/vet/copylock.go
+++ b/src/cmd/vet/copylock.go
@@ -18,7 +18,7 @@ func init() {
        register("copylocks",
                "check that locks are not passed by value",
                checkCopyLocks,
-               funcDecl, rangeStmt, funcLit)
+               funcDecl, rangeStmt, funcLit, assignStmt)
 }
 
 // checkCopyLocks checks whether node might
@@ -31,6 +31,18 @@ func checkCopyLocks(f *File, node ast.Node) {
                checkCopyLocksFunc(f, node.Name.Name, node.Recv, node.Type)
        case *ast.FuncLit:
                checkCopyLocksFunc(f, "func", nil, node.Type)
+       case *ast.AssignStmt:
+               checkCopyLocksAssign(f, node)
+       }
+}
+
+// checkCopyLocksAssign checks whether an assignment
+// copies a lock.
+func checkCopyLocksAssign(f *File, as *ast.AssignStmt) {
+       for _, x := range as.Lhs {
+               if path := lockPath(f.pkg.typesPkg, f.pkg.types[x].Type); path != nil {
+                       f.Badf(x.Pos(), "assignment copies lock value to %v: %v", f.gofmt(x), path)
+               }
        }
 }
 
@@ -42,7 +54,7 @@ func checkCopyLocksFunc(f *File, name string, recv *ast.FieldList, typ *ast.Func
        if recv != nil && len(recv.List) > 0 {
                expr := recv.List[0].Type
                if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil {
-                       f.Badf(expr.Pos(), "%s passes Lock by value: %v", name, path)
+                       f.Badf(expr.Pos(), "%s passes lock by value: %v", name, path)
                }
        }
 
@@ -50,7 +62,7 @@ func checkCopyLocksFunc(f *File, name string, recv *ast.FieldList, typ *ast.Func
                for _, field := range typ.Params.List {
                        expr := field.Type
                        if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil {
-                               f.Badf(expr.Pos(), "%s passes Lock by value: %v", name, path)
+                               f.Badf(expr.Pos(), "%s passes lock by value: %v", name, path)
                        }
                }
        }
@@ -59,7 +71,7 @@ func checkCopyLocksFunc(f *File, name string, recv *ast.FieldList, typ *ast.Func
                for _, field := range typ.Results.List {
                        expr := field.Type
                        if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil {
-                               f.Badf(expr.Pos(), "%s returns Lock by value: %v", name, path)
+                               f.Badf(expr.Pos(), "%s returns lock by value: %v", name, path)
                        }
                }
        }
@@ -100,7 +112,7 @@ func checkCopyLocksRangeVar(f *File, rtok token.Token, e ast.Expr) {
                return
        }
        if path := lockPath(f.pkg.typesPkg, typ); path != nil {
-               f.Badf(e.Pos(), "range var %s copies Lock: %v", f.gofmt(e), path)
+               f.Badf(e.Pos(), "range var %s copies lock: %v", f.gofmt(e), path)
        }
 }
 

diff --git a/src/cmd/vet/testdata/copylock.go b/src/cmd/vet/testdata/copylock.go
new file mode 100644 (file)
index 0000000..03d0c33
--- /dev/null
+++ b/src/cmd/vet/testdata/copylock.go
@@ -0,0 +1,28 @@
+package testdata
+
+import "sync"
+
+func OkFunc() {
+       var x *sync.Mutex
+       p := x
+       var y sync.Mutex
+       p = &y
+}
+
+type Tlock struct {
+       once sync.Once
+}
+
+func BadFunc() {
+       var x *sync.Mutex
+       p := x
+       var y sync.Mutex
+       p = &y
+       *p = *x // ERROR "assignment copies lock value to \*p: sync.Mutex"
+
+       var t Tlock
+       var tp *Tlock
+       tp = &t
+       *tp = t // ERROR "assignment copies lock value to \*tp: testdata.Tlock contains sync.Once contains sync.Mutex"
+       t = *tp // ERROR "assignment copies lock value to t: testdata.Tlock contains sync.Once contains sync.Mutex"
+}

diff --git a/src/cmd/vet/testdata/copylock_func.go b/src/cmd/vet/testdata/copylock_func.go
index d83957fe1832f6b6afa9255f005e7e98a5769287..62725d9fa10bf39a381e783952a2b2e6e402ac64 100644 (file)
--- a/src/cmd/vet/testdata/copylock_func.go
+++ b/src/cmd/vet/testdata/copylock_func.go
@@ -10,13 +10,13 @@ package testdata
 import "sync"
 
 func OkFunc(*sync.Mutex) {}
-func BadFunc(sync.Mutex) {} // ERROR "BadFunc passes Lock by value: 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"
+func BadRet() sync.Mutex {} // ERROR "BadRet returns lock by value: sync.Mutex"
 
 var (
        OkClosure  = func(*sync.Mutex) {}
-       BadClosure = func(sync.Mutex) {} // ERROR "func passes Lock by value: sync.Mutex"
+       BadClosure = func(sync.Mutex) {} // ERROR "func passes lock by value: sync.Mutex"
 )
 
 type EmbeddedRWMutex struct {
@@ -24,20 +24,20 @@ type EmbeddedRWMutex struct {
 }
 
 func (*EmbeddedRWMutex) OkMeth() {}
-func (EmbeddedRWMutex) BadMeth() {} // ERROR "BadMeth passes Lock by value: testdata.EmbeddedRWMutex"
+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 BadFunc(EmbeddedRWMutex)    {} // ERROR "BadFunc passes lock by value: testdata.EmbeddedRWMutex"
 func OkRet() *EmbeddedRWMutex    {}
-func BadRet() EmbeddedRWMutex    {} // ERROR "BadRet returns Lock by value: testdata.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 (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"
+func BadFunc(FieldMutex, int) {} // ERROR "BadFunc passes lock by value: testdata.FieldMutex contains sync.Mutex"
 
 type L0 struct {
        L1
@@ -52,7 +52,7 @@ type L2 struct {
 }
 
 func (*L0) Ok() {}
-func (L0) Bad() {} // ERROR "Bad passes Lock by value: testdata.L0 contains testdata.L1 contains testdata.L2"
+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
@@ -76,7 +76,7 @@ func (*CustomLock) Lock()   {}
 func (*CustomLock) Unlock() {}
 
 func Ok(*CustomLock) {}
-func Bad(CustomLock) {} // ERROR "Bad passes Lock by value: testdata.CustomLock"
+func Bad(CustomLock) {} // ERROR "Bad passes lock by value: testdata.CustomLock"
 
 // TODO: Unfortunate cases
 
@@ -85,7 +85,7 @@ func Bad(CustomLock) {} // ERROR "Bad passes Lock by value: testdata.CustomLock"
 // 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"
+func (LocalOnce) Bad() {} // ERROR "Bad passes lock by value: testdata.LocalOnce contains sync.Mutex"
 
 // False negative:
 // LocalMutex doesn't have a Lock method.

diff --git a/src/cmd/vet/testdata/copylock_range.go b/src/cmd/vet/testdata/copylock_range.go
index f95b0252b6f4da6f6d57e408915cec381df53944..f127381213cd7571e4d5ed350f615420be7279ad 100644 (file)
--- a/src/cmd/vet/testdata/copylock_range.go
+++ b/src/cmd/vet/testdata/copylock_range.go
@@ -24,37 +24,37 @@ func rangeMutex() {
        }
        for i, _ := range s {
        }
-       for _, mu = range s { // ERROR "range var mu copies Lock: sync.Mutex"
+       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 _, 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, mu = range s { // ERROR "range var mu copies lock: sync.Mutex"
        }
-       for i, m := range s { // ERROR "range var m 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"
+       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 k := range m { // ERROR "range var k copies lock: sync.Mutex"
        }
-       for mu, _ = range m { // ERROR "range var mu 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 k, _ := range m { // ERROR "range var k copies lock: sync.Mutex"
        }
-       for _, mu = range m { // ERROR "range var mu 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"
+       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 mu = range c { // ERROR "range var mu copies lock: sync.Mutex"
        }
-       for v := range c { // ERROR "range var v copies Lock: sync.Mutex"
+       for v := range c { // ERROR "range var v copies lock: sync.Mutex"
        }
 
        // Test non-idents in range variables
@@ -62,6 +62,6 @@ func rangeMutex() {
                i  int
                mu sync.Mutex
        }
-       for t.i, t.mu = range s { // ERROR "range var t.mu copies Lock: sync.Mutex"
+       for t.i, t.mu = range s { // ERROR "range var t.mu copies lock: sync.Mutex"
        }
 }