From 95299016986435f846545c27f956768ad3c3cb2f Mon Sep 17 00:00:00 2001
From: Shengyu Zhang <shengyu.zhang@chaitin.com>
Date: Wed, 8 May 2019 02:06:31 +0000
Subject: [PATCH] lostcancel: do not analyze cancel variable which defined
 outside current function scope

See golang/go#31856.

Change-Id: I229a7f4a48e7806df62941f801302b6da8a0c12b
GitHub-Last-Rev: 33f85236bb79e325112866ee555950a4479ad7a7
GitHub-Pull-Request: golang/tools#95
Reviewed-on: https://go-review.googlesource.com/c/tools/+/175617
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 go/analysis/passes/lostcancel/lostcancel.go   | 17 ++++++++++++++++-
 .../passes/lostcancel/testdata/src/a/a.go     | 19 +++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/go/analysis/passes/lostcancel/lostcancel.go b/go/analysis/passes/lostcancel/lostcancel.go
index b5161836..e88cf57d 100644
--- a/go/analysis/passes/lostcancel/lostcancel.go
+++ b/go/analysis/passes/lostcancel/lostcancel.go
@@ -45,6 +45,8 @@ var contextPackage = "context"
 // control-flow path from the call to a return statement and that path
 // does not "use" the cancel function.  Any reference to the variable
 // counts as a use, even within a nested function literal.
+// If the variable's scope is larger than the function
+// containing the assignment, we assume that other uses exist.
 //
 // checkLostCancel analyzes a single named or literal function.
 func run(pass *analysis.Pass) (interface{}, error) {
@@ -66,6 +68,15 @@ func run(pass *analysis.Pass) (interface{}, error) {
 }
 
 func runFunc(pass *analysis.Pass, node ast.Node) {
+	// Find scope of function node
+	var funcScope *types.Scope
+	switch v := node.(type) {
+	case *ast.FuncLit:
+		funcScope = pass.TypesInfo.Scopes[v.Type]
+	case *ast.FuncDecl:
+		funcScope = pass.TypesInfo.Scopes[v.Type]
+	}
+
 	// Maps each cancel variable to its defining ValueSpec/AssignStmt.
 	cancelvars := make(map[*types.Var]ast.Node)
 
@@ -114,7 +125,11 @@ func runFunc(pass *analysis.Pass, node ast.Node) {
 					"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 := pass.TypesInfo.Uses[id].(*types.Var); ok {
-				cancelvars[v] = stmt
+				// If the cancel variable is defined outside function scope,
+				// do not analyze it.
+				if funcScope.Contains(v.Pos()) {
+					cancelvars[v] = stmt
+				}
 			} else if v, ok := pass.TypesInfo.Defs[id].(*types.Var); ok {
 				cancelvars[v] = stmt
 			}
diff --git a/go/analysis/passes/lostcancel/testdata/src/a/a.go b/go/analysis/passes/lostcancel/testdata/src/a/a.go
index f9277baf..4aa3a61f 100644
--- a/go/analysis/passes/lostcancel/testdata/src/a/a.go
+++ b/go/analysis/passes/lostcancel/testdata/src/a/a.go
@@ -171,3 +171,22 @@ var _ = func() (ctx context.Context, cancel func()) {
 	ctx, cancel = context.WithCancel(bg)
 	return
 }
+
+// Test for Go issue 31856.
+func _() {
+	var cancel func()
+
+	func() {
+		_, cancel = context.WithCancel(bg)
+	}()
+
+	cancel()
+}
+
+var cancel1 func()
+
+// Same as above, but for package-level cancel variable.
+func _() {
+	// We assume that other uses of cancel1 exist.
+	_, cancel1 = context.WithCancel(bg)
+}