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>
diff --git a/go/analysis/passes/lostcancel/cmd/lostcancel/main.go b/go/analysis/passes/lostcancel/cmd/lostcancel/main.go
new file mode 100644
index 0000000..593567b
--- /dev/null
+++ b/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) }
diff --git a/go/analysis/passes/vet/lostcancel.go b/go/analysis/passes/lostcancel/lostcancel.go
similarity index 60%
rename from go/analysis/passes/vet/lostcancel.go
rename to go/analysis/passes/lostcancel/lostcancel.go
index f82db47..d022b34 100644
--- a/go/analysis/passes/vet/lostcancel.go
+++ b/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() {
- register("lostcancel",
- "check for failure to call cancelation function returned by context.WithCancel",
- checkLostCancel,
- funcDecl, funcLit)
+const doc = `check cancel func returned by context.WithCancel is called
+
+The cancelation function returned by context.WithCancel, WithTimeout,
+and WithDeadline must be called or the new context will remain live
+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 @@
// 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) {
- return
+ if !hasImport(pass.Pkg, contextPackage) {
+ 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 @@
// 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 @@
}
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 @@
})
if len(cancelvars) == 0 {
- return // no need to build CFG
+ return // no need to inspect CFG
}
- // Tell the CFG builder which functions never return.
- info := &types.Info{Uses: f.pkg.uses, Selections: f.pkg.selectors}
- mayReturn := func(call *ast.CallExpr) bool {
- name := callName(info, call)
- return !noReturnFuncs[name]
- }
-
- // Build the CFG.
+ // Obtain the CFG.
+ cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
var g *cfg.CFG
var sig *types.Signature
switch node := node.(type) {
case *ast.FuncDecl:
- obj := f.pkg.defs[node.Name]
- if obj == nil {
- return // type error (e.g. duplicate function declaration)
- }
- sig, _ = obj.Type().(*types.Signature)
- g = cfg.New(node.Body, mayReturn)
+ g = cfgs.FuncDecl(node)
+ sig, _ = pass.TypesInfo.Defs[node.Name].Type().(*types.Signature)
case *ast.FuncLit:
- sig, _ = f.pkg.types[node.Type].Type.(*types.Signature)
- g = cfg.New(node.Body, mayReturn)
+ g = cfgs.FuncLit(node)
+ sig, _ = pass.TypesInfo.Types[node.Type].Type.(*types.Signature)
+ }
+ if sig == nil {
+ return // missing type information
}
// Print CFG.
- if debugLostCancel {
- fmt.Println(g.Format(f.fset))
+ if debug {
+ 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 {
- lineno := f.fset.Position(stmt.Pos()).Line
- f.Badf(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)
+ if ret := lostCancelPath(pass, g, v, stmt, sig); ret != nil {
+ lineno := pass.Fset.Position(stmt.Pos()).Line
+ pass.Reportf(stmt.Pos(), "the %s function is not used on all paths (possible context leak)", v.Name())
+ 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 {
- for _, imp := range f.Imports {
- v, _ := strconv.Unquote(imp.Path.Value)
- if v == path {
+func hasImport(pkg *types.Package, path string) bool {
+ for _, imp := range pkg.Imports() {
+ if imp.Path() == path {
return true
}
}
@@ -148,12 +170,12 @@
// 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 @@
// 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 @@
// 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 @@
}
// 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 @@
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 @@
// 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 @@
}
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 ""
-}
diff --git a/go/analysis/passes/lostcancel/lostcancel_test.go b/go/analysis/passes/lostcancel/lostcancel_test.go
new file mode 100644
index 0000000..759634b
--- /dev/null
+++ b/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
+}
diff --git a/go/analysis/passes/lostcancel/testdata/src/a/a.go b/go/analysis/passes/lostcancel/testdata/src/a/a.go
new file mode 100644
index 0000000..f9277ba
--- /dev/null
+++ b/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
+}
diff --git a/go/analysis/passes/vet/testdata/lostcancel.go b/go/analysis/passes/vet/testdata/lostcancel.go
deleted file mode 100644
index b7549c0..0000000
--- a/go/analysis/passes/vet/testdata/lostcancel.go
+++ /dev/null
@@ -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
-}
