go/analysis/passes/httpresponse: inspect enclosing context of resp, err

The resp, err results of, say, http.Get can immediatelly be passed to a
helper function that checks err and returns resp. In that case, it is ok
for defer to occur immediately after.

The checker currently assumes that a call to, say, http.Get is the only
call made when a resp is returned. The fix is to check if the original
call is made as a part of another (helper) call and, if so, punt.

For golang/go#52661

Change-Id: If9dc4815013476de381fe69548d1fb9c04aa9fd9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/404656
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

go/analysis/passes/httpresponse/httpresponse.go

@@ -62,7 +62,13 @@ 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 := restOfBlock(stack)
+		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).
+			return true
+		}
 		if len(stmts) < 2 {
 			return true // the call to the http function is the last statement of the block.
 		}
@@ -71,6 +77,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		if !ok {
 			return true // the first statement is not assignment.
 		}
+
 		resp := rootIdent(asg.Lhs[0])
 		if resp == nil {
 			return true // could not find the http.Response in the assignment.
@@ -129,21 +136,34 @@ 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, finds the innermost containing
-// block and returns the suffix of its statements starting with the
-// current node (the last element of stack).
-func restOfBlock(stack []ast.Node) []ast.Stmt {
+// 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) {
 	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:]
+					return b.List[j:], false
 				}
 			}
 			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(...))"
+		}
+
 	}
-	return nil
+	return nil, false
 }
 
 // rootIdent finds the root identifier x in a chain of selections x.y.z, or nil if not found.

go/analysis/passes/httpresponse/httpresponse_test.go

@@ -5,10 +5,11 @@
 package httpresponse_test
 
 import (
+	"testing"
+
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/httpresponse"
 	"golang.org/x/tools/internal/typeparams"
-	"testing"
 )
 
 func Test(t *testing.T) {

go/analysis/passes/httpresponse/testdata/src/a/a.go

@@ -83,3 +83,30 @@ func badClientDo() {
 		log.Fatal(err)
 	}
 }
+
+func goodUnwrapResp() {
+	unwrapResp := func(resp *http.Response, err error) *http.Response {
+		if err != nil {
+			panic(err)
+		}
+		return resp
+	}
+	resp := unwrapResp(http.Get("https://golang.org"))
+	// It is ok to call defer here immediately as err has
+	// been checked in unwrapResp (see #52661).
+	defer resp.Body.Close()
+}
+
+func badUnwrapResp() {
+	unwrapResp := func(resp *http.Response, err error) string {
+		if err != nil {
+			panic(err)
+		}
+		return "https://golang.org/" + resp.Status
+	}
+	resp, err := http.Get(unwrapResp(http.Get("https://golang.org")))
+	defer resp.Body.Close() // want "using resp before checking for errors"
+	if err != nil {
+		log.Fatal(err)
+	}
+}