Skipping error checks for json.Encoder.Encode or json.MarshalIndent is not safe

[scop]

$ cat t.go
package main

import (
	"encoding/json"
	"os"
)

type Foo struct {
	X string
}

func main() {
	err := json.NewEncoder(os.Stdout).Encode(Foo{"bar"})
	if err != nil {
		panic("meh")
	}
}
$ ./errchkjson t.go
t.go:13:9: Error return value of `(*encoding/json.Encoder).Encode` is checked but passed argument is safe
$ ./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

json.Encoder.Encode can return an error due to a number of reasons unrelated to the argument's general JSON marshal safety:

• https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/stream.go;l=202
• https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/stream.go;l=225
• https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/stream.go;l=231

The same applies to json.MarshalIndent:

• https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/encode.go;l=181

I think errchkjson should not encourage skipping error checks for these functions at all.

(Haven't thought through if there are other messages in errchkjson that should be adjusted due to these considerations, but I suspect there might be some.)

[breml]

@scop I guess, you are right. I started with json.Marshal only and later extended it to json.MarshalIndent and json.Encoder.Encode. While doing so, I did not properly check for all the potential error cases.
I will have a look at this.

[breml]

I just looked again at the json.MarshalIndent case and I think it is save to keep this case for the following reason:

While it is true, that json.MarshalIndent might in theory return an error on https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/encode.go;l=181, I don't think, this will ever happen, because json.Indent only returns an error, if the scanning of the passed JSON returns an error. Such an error would imply, that json.Marshal did produce invalid JSON, which would be an error in the JSON encoding of the standard library.

[scop]

https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/encoding/json/scanner.go;l=181;drc=refs%2Ftags%2Fgo1.17.6 is arguably a (pathological) case where scanning the json.Marshal generated, valid JSON could result in an error while indenting. (Caveat: haven't checked if json.Marshal could actually produce such output.)

Ignoring that, I guess the call on this boils down to how much one is ready to rely on the internals of the function in question, and that it won't start to actually return errors -- changes in this area aren't necessarily breaking ones and could conceivably occur within Go 1.x series. Then again, I suppose the same could be said about treating any JSON marshalling operations as safe... but it's good that this part of the linter's functionality is user controllable. And IMHO it's good that suggesting omitting error handling is off by default in golangci-lint, even though it's enabled by default in plain errchkjson.