-
Notifications
You must be signed in to change notification settings - Fork 5
"Not checked" issue when error passed as arg #15
Description
File t_test.go:
package t
import (
"encoding/json"
"fmt"
"os"
"testing"
"github.com/stretchr/testify/assert"
)
type Foo struct {
X interface{}
}
func TestSomething(t *testing.T) {
// 1) This provokes a "error return value not checked" error
assert.NoError(t, json.NewEncoder(os.Stdout).Encode(Foo{""}))
// 2) This does not
err := json.NewEncoder(os.Stdout).Encode(Foo{""})
fmt.Println(err)
}Checking it:
$ ./errchkjson t_test.go
.../t_test.go:18:20: Error return value of `(*encoding/json.Encoder).Encode` is not checked: unsafe type `interface{}` found
$ ./errchkjson -V
errchkjson version 0.2.3
commit: 3aae21891fb6867e0ae247403f8eb67c0573c5d5
built at: 2022-02-08T16:17:38Z
module version: v0.2.3, checksum: h1:97eGTmR/w0paL2SwfRPI1jaAZHaH/fXnxWTw2eEIqE0=
goos: linux
goarch: amd64I find it kind of inconsistent that passing the error value from Encode here to another function -- in this case the often used assert.NoError -- provokes a linter failure (see 1) above), whereas simply capturing an error to a non-underscore variable does not (see 2)). (Ironically, for this particular example, 1) actually does check it due to nature of assert.NoError, whereas 2) doesn't, but that's a sidetrack.)
In my opinion, it would be prudent to skip reporting the error in case 1), i.e. when the returned err value is passed to some other function, because the error might actually be checked there. Just as in 2), capturing the error to a non-underscore variable means it could be checked later. As long as the tool does not make a deeper attempt to see if the error is actually checked later on, I'm not sure why it would differentiate between capturing and passing.