From 1ca53e67e529c4f4ac570c2e6999f4bb77280abf Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 4 Oct 2018 10:30:31 -0400 Subject: [PATCH] go/analysis/passes/lostcancel: split out from vet Change-Id: Id19410d1e81ae29813404cd2e9781f57813110ef Reviewed-on: https://go-review.googlesource.com/c/139677 Reviewed-by: Michael Matloob Run-TryBot: Michael Matloob --- .../passes/lostcancel/cmd/lostcancel/main.go | 10 + .../passes/{vet => lostcancel}/lostcancel.go | 178 ++++++++---------- .../passes/lostcancel/lostcancel_test.go | 13 ++ .../passes/lostcancel/testdata/src/a/a.go | 173 +++++++++++++++++ go/analysis/passes/vet/testdata/lostcancel.go | 155 --------------- 5 files changed, 274 insertions(+), 255 deletions(-) create mode 100644 go/analysis/passes/lostcancel/cmd/lostcancel/main.go rename go/analysis/passes/{vet => lostcancel}/lostcancel.go (60%) create mode 100644 go/analysis/passes/lostcancel/lostcancel_test.go create mode 100644 go/analysis/passes/lostcancel/testdata/src/a/a.go delete mode 100644 go/analysis/passes/vet/testdata/lostcancel.go 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 00000000..593567b6 --- /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 f82db470..d022b34e 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 @@ 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) { - 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 @@ 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. - 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 @@ 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 "" -} diff --git a/go/analysis/passes/lostcancel/lostcancel_test.go b/go/analysis/passes/lostcancel/lostcancel_test.go new file mode 100644 index 00000000..759634b4 --- /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 00000000..f9277baf --- /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 b7549c00..00000000 --- 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 -}