From 62d837c1f3085c8db47ecb19c5cbef8a4beaf128 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Wed, 11 May 2022 09:01:03 -0400
Subject: [PATCH] go/analysis/passes/httpresponse: minor clarification
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

See discussion in https://go-review.googlesource.com/c/tools/+/405314.

Change-Id: Ic5f5182a1cc55b4a2c40f43ebb2250c508388c75
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405537
Reviewed-by: Tim King <taking@google.com>
Reviewed-by: Nooras Saba‎ <saba@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
---
 .../passes/httpresponse/httpresponse.go       | 41 ++++++++-----------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/go/analysis/passes/httpresponse/httpresponse.go b/go/analysis/passes/httpresponse/httpresponse.go
index 092ac75cca..3b9168c6c3 100644
--- a/go/analysis/passes/httpresponse/httpresponse.go
+++ b/go/analysis/passes/httpresponse/httpresponse.go
@@ -62,15 +62,16 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		// Find the innermost containing block, and get the list
 		// of statements starting with the one containing call.
-		stmts, withinAnotherCall := restOfBlock(stack)
-		if withinAnotherCall {
-			// We skip cases when the results of a call to http member
-			// are passed directly to another call, as that later call
-			// could check err != nil and create false positives (#52661).
+		stmts, ncalls := restOfBlock(stack)
+		if len(stmts) < 2 {
+			// The call to the http function is the last statement of the block.
 			return true
 		}
-		if len(stmts) < 2 {
-			return true // the call to the http function is the last statement of the block.
+
+		// Skip cases in which the call is wrapped by another (#52661).
+		// Example:  resp, err := checkError(http.Get(url))
+		if ncalls > 1 {
+			return true
 		}
 
 		asg, ok := stmts[0].(*ast.AssignStmt)
@@ -136,34 +137,26 @@ func isHTTPFuncOrMethodOnClient(info *types.Info, expr *ast.CallExpr) bool {
 	return ok && isNamedType(ptr.Elem(), "net/http", "Client") // method on *http.Client.
 }
 
-// restOfBlock, given a traversal stack, checks if the current node
-// (the last element of stack) appears as an argument to another call.
-// If not, it finds the innermost containing block and returns the
-// suffix of its statements starting with the current node. Otherwise,
-// returns an empty slice.
-func restOfBlock(stack []ast.Node) ([]ast.Stmt, bool) {
+// restOfBlock, given a traversal stack, finds the innermost containing
+// block and returns the suffix of its statements starting with the current
+// node, along with the number of call expressions encountered.
+func restOfBlock(stack []ast.Node) ([]ast.Stmt, int) {
+	var ncalls int
 	for i := len(stack) - 1; i >= 0; i-- {
-		// If the current node appears within another call, then
-		// this has to happen within the same block. We can thus
-		// immediately return on whichever we see first, a block
-		// statement or a call statement.
-
 		if b, ok := stack[i].(*ast.BlockStmt); ok {
 			for j, v := range b.List {
 				if v == stack[i+1] {
-					return b.List[j:], false
+					return b.List[j:], ncalls
 				}
 			}
 			break
 		}
 
-		// The call to an http member currently analyzed is at len(stack)-1.
-		if _, ok := stack[i].(*ast.CallExpr); ok && i != len(stack)-1 {
-			return nil, true // e.g. "resp, err := wrap(http.Get(...))"
+		if _, ok := stack[i].(*ast.CallExpr); ok {
+			ncalls++
 		}
-
 	}
-	return nil, false
+	return nil, 0
 }
 
 // rootIdent finds the root identifier x in a chain of selections x.y.z, or nil if not found.