Useless recover not detected

[jvmatl]

I guess this is a request for a new check, but I can't add labels to the issue.

• The output of 'staticcheck -version' staticcheck 2023.1.3 (v0.4.3)

• The output of 'staticcheck -debug.version' (it is fine if this command fails)

  
  staticcheck 2023.1.3 (v0.4.3)

Compiled with Go version: go1.20.4 Main module: honnef.co/go/tools@v0.4.3 (sum: h1:o/n5/K5gXqk8Gozvs2cnL0F2S1/g1vcGCAx2vETjITw=) Dependencies: github.com/BurntSushi/toml@v1.2.1 (sum: h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=) golang.org/x/exp/typeparams@v0.0.0-20221208152030-732eee02a75a (sum: h1:Jw5wfR+h9mnIYH+OtGT2im5wV1YGGDora5vTv/aa5bE=) golang.org/x/mod@v0.7.0 (sum: h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA=) golang.org/x/sys@v0.3.0 (sum: h1:w8ZOecv6NaNa/zC8944JTU3vz4u6Lagfk4RPQxv92NQ=) golang.org/x/tools@v0.4.1-0.20221208213631-3f74d914ae6d (sum: h1:9ZNWAi4CYhNv60mXGgAncgq7SGc5qa7C8VZV8Tg7Ggs=)

- The output of 'go version'
`go version go1.20.4 linux/amd64`

- The output of 'go env'

GO111MODULE="" GOARCH="amd64" GOBIN="" GOCACHE="/go/code/.cache/alpine-gobe/go-build" GOENV="/.config/go/env" GOEXE="" GOEXPERIMENT="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOINSECURE="" GOMODCACHE="/go/pkg/mod" GONOPROXY="" GONOSUMDB="" GOOS="linux" GOPATH="/go" GOPRIVATE="" GOPROXY="https://proxy.golang.org,direct" GOROOT="/usr/local/go" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64" GOVCS="" GOVERSION="go1.20.4" GCCGO="gccgo" GOAMD64="v1" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/go/code/go.mod" GOWORK="" CGO_CFLAGS="-O2 -g" CGO_CPPFLAGS="" CGO_CXXFLAGS="-O2 -g" CGO_FFLAGS="-O2 -g" CGO_LDFLAGS="-O2 -g" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2964286847=/tmp/go-build -gno-record-gcc-switches"

- Exactly which command you ran
`staticcheck main.go`

- Output of the command and what's wrong with the output
  - No output. 
  - Staticcheck does not warn about the useless call to `recover()`. 
  - Running the program panics, (obviously,) because the `recover()` is not working as the programmer expected -- they forgot to put it in a `defer` clause.
  - I expected staticcheck would warn/complain that `recover()` has no effect outside a `defer` clause. If nothing else, the code inside the `if p != nil { ... }` block is effectively unreachable code outside a `defer` context, because `recover()` will always return `nil`

- Test code (main.go) below:
```go
package main

import "fmt"

func main() {
        // MISSING -> defer func() {
        if p := recover(); p != nil {
                fmt.Println("caught the panic")
        }
        // MISSING -> }()

        panic("Aaaaah!")
}

[ainar-g]

Iirc, it isn't safe to just mark all recovers outside of a defer, because it could be called inside a helper. That is:

func logOnPanic() {
    if v := recover(); v != nil {
        log.Println(v)
    }
}

// …

func f() {
    // …
    defer logOnPanic()
    // …
}

[jvmatl]

Hmmm. I see the point, although, with the exception of trivial functions like the example logOnPanic(), I would think most defer-handling code lives right in the same function, so that you can, for example, log information that's accessible via variables in that scope.

But yeah, without the ability to trace code across function calls, it would be impossible to be sure (other than in main()) whether a function would get called in a defer scope or not. This seems similar in concept the the kind of cross-functional analysis that some linters use to check for potential null pointer exceptions.

I'm not a huge fan of code annotations, but IMO, in a case like this, I'd be willing to live with false-positives that I could silence with a comment...

[dominikh]

There are two scenarios where we can check for this without violating our requirement for virtually no false positives

1. Unexported functions that never get called in a deferred context
2. Checks that only run during code review.

[jvmatl]

I like both of those ideas:

• Unexported functions that never get called in a deferred context -- this seems like an unambiguous bug and worthy of being a regular check. (Anecdotally, I can report that this would have found and prevented the crash that inspired this github issue 😄 )
  • In order to make this more valuable in the context of small utlity programs, I would extend the scope to be (unexported functions OR any functions in package "main") that never get called in a deferred context
• Add as a code-review check if the function is exported: I could be wrong, but IMHO, the 'call recover from inside a helper function' case is unlikely to be as common as a simple oversight/confusion about how recover works, so flagging this for human code review would add value.

(maybe the thing about package main was an unnecessary nitpick - I'm not sure whether capitalized symbols in main are actually considered "exported" or not..)

[dominikh]

I'm not sure whether capitalized symbols in main are actually considered "exported" or not

Depends on what end of the nitpicking one is at. They're exported according to the syntactic rules, they're not exported in the sense that you can't even import the package, but they're exported in that you can probably do some custom compiler and linker invocations to import it. At any rate, it was a useful clarification.