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>
diff --git a/go/analysis/passes/httpresponse/httpresponse.go b/go/analysis/passes/httpresponse/httpresponse.go
index fd9e2af..092ac75 100644
--- a/go/analysis/passes/httpresponse/httpresponse.go
+++ b/go/analysis/passes/httpresponse/httpresponse.go
@@ -62,7 +62,13 @@
// 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 @@
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 @@
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.
diff --git a/go/analysis/passes/httpresponse/httpresponse_test.go b/go/analysis/passes/httpresponse/httpresponse_test.go
index 14e1667..34dc78c 100644
--- a/go/analysis/passes/httpresponse/httpresponse_test.go
+++ b/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) {
diff --git a/go/analysis/passes/httpresponse/testdata/src/a/a.go b/go/analysis/passes/httpresponse/testdata/src/a/a.go
index df7703f..de41212 100644
--- a/go/analysis/passes/httpresponse/testdata/src/a/a.go
+++ b/go/analysis/passes/httpresponse/testdata/src/a/a.go
@@ -83,3 +83,30 @@
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)
+ }
+}
