go: cmd/vet: detect "defer resp.Body.Close()"s that will panic

but I was a little surprised when no static analysis tools caught this one so far

@ahmetalpbalkan, FYI, I know that staticcheck does catch this issue (and similar ones, like with os.Open, etc.):

• Don’t defer rc.Close() before having checked the error returned by Open or similar.

For example, if I run it on the following program:

package main

import (
    "fmt"
    "io"
    "io/ioutil"
    "net/http"
)

func main() {
    resp, err := http.Get("https://www.example.com/")
    defer resp.Body.Close()
    if err != nil {
        fmt.Println(err)
        return
    }
    // read resp.Body or something
    io.Copy(ioutil.Discard, resp.Body)
}

The output is:

$ staticcheck 
main.go:12:2: should check returned error before deferring resp.Body.Close()

Frequency could be covered by generalizing the check: instead of looking for resp.Body specifically, report all guaranteed nil dereferences, and all dead code caused by if x == nil conditions in which x is known to be always nil (or always non-nil). You’d also need to tell it that some functions like http.Get return zero when err != nil, thought it could figure this out for most functions in the same package.

I ran this check across Google’s Go code and it found a lot of errors, some quite hard to spot. That implementation used golang.org/x/tools/go/ssa, which needs well-typed input, but it would not be hard to make it build sound SSA output from unsound source input, inserting special “fatal” operators as needed to smooth over the gaps. I’m not sure cmd/vet wants to bundle a copy of x/tools/go/ssa, though.

On 13 November 2016 at 18:43, Dmitri Shuralyov notifications@github.com wrote:

• Don’t defer rc.Close() before having checked the error returned by Open or similar.

That does seem like a sensible generalization of the bug pattern.

@robpike is the decider.

Seems like a reasonable rule, at least when limited specifically to net/http: No false positives, easy to get wrong, might not be caught during testing because most http requests don’t fail, blows up in a bad way when something does go wrong.