From 08086e624689e0fdf5b53030ecfb96ea709b6d86 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 30 Jun 2016 14:32:03 -0400 Subject: [PATCH] cmd/vet: lostcancel: treat naked return as a use of named results + test. Fixes #16230 Change-Id: Idac995437146a9df9e73f094d2a31abc25b1fa62 Reviewed-on: https://go-review.googlesource.com/24681 Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot --- src/cmd/vet/lostcancel.go | 30 ++++++++++++++++++++++++++---- src/cmd/vet/testdata/lostcancel.go | 12 ++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/cmd/vet/lostcancel.go b/src/cmd/vet/lostcancel.go index 11c3c47783..d049a3e888 100644 --- a/src/cmd/vet/lostcancel.go +++ b/src/cmd/vet/lostcancel.go @@ -101,10 +101,13 @@ func checkLostCancel(f *File, node ast.Node) { // Build the CFG. var g *cfg.CFG + var sig *types.Signature switch node := node.(type) { case *ast.FuncDecl: + sig, _ = f.pkg.defs[node.Name].Type().(*types.Signature) g = cfg.New(node.Body, mayReturn) case *ast.FuncLit: + sig, _ = f.pkg.types[node.Type].Type.(*types.Signature) g = cfg.New(node.Body, mayReturn) } @@ -117,7 +120,7 @@ func checkLostCancel(f *File, node ast.Node) { // (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); ret != nil { + 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) @@ -159,14 +162,24 @@ func isContextWithCancel(f *File, n ast.Node) bool { // lostCancelPath finds a path through the CFG, from stmt (which defines // 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). -func lostCancelPath(f *File, g *cfg.CFG, v *types.Var, stmt ast.Node) *ast.ReturnStmt { +// 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 { + 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 { found := false for _, stmt := range stmts { ast.Inspect(stmt, func(n ast.Node) bool { - if id, ok := n.(*ast.Ident); ok { - if f.pkg.uses[id] == v { + switch n := n.(type) { + case *ast.Ident: + if f.pkg.uses[n] == v { + found = true + } + case *ast.ReturnStmt: + // A naked return statement counts as a use + // of the named result variables. + if n.Results == nil && vIsNamedResult { found = true } } @@ -251,6 +264,15 @@ outer: return search(defblock.Succs) } +func tupleContains(tuple *types.Tuple, v *types.Var) bool { + for i := 0; i < tuple.Len(); i++ { + if tuple.At(i) == v { + return true + } + } + return false +} + var noReturnFuncs = map[string]bool{ "(*testing.common).FailNow": true, "(*testing.common).Fatal": true, diff --git a/src/cmd/vet/testdata/lostcancel.go b/src/cmd/vet/testdata/lostcancel.go index 213dd1832d..b7549c0051 100644 --- a/src/cmd/vet/testdata/lostcancel.go +++ b/src/cmd/vet/testdata/lostcancel.go @@ -141,3 +141,15 @@ 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 +} -- 2.48.1