diff --git a/go/analysis/passes/httpresponse/httpresponse.go b/go/analysis/passes/httpresponse/httpresponse.go index fd9e2af2b1..092ac75cca 100644 --- a/go/analysis/passes/httpresponse/httpresponse.go +++ b/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. diff --git a/go/analysis/passes/httpresponse/httpresponse_test.go b/go/analysis/passes/httpresponse/httpresponse_test.go index 14e1667890..34dc78ce20 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 df7703f412..de41212703 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 @@ 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) + } +}