go/analysis/passes/httpresponse: split out from vet

Change-Id: Ica54852964837182d1848d4d96d43309ad0a6d84
Reviewed-on: https://go-review.googlesource.com/c/142477
Reviewed-by: Michael Matloob <matloob@golang.org>

go/analysis/passes/httpresponse/httpresponse.go

@@ -0,0 +1,177 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package httpresponse defines an Analyzer that checks for mistakes
+// using HTTP responses.
+package httpresponse
+
+import (
+	"go/ast"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+const Doc = `check for mistakes using HTTP responses
+
+A common mistake when using the net/http package is to defer a function
+call to close the http.Response Body before checking the error that
+determines whether the response is valid:
+
+	resp, err := http.Head(url)
+	defer resp.Body.Close()
+	if err != nil {
+		log.Fatal(err)
+	}
+	// (defer statement belongs here)
+
+This checker helps uncover latent nil dereference bugs by reporting a
+diagnostic for such mistakes.`
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "httpresponse",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	// Fast path: if the package doesn't import net/http,
+	// skip the traversal.
+	if !imports(pass.Pkg, "net/http") {
+		return nil, nil
+	}
+
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+	inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool {
+		if !push {
+			return true
+		}
+		call := n.(*ast.CallExpr)
+		if !isHTTPFuncOrMethodOnClient(pass.TypesInfo, call) {
+			return true // the function call is not related to this check.
+		}
+
+		// Find the innermost containing block, and get the list
+		// of statements starting with the one containing call.
+		stmts := restOfBlock(stack)
+		if len(stmts) < 2 {
+			return true // the call to the http function is the last statement of the block.
+		}
+
+		asg, ok := stmts[0].(*ast.AssignStmt)
+		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.
+		}
+
+		def, ok := stmts[1].(*ast.DeferStmt)
+		if !ok {
+			return true // the following statement is not a defer.
+		}
+		root := rootIdent(def.Call.Fun)
+		if root == nil {
+			return true // could not find the receiver of the defer call.
+		}
+
+		if resp.Obj == root.Obj {
+			pass.Reportf(root.Pos(), "using %s before checking for errors", resp.Name)
+		}
+		return true
+	})
+	return nil, nil
+}
+
+// isHTTPFuncOrMethodOnClient checks whether the given call expression is on
+// either a function of the net/http package or a method of http.Client that
+// returns (*http.Response, error).
+func isHTTPFuncOrMethodOnClient(info *types.Info, expr *ast.CallExpr) bool {
+	fun, _ := expr.Fun.(*ast.SelectorExpr)
+	sig, _ := info.Types[fun].Type.(*types.Signature)
+	if sig == nil {
+		return false // the call is not of the form x.f()
+	}
+
+	res := sig.Results()
+	if res.Len() != 2 {
+		return false // the function called does not return two values.
+	}
+	if ptr, ok := res.At(0).Type().(*types.Pointer); !ok || !isNamedType(ptr.Elem(), "net/http", "Response") {
+		return false // the first return type is not *http.Response.
+	}
+
+	errorType := types.Universe.Lookup("error").Type()
+	if !types.Identical(res.At(1).Type(), errorType) {
+		return false // the second return type is not error
+	}
+
+	typ := info.Types[fun.X].Type
+	if typ == nil {
+		id, ok := fun.X.(*ast.Ident)
+		return ok && id.Name == "http" // function in net/http package.
+	}
+
+	if isNamedType(typ, "net/http", "Client") {
+		return true // method on http.Client.
+	}
+	ptr, ok := typ.(*types.Pointer)
+	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 {
+	for i := len(stack) - 1; i >= 0; i-- {
+		if b, ok := stack[i].(*ast.BlockStmt); ok {
+			for j, v := range b.List {
+				if v == stack[i+1] {
+					return b.List[j:]
+				}
+			}
+			break
+		}
+	}
+	return nil
+}
+
+// rootIdent finds the root identifier x in a chain of selections x.y.z, or nil if not found.
+func rootIdent(n ast.Node) *ast.Ident {
+	switch n := n.(type) {
+	case *ast.SelectorExpr:
+		return rootIdent(n.X)
+	case *ast.Ident:
+		return n
+	default:
+		return nil
+	}
+}
+
+// isNamedType reports whether t is the named type path.name.
+func isNamedType(t types.Type, path, name string) bool {
+	n, ok := t.(*types.Named)
+	if !ok {
+		return false
+	}
+	obj := n.Obj()
+	return obj.Name() == name && obj.Pkg() != nil && obj.Pkg().Path() == path
+}
+
+func imports(pkg *types.Package, path string) bool {
+	for _, imp := range pkg.Imports() {
+		if imp.Path() == path {
+			return true
+		}
+	}
+	return false
+}

go/analysis/passes/httpresponse/httpresponse_test.go

@@ -0,0 +1,17 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package httpresponse_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/httpresponse"
+)
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, httpresponse.Analyzer, "a")
+}

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

@@ -1,4 +1,4 @@
-package testdata
+package a
 
 import (
 	"log"
@@ -15,7 +15,7 @@ func goodHTTPGet() {
 
 func badHTTPGet() {
 	res, err := http.Get("http://foo.com")
-	defer res.Body.Close() // ERROR "using res before checking for errors"
+	defer res.Body.Close() // want "using res before checking for errors"
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -23,7 +23,7 @@ func badHTTPGet() {
 
 func badHTTPHead() {
 	res, err := http.Head("http://foo.com")
-	defer res.Body.Close() // ERROR "using res before checking for errors"
+	defer res.Body.Close() // want "using res before checking for errors"
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -41,7 +41,7 @@ func goodClientGet() {
 func badClientPtrGet() {
 	client := http.DefaultClient
 	resp, err := client.Get("http://foo.com")
-	defer resp.Body.Close() // ERROR "using resp before checking for errors"
+	defer resp.Body.Close() // want "using resp before checking for errors"
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -50,7 +50,7 @@ func badClientPtrGet() {
 func badClientGet() {
 	client := http.Client{}
 	resp, err := client.Get("http://foo.com")
-	defer resp.Body.Close() // ERROR "using resp before checking for errors"
+	defer resp.Body.Close() // want "using resp before checking for errors"
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -64,7 +64,7 @@ func badClientPtrDo() {
 	}
 
 	resp, err := client.Do(req)
-	defer resp.Body.Close() // ERROR "using resp before checking for errors"
+	defer resp.Body.Close() // want "using resp before checking for errors"
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -78,7 +78,7 @@ func badClientDo() {
 	}
 
 	resp, err := client.Do(req)
-	defer resp.Body.Close() // ERROR "using resp before checking for errors"
+	defer resp.Body.Close() // want "using resp before checking for errors"
 	if err != nil {
 		log.Fatal(err)
 	}

go/analysis/passes/vet/httpresponse.go

@@ -1,139 +0,0 @@
-// +build ignore
-
-// Copyright 2016 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-// This file contains the check for http.Response values being used before
-// checking for errors.
-
-package main
-
-import (
-	"go/ast"
-	"go/types"
-)
-
-func init() {
-	register("httpresponse",
-		"check errors are checked before using an http Response",
-		checkHTTPResponse, callExpr)
-}
-
-func checkHTTPResponse(f *File, node ast.Node) {
-	call := node.(*ast.CallExpr)
-	if !isHTTPFuncOrMethodOnClient(f, call) {
-		return // the function call is not related to this check.
-	}
-
-	finder := &blockStmtFinder{node: call}
-	ast.Walk(finder, f.file)
-	stmts := finder.stmts()
-	if len(stmts) < 2 {
-		return // the call to the http function is the last statement of the block.
-	}
-
-	asg, ok := stmts[0].(*ast.AssignStmt)
-	if !ok {
-		return // the first statement is not assignment.
-	}
-	resp := rootIdent(asg.Lhs[0])
-	if resp == nil {
-		return // could not find the http.Response in the assignment.
-	}
-
-	def, ok := stmts[1].(*ast.DeferStmt)
-	if !ok {
-		return // the following statement is not a defer.
-	}
-	root := rootIdent(def.Call.Fun)
-	if root == nil {
-		return // could not find the receiver of the defer call.
-	}
-
-	if resp.Obj == root.Obj {
-		f.Badf(root.Pos(), "using %s before checking for errors", resp.Name)
-	}
-}
-
-// isHTTPFuncOrMethodOnClient checks whether the given call expression is on
-// either a function of the net/http package or a method of http.Client that
-// returns (*http.Response, error).
-func isHTTPFuncOrMethodOnClient(f *File, expr *ast.CallExpr) bool {
-	fun, _ := expr.Fun.(*ast.SelectorExpr)
-	sig, _ := f.pkg.types[fun].Type.(*types.Signature)
-	if sig == nil {
-		return false // the call is not on of the form x.f()
-	}
-
-	res := sig.Results()
-	if res.Len() != 2 {
-		return false // the function called does not return two values.
-	}
-	if ptr, ok := res.At(0).Type().(*types.Pointer); !ok || !isNamedType(ptr.Elem(), "net/http", "Response") {
-		return false // the first return type is not *http.Response.
-	}
-	if !types.Identical(res.At(1).Type().Underlying(), errorType) {
-		return false // the second return type is not error
-	}
-
-	typ := f.pkg.types[fun.X].Type
-	if typ == nil {
-		id, ok := fun.X.(*ast.Ident)
-		return ok && id.Name == "http" // function in net/http package.
-	}
-
-	if isNamedType(typ, "net/http", "Client") {
-		return true // method on http.Client.
-	}
-	ptr, ok := typ.(*types.Pointer)
-	return ok && isNamedType(ptr.Elem(), "net/http", "Client") // method on *http.Client.
-}
-
-// blockStmtFinder is an ast.Visitor that given any ast node can find the
-// statement containing it and its succeeding statements in the same block.
-type blockStmtFinder struct {
-	node  ast.Node       // target of search
-	stmt  ast.Stmt       // innermost statement enclosing argument to Visit
-	block *ast.BlockStmt // innermost block enclosing argument to Visit.
-}
-
-// Visit finds f.node performing a search down the ast tree.
-// It keeps the last block statement and statement seen for later use.
-func (f *blockStmtFinder) Visit(node ast.Node) ast.Visitor {
-	if node == nil || f.node.Pos() < node.Pos() || f.node.End() > node.End() {
-		return nil // not here
-	}
-	switch n := node.(type) {
-	case *ast.BlockStmt:
-		f.block = n
-	case ast.Stmt:
-		f.stmt = n
-	}
-	if f.node.Pos() == node.Pos() && f.node.End() == node.End() {
-		return nil // found
-	}
-	return f // keep looking
-}
-
-// stmts returns the statements of f.block starting from the one including f.node.
-func (f *blockStmtFinder) stmts() []ast.Stmt {
-	for i, v := range f.block.List {
-		if f.stmt == v {
-			return f.block.List[i:]
-		}
-	}
-	return nil
-}
-
-// rootIdent finds the root identifier x in a chain of selections x.y.z, or nil if not found.
-func rootIdent(n ast.Node) *ast.Ident {
-	switch n := n.(type) {
-	case *ast.SelectorExpr:
-		return rootIdent(n.X)
-	case *ast.Ident:
-		return n
-	default:
-		return nil
-	}
-}