Skip to content

"Not checked" issue when error passed as arg #15

@scop

Description

@scop

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: amd64

I 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions