go/analysis/passes/copylock: find locks via type parameters

Add support for finding incorrect usage of locks via type parameters. It
would probably be fine not to do this, since using locks explicitly via
type parameters should be exceedingly rare. However, it was
straightforward to add, and is consistent with other analyzers.

Updates golang/go#48704

Change-Id: I329a2fa9f11c6bbb491d49afde7fabce8299cbdf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/360234
Trust: Robert Findley <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Tim King <[email protected]>

Author: findleyr

go/analysis/passes/copylock/copylock.go

@@ -17,6 +17,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
 	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/internal/typeparams"
 )
 
 const Doc = `check for locks erroneously passed by value
@@ -204,7 +205,7 @@ func checkCopyLocksRangeVar(pass *analysis.Pass, rtok token.Token, e ast.Expr) {
 	}
 }
 
-type typePath []types.Type
+type typePath []string
 
 // String pretty-prints a typePath.
 func (path typePath) String() string {
@@ -215,7 +216,7 @@ func (path typePath) String() string {
 			fmt.Fprint(&buf, " contains ")
 		}
 		// The human-readable path is in reverse order, outermost to innermost.
-		fmt.Fprint(&buf, path[n-i-1].String())
+		fmt.Fprint(&buf, path[n-i-1])
 	}
 	return buf.String()
 }
@@ -244,6 +245,35 @@ func lockPath(tpkg *types.Package, typ types.Type) typePath {
 		return nil
 	}
 
+	if tpar, ok := typ.(*typeparams.TypeParam); ok {
+		terms, err := typeparams.StructuralTerms(tpar)
+		if err != nil {
+			return nil // invalid type
+		}
+		for _, term := range terms {
+			subpath := lockPath(tpkg, term.Type())
+			if len(subpath) > 0 {
+				if term.Tilde() {
+					// Prepend a tilde to our lock path entry to clarify the resulting
+					// diagnostic message. Consider the following example:
+					//
+					//  func _[Mutex interface{ ~sync.Mutex; M() }](m Mutex) {}
+					//
+					// Here the naive error message will be something like "passes lock
+					// by value: Mutex contains sync.Mutex". This is misleading because
+					// the local type parameter doesn't actually contain sync.Mutex,
+					// which lacks the M method.
+					//
+					// With tilde, it is clearer that the containment is via an
+					// approximation element.
+					subpath[len(subpath)-1] = "~" + subpath[len(subpath)-1]
+				}
+				return append(subpath, typ.String())
+			}
+		}
+		return nil
+	}
+
 	for {
 		atyp, ok := typ.Underlying().(*types.Array)
 		if !ok {
@@ -263,7 +293,7 @@ func lockPath(tpkg *types.Package, typ types.Type) typePath {
 	// is a sync.Locker, but a value is not. This differentiates
 	// embedded interfaces from embedded values.
 	if types.Implements(types.NewPointer(typ), lockerType) && !types.Implements(typ, lockerType) {
-		return []types.Type{typ}
+		return []string{typ.String()}
 	}
 
 	// In go1.10, sync.noCopy did not implement Locker.
@@ -272,15 +302,15 @@ func lockPath(tpkg *types.Package, typ types.Type) typePath {
 	if named, ok := typ.(*types.Named); ok &&
 		named.Obj().Name() == "noCopy" &&
 		named.Obj().Pkg().Path() == "sync" {
-		return []types.Type{typ}
+		return []string{typ.String()}
 	}
 
 	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 append(subpath, typ.String())
 		}
 	}
 

go/analysis/passes/copylock/copylock_test.go

@@ -9,9 +9,14 @@ import (
 
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/copylock"
+	"golang.org/x/tools/internal/typeparams"
 )
 
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
-	analysistest.Run(t, testdata, copylock.Analyzer, "a")
+	pkgs := []string{"a"}
+	if typeparams.Enabled {
+		pkgs = append(pkgs, "typeparams")
+	}
+	analysistest.Run(t, testdata, copylock.Analyzer, pkgs...)
 }

go/analysis/passes/copylock/testdata/src/typeparams/typeparams.go

@@ -0,0 +1,45 @@
+// Copyright 2021 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.
+
+package typeparams
+
+import "sync"
+
+func OkFunc1[Struct ~*struct{ mu sync.Mutex }](s Struct) {
+}
+
+func BadFunc1[Struct ~struct{ mu sync.Mutex }](s Struct) { // want `passes lock by value: .*Struct contains ~struct{mu sync.Mutex}`
+}
+
+func OkFunc2[MutexPtr *sync.Mutex](m MutexPtr) {
+	var x *MutexPtr
+	p := x
+	var y MutexPtr
+	p = &y
+	*p = *x
+
+	var mus []MutexPtr
+
+	for _, _ = range mus {
+	}
+}
+
+func BadFunc2[Mutex sync.Mutex](m Mutex) { // want `passes lock by value: .*Mutex contains sync.Mutex`
+	var x *Mutex
+	p := x
+	var y Mutex
+	p = &y
+	*p = *x // want `assignment copies lock value to \*p: .*Mutex contains sync.Mutex`
+
+	var mus []Mutex
+
+	for _, _ = range mus {
+	}
+}
+
+func ApproximationError[Mutex interface {
+	~sync.Mutex
+	M()
+}](m Mutex) { // want `passes lock by value: .*Mutex contains ~sync.Mutex`
+}