go/analysis/passes/lostcancel: split out from vet

Change-Id: Id19410d1e81ae29813404cd2e9781f57813110ef
Reviewed-on: https://go-review.googlesource.com/c/139677
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>

go/analysis/passes/lostcancel/cmd/lostcancel/main.go

@@ -0,0 +1,10 @@
+// The nilness command applies the golang.org/x/tools/go/analysis/passes/lostcancel
+// analysis to the specified packages of Go source code.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/passes/lostcancel"
+	"golang.org/x/tools/go/analysis/singlechecker"
+)
+
+func main() { singlechecker.Main(lostcancel.Analyzer) }

go/analysis/passes/lostcancel/lostcancel.go

@@ -1,27 +1,39 @@
-// +build ignore
-
 // Copyright 2016 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 main
+package lostcancel
 
 import (
-	"cmd/vet/internal/cfg"
 	"fmt"
 	"go/ast"
 	"go/types"
-	"strconv"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/ctrlflow"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/cfg"
 )
 
-func init() {
+const doc = `check cancel func returned by context.WithCancel is called
-	register("lostcancel",
+
-		"check for failure to call cancelation function returned by context.WithCancel",
+The cancelation function returned by context.WithCancel, WithTimeout,
-		checkLostCancel,
+and WithDeadline must be called or the new context will remain live
-		funcDecl, funcLit)
+until its parent context is cancelled.
+(The background context is never cancelled.)`
+
+var Analyzer = &analysis.Analyzer{
+	Name: "lostcancel",
+	Doc:  doc,
+	Run:  run,
+	Requires: []*analysis.Analyzer{
+		inspect.Analyzer,
+		ctrlflow.Analyzer,
+	},
 }
 
-const debugLostCancel = false
+const debug = false
 
 var contextPackage = "context"
 
@@ -33,15 +45,32 @@ var contextPackage = "context"
 // counts as a use, even within a nested function literal.
 //
 // checkLostCancel analyzes a single named or literal function.
-func checkLostCancel(f *File, node ast.Node) {
+func run(pass *analysis.Pass) (interface{}, error) {
 	// Fast path: bypass check if file doesn't use context.WithCancel.
-	if !hasImport(f.file, contextPackage) {
+	if !hasImport(pass.Pkg, contextPackage) {
-		return
+		return nil, nil
 	}
 
+	// Call runFunc for each Func{Decl,Lit}.
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	nodeTypes := []ast.Node{
+		(*ast.FuncLit)(nil),
+		(*ast.FuncDecl)(nil),
+	}
+	inspect.Preorder(nodeTypes, func(n ast.Node) {
+		runFunc(pass, n)
+	})
+	return nil, nil
+}
+
+func runFunc(pass *analysis.Pass, node ast.Node) {
 	// Maps each cancel variable to its defining ValueSpec/AssignStmt.
 	cancelvars := make(map[*types.Var]ast.Node)
 
+	// TODO(adonovan): opt: refactor to make a single pass
+	// over the AST using inspect.WithStack and node types
+	// {FuncDecl,FuncLit,CallExpr,SelectorExpr}.
+
 	// Find the set of cancel vars to analyze.
 	stack := make([]ast.Node, 0, 32)
 	ast.Inspect(node, func(n ast.Node) bool {
@@ -62,7 +91,7 @@ func checkLostCancel(f *File, node ast.Node) {
 		//   ctx, cancel     = context.WithCancel(...)
 		//   var ctx, cancel = context.WithCancel(...)
 		//
-		if isContextWithCancel(f, n) && isCall(stack[len(stack)-2]) {
+		if isContextWithCancel(pass.TypesInfo, n) && isCall(stack[len(stack)-2]) {
 			var id *ast.Ident // id of cancel var
 			stmt := stack[len(stack)-3]
 			switch stmt := stmt.(type) {
@@ -77,11 +106,12 @@ func checkLostCancel(f *File, node ast.Node) {
 			}
 			if id != nil {
 				if id.Name == "_" {
-					f.Badf(id.Pos(), "the cancel function returned by context.%s should be called, not discarded, to avoid a context leak",
+					pass.Reportf(id.Pos(),
+						"the cancel function returned by context.%s should be called, not discarded, to avoid a context leak",
 						n.(*ast.SelectorExpr).Sel.Name)
-				} else if v, ok := f.pkg.uses[id].(*types.Var); ok {
+				} else if v, ok := pass.TypesInfo.Uses[id].(*types.Var); ok {
 					cancelvars[v] = stmt
-				} else if v, ok := f.pkg.defs[id].(*types.Var); ok {
+				} else if v, ok := pass.TypesInfo.Defs[id].(*types.Var); ok {
 					cancelvars[v] = stmt
 				}
 			}
@@ -91,55 +121,47 @@ func checkLostCancel(f *File, node ast.Node) {
 	})
 
 	if len(cancelvars) == 0 {
-		return // no need to build CFG
+		return // no need to inspect CFG
 	}
 
-	// Tell the CFG builder which functions never return.
+	// Obtain the CFG.
-	info := &types.Info{Uses: f.pkg.uses, Selections: f.pkg.selectors}
+	cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
-	mayReturn := func(call *ast.CallExpr) bool {
-		name := callName(info, call)
-		return !noReturnFuncs[name]
-	}
-
-	// Build the CFG.
 	var g *cfg.CFG
 	var sig *types.Signature
 	switch node := node.(type) {
 	case *ast.FuncDecl:
-		obj := f.pkg.defs[node.Name]
+		g = cfgs.FuncDecl(node)
-		if obj == nil {
+		sig, _ = pass.TypesInfo.Defs[node.Name].Type().(*types.Signature)
-			return // type error (e.g. duplicate function declaration)
-		}
-		sig, _ = obj.Type().(*types.Signature)
-		g = cfg.New(node.Body, mayReturn)
 	case *ast.FuncLit:
-		sig, _ = f.pkg.types[node.Type].Type.(*types.Signature)
+		g = cfgs.FuncLit(node)
-		g = cfg.New(node.Body, mayReturn)
+		sig, _ = pass.TypesInfo.Types[node.Type].Type.(*types.Signature)
+	}
+	if sig == nil {
+		return // missing type information
 	}
 
 	// Print CFG.
-	if debugLostCancel {
+	if debug {
-		fmt.Println(g.Format(f.fset))
+		fmt.Println(g.Format(pass.Fset))
 	}
 
 	// Examine the CFG for each variable in turn.
 	// (It would be more efficient to analyze all cancelvars in a
 	// single pass over the AST, but seldom is there more than one.)
 	for v, stmt := range cancelvars {
-		if ret := lostCancelPath(f, g, v, stmt, sig); ret != nil {
+		if ret := lostCancelPath(pass, g, v, stmt, sig); ret != nil {
-			lineno := f.fset.Position(stmt.Pos()).Line
+			lineno := pass.Fset.Position(stmt.Pos()).Line
-			f.Badf(stmt.Pos(), "the %s function is not used on all paths (possible context leak)", v.Name())
+			pass.Reportf(stmt.Pos(), "the %s function is not used on all paths (possible context leak)", v.Name())
-			f.Badf(ret.Pos(), "this return statement may be reached without using the %s var defined on line %d", v.Name(), lineno)
+			pass.Reportf(ret.Pos(), "this return statement may be reached without using the %s var defined on line %d", v.Name(), lineno)
 		}
 	}
 }
 
 func isCall(n ast.Node) bool { _, ok := n.(*ast.CallExpr); return ok }
 
-func hasImport(f *ast.File, path string) bool {
+func hasImport(pkg *types.Package, path string) bool {
-	for _, imp := range f.Imports {
+	for _, imp := range pkg.Imports() {
-		v, _ := strconv.Unquote(imp.Path.Value)
+		if imp.Path() == path {
-		if v == path {
 			return true
 		}
 	}
@@ -148,12 +170,12 @@ func hasImport(f *ast.File, path string) bool {
 
 // isContextWithCancel reports whether n is one of the qualified identifiers
 // context.With{Cancel,Timeout,Deadline}.
-func isContextWithCancel(f *File, n ast.Node) bool {
+func isContextWithCancel(info *types.Info, n ast.Node) bool {
 	if sel, ok := n.(*ast.SelectorExpr); ok {
 		switch sel.Sel.Name {
 		case "WithCancel", "WithTimeout", "WithDeadline":
 			if x, ok := sel.X.(*ast.Ident); ok {
-				if pkgname, ok := f.pkg.uses[x].(*types.PkgName); ok {
+				if pkgname, ok := info.Uses[x].(*types.PkgName); ok {
 					return pkgname.Imported().Path() == contextPackage
 				}
 				// Import failed, so we can't check package path.
@@ -169,17 +191,17 @@ func isContextWithCancel(f *File, n ast.Node) bool {
 // the 'cancel' variable v) to a return statement, that doesn't "use" v.
 // If it finds one, it returns the return statement (which may be synthetic).
 // sig is the function's type, if known.
-func lostCancelPath(f *File, g *cfg.CFG, v *types.Var, stmt ast.Node, sig *types.Signature) *ast.ReturnStmt {
+func lostCancelPath(pass *analysis.Pass, g *cfg.CFG, v *types.Var, stmt ast.Node, sig *types.Signature) *ast.ReturnStmt {
 	vIsNamedResult := sig != nil && tupleContains(sig.Results(), v)
 
 	// uses reports whether stmts contain a "use" of variable v.
-	uses := func(f *File, v *types.Var, stmts []ast.Node) bool {
+	uses := func(pass *analysis.Pass, v *types.Var, stmts []ast.Node) bool {
 		found := false
 		for _, stmt := range stmts {
 			ast.Inspect(stmt, func(n ast.Node) bool {
 				switch n := n.(type) {
 				case *ast.Ident:
-					if f.pkg.uses[n] == v {
+					if pass.TypesInfo.Uses[n] == v {
 						found = true
 					}
 				case *ast.ReturnStmt:
@@ -197,10 +219,10 @@ func lostCancelPath(f *File, g *cfg.CFG, v *types.Var, stmt ast.Node, sig *types
 
 	// blockUses computes "uses" for each block, caching the result.
 	memo := make(map[*cfg.Block]bool)
-	blockUses := func(f *File, v *types.Var, b *cfg.Block) bool {
+	blockUses := func(pass *analysis.Pass, v *types.Var, b *cfg.Block) bool {
 		res, ok := memo[b]
 		if !ok {
-			res = uses(f, v, b.Nodes)
+			res = uses(pass, v, b.Nodes)
 			memo[b] = res
 		}
 		return res
@@ -225,7 +247,7 @@ outer:
 	}
 
 	// Is v "used" in the remainder of its defining block?
-	if uses(f, v, rest) {
+	if uses(pass, v, rest) {
 		return nil
 	}
 
@@ -244,13 +266,13 @@ outer:
 				seen[b] = true
 
 				// Prune the search if the block uses v.
-				if blockUses(f, v, b) {
+				if blockUses(pass, v, b) {
 					continue
 				}
 
 				// Found path to return statement?
 				if ret := b.Return(); ret != nil {
-					if debugLostCancel {
+					if debug {
 						fmt.Printf("found path to return in block %s\n", b)
 					}
 					return ret // found
@@ -258,7 +280,7 @@ outer:
 
 				// Recur
 				if ret := search(b.Succs); ret != nil {
-					if debugLostCancel {
+					if debug {
 						fmt.Printf(" from block %s\n", b)
 					}
 					return ret
@@ -278,47 +300,3 @@ func tupleContains(tuple *types.Tuple, v *types.Var) bool {
 	}
 	return false
 }
-
-var noReturnFuncs = map[string]bool{
-	"(*testing.common).FailNow": true,
-	"(*testing.common).Fatal":   true,
-	"(*testing.common).Fatalf":  true,
-	"(*testing.common).Skip":    true,
-	"(*testing.common).SkipNow": true,
-	"(*testing.common).Skipf":   true,
-	"log.Fatal":                 true,
-	"log.Fatalf":                true,
-	"log.Fatalln":               true,
-	"os.Exit":                   true,
-	"panic":                     true,
-	"runtime.Goexit":            true,
-}
-
-// callName returns the canonical name of the builtin, method, or
-// function called by call, if known.
-func callName(info *types.Info, call *ast.CallExpr) string {
-	switch fun := call.Fun.(type) {
-	case *ast.Ident:
-		// builtin, e.g. "panic"
-		if obj, ok := info.Uses[fun].(*types.Builtin); ok {
-			return obj.Name()
-		}
-	case *ast.SelectorExpr:
-		if sel, ok := info.Selections[fun]; ok && sel.Kind() == types.MethodVal {
-			// method call, e.g. "(*testing.common).Fatal"
-			meth := sel.Obj()
-			return fmt.Sprintf("(%s).%s",
-				meth.Type().(*types.Signature).Recv().Type(),
-				meth.Name())
-		}
-		if obj, ok := info.Uses[fun.Sel]; ok {
-			// qualified identifier, e.g. "os.Exit"
-			return fmt.Sprintf("%s.%s",
-				obj.Pkg().Path(),
-				obj.Name())
-		}
-	}
-
-	// function with no name, or defined in missing imported package
-	return ""
-}

go/analysis/passes/lostcancel/lostcancel_test.go

@@ -0,0 +1,13 @@
+package lostcancel_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/lostcancel"
+)
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, lostcancel.Analyzer, "a") // load testdata/src/a/a.go
+}

go/analysis/passes/lostcancel/testdata/src/a/a.go

@@ -0,0 +1,173 @@
+// Copyright 2016 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 a
+
+import (
+	"context"
+	"log"
+	"os"
+	"testing"
+	"time"
+)
+
+var bg = context.Background()
+
+// Check the three functions and assignment forms (var, :=, =) we look for.
+// (Do these early: line numbers are fragile.)
+func _() {
+	var _, cancel = context.WithCancel(bg) // want `the cancel function is not used on all paths \(possible context leak\)`
+	if false {
+		_ = cancel
+	}
+} // want "this return statement may be reached without using the cancel var defined on line 20"
+
+func _() {
+	_, cancel2 := context.WithDeadline(bg, time.Time{}) // want "the cancel2 function is not used..."
+	if false {
+		_ = cancel2
+	}
+} // want "may be reached without using the cancel2 var defined on line 27"
+
+func _() {
+	var cancel3 func()
+	_, cancel3 = context.WithTimeout(bg, 0) // want "function is not used..."
+	if false {
+		_ = cancel3
+	}
+} // want "this return statement may be reached without using the cancel3 var defined on line 35"
+
+func _() {
+	ctx, _ := context.WithCancel(bg)               // want "the cancel function returned by context.WithCancel should be called, not discarded, to avoid a context leak"
+	ctx, _ = context.WithTimeout(bg, 0)            // want "the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak"
+	ctx, _ = context.WithDeadline(bg, time.Time{}) // want "the cancel function returned by context.WithDeadline should be called, not discarded, to avoid a context leak"
+	_ = ctx
+}
+
+func _() {
+	_, cancel := context.WithCancel(bg)
+	defer cancel() // ok
+}
+
+func _() {
+	_, cancel := context.WithCancel(bg) // want "not used on all paths"
+	if condition {
+		cancel()
+	}
+	return // want "this return statement may be reached without using the cancel var"
+}
+
+func _() {
+	_, cancel := context.WithCancel(bg)
+	if condition {
+		cancel()
+	} else {
+		// ok: infinite loop
+		for {
+			print(0)
+		}
+	}
+}
+
+func _() {
+	_, cancel := context.WithCancel(bg) // want "not used on all paths"
+	if condition {
+		cancel()
+	} else {
+		for i := 0; i < 10; i++ {
+			print(0)
+		}
+	}
+} // want "this return statement may be reached without using the cancel var"
+
+func _() {
+	_, cancel := context.WithCancel(bg)
+	// ok: used on all paths
+	switch someInt {
+	case 0:
+		new(testing.T).FailNow()
+	case 1:
+		log.Fatal()
+	case 2:
+		cancel()
+	case 3:
+		print("hi")
+		fallthrough
+	default:
+		os.Exit(1)
+	}
+}
+
+func _() {
+	_, cancel := context.WithCancel(bg) // want "not used on all paths"
+	switch someInt {
+	case 0:
+		new(testing.T).FailNow()
+	case 1:
+		log.Fatal()
+	case 2:
+		cancel()
+	case 3:
+		print("hi") // falls through to implicit return
+	default:
+		os.Exit(1)
+	}
+} // want "this return statement may be reached without using the cancel var"
+
+func _(ch chan int) {
+	_, cancel := context.WithCancel(bg) // want "not used on all paths"
+	select {
+	case <-ch:
+		new(testing.T).FailNow()
+	case ch <- 2:
+		print("hi") // falls through to implicit return
+	case ch <- 1:
+		cancel()
+	default:
+		os.Exit(1)
+	}
+} // want "this return statement may be reached without using the cancel var"
+
+func _(ch chan int) {
+	_, cancel := context.WithCancel(bg)
+	// A blocking select must execute one of its cases.
+	select {
+	case <-ch:
+		panic(0)
+	}
+	if false {
+		_ = cancel
+	}
+}
+
+func _() {
+	go func() {
+		ctx, cancel := context.WithCancel(bg) // want "not used on all paths"
+		if false {
+			_ = cancel
+		}
+		print(ctx)
+	}() // want "may be reached without using the cancel var"
+}
+
+var condition bool
+var someInt int
+
+// Regression test for Go issue 16143.
+func _() {
+	var x struct{ f func() }
+	x.f()
+}
+
+// Regression test for Go issue 16230.
+func _() (ctx context.Context, cancel func()) {
+	ctx, cancel = context.WithCancel(bg)
+	return // a naked return counts as a load of the named result values
+}
+
+// Same as above, but for literal function.
+var _ = func() (ctx context.Context, cancel func()) {
+	ctx, cancel = context.WithCancel(bg)
+	return
+}

go/analysis/passes/vet/testdata/lostcancel.go

@@ -1,155 +0,0 @@
-// Copyright 2016 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 testdata
-
-import (
-	"context"
-	"log"
-	"os"
-	"testing"
-)
-
-// Check the three functions and assignment forms (var, :=, =) we look for.
-// (Do these early: line numbers are fragile.)
-func _() {
-	var ctx, cancel = context.WithCancel() // ERROR "the cancel function is not used on all paths \(possible context leak\)"
-} // ERROR "this return statement may be reached without using the cancel var defined on line 17"
-
-func _() {
-	ctx, cancel2 := context.WithDeadline() // ERROR "the cancel2 function is not used..."
-} // ERROR "may be reached without using the cancel2 var defined on line 21"
-
-func _() {
-	var ctx context.Context
-	var cancel3 func()
-	ctx, cancel3 = context.WithTimeout() // ERROR "function is not used..."
-} // ERROR "this return statement may be reached without using the cancel3 var defined on line 27"
-
-func _() {
-	ctx, _ := context.WithCancel()  // ERROR "the cancel function returned by context.WithCancel should be called, not discarded, to avoid a context leak"
-	ctx, _ = context.WithTimeout()  // ERROR "the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak"
-	ctx, _ = context.WithDeadline() // ERROR "the cancel function returned by context.WithDeadline should be called, not discarded, to avoid a context leak"
-}
-
-func _() {
-	ctx, cancel := context.WithCancel()
-	defer cancel() // ok
-}
-
-func _() {
-	ctx, cancel := context.WithCancel() // ERROR "not used on all paths"
-	if condition {
-		cancel()
-	}
-	return // ERROR "this return statement may be reached without using the cancel var"
-}
-
-func _() {
-	ctx, cancel := context.WithCancel()
-	if condition {
-		cancel()
-	} else {
-		// ok: infinite loop
-		for {
-			print(0)
-		}
-	}
-}
-
-func _() {
-	ctx, cancel := context.WithCancel() // ERROR "not used on all paths"
-	if condition {
-		cancel()
-	} else {
-		for i := 0; i < 10; i++ {
-			print(0)
-		}
-	}
-} // ERROR "this return statement may be reached without using the cancel var"
-
-func _() {
-	ctx, cancel := context.WithCancel()
-	// ok: used on all paths
-	switch someInt {
-	case 0:
-		new(testing.T).FailNow()
-	case 1:
-		log.Fatal()
-	case 2:
-		cancel()
-	case 3:
-		print("hi")
-		fallthrough
-	default:
-		os.Exit(1)
-	}
-}
-
-func _() {
-	ctx, cancel := context.WithCancel() // ERROR "not used on all paths"
-	switch someInt {
-	case 0:
-		new(testing.T).FailNow()
-	case 1:
-		log.Fatal()
-	case 2:
-		cancel()
-	case 3:
-		print("hi") // falls through to implicit return
-	default:
-		os.Exit(1)
-	}
-} // ERROR "this return statement may be reached without using the cancel var"
-
-func _(ch chan int) int {
-	ctx, cancel := context.WithCancel() // ERROR "not used on all paths"
-	select {
-	case <-ch:
-		new(testing.T).FailNow()
-	case y <- ch:
-		print("hi") // falls through to implicit return
-	case ch <- 1:
-		cancel()
-	default:
-		os.Exit(1)
-	}
-} // ERROR "this return statement may be reached without using the cancel var"
-
-func _(ch chan int) int {
-	ctx, cancel := context.WithCancel()
-	// A blocking select must execute one of its cases.
-	select {
-	case <-ch:
-		panic()
-	}
-}
-
-func _() {
-	go func() {
-		ctx, cancel := context.WithCancel() // ERROR "not used on all paths"
-		print(ctx)
-	}() // ERROR "may be reached without using the cancel var"
-}
-
-var condition bool
-var someInt int
-
-// Regression test for Go issue 16143.
-func _() {
-	var x struct{ f func() }
-	x.f()
-}
-
-// Regression test for Go issue 16230.
-func _() (ctx context.Context, cancel func()) {
-	ctx, cancel = context.WithCancel()
-	return // a naked return counts as a load of the named result values
-}
-
-// Same as above, but for literal function.
-var _ = func() (ctx context.Context, cancel func()) {
-	ctx, cancel = context.WithCancel()
-	return
-}