fmt.Print* should be checked for errors

[egonk]

Use case: command line apps that write actual data on stdout (not just logs), error on write indicates that the data processing should stop or report error on stderr. e.g. "cat"

The exclude list is combined with an internal list for functions in the Go standard library that have an error return type but are documented to never return an error.

If my reading of stdlib docs & code is correct, fmt.Print* may return errors just like fmt.Fprint*(os.Stderr) for example.

Possible solutions

1. remove fmt.Print* from exclude list, cons: most correct (IMO) but annoyance for existing users
2. add -strict flag to remove fmt.Print* from exclude list and any such future cases
3. ...?

Thoughts?

[egonk]

To illustrate the issue:

package main

import (
	"bufio"
	"fmt"
	"os"
	"os/exec"
)

func main() {
	if len(os.Args) >= 2 && os.Args[1] == "writetest" {
		for {
			_, err := fmt.Println("test")
			p(err) // write /dev/stdout: The pipe is being closed.
		}
	}
	c := exec.Command("./stdouterr", "writetest")
	outp, err := c.StdoutPipe()
	p(err)
	errp, err := c.StderrPipe()
	p(err)
	go func() {
		scan := bufio.NewScanner(errp)
		for scan.Scan() {
			_, err := fmt.Printf("STDERR [%v]\n", scan.Text())
			p(err)
		}
	}()
	p(c.Start())
	scan := bufio.NewScanner(outp)
	for i := 0; i < 10 && scan.Scan(); i++ {
		_, err := fmt.Printf("STDOUT [%v]\n", scan.Text())
		p(err)
	}
	_, err = fmt.Printf("CLOSE STDOUT\n")
	p(err)
	p(outp.Close())
	_, err = fmt.Printf("WAIT\n")
	p(c.Wait())
}

func p(err error) {
	if err != nil {
		panic(err)
	}
}

Output:

STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
STDOUT [test]
CLOSE STDOUT
WAIT
STDERR [panic: write /dev/stdout: The pipe is being closed.]
STDERR []
STDERR [goroutine 1 [running]:]
STDERR [main.p(0x4fa1a0, 0xc0420701e0)]
STDERR [        C:/i/tmp/go/src/x/stdouterr.go:44 +0x51]
STDERR [main.main()]
STDERR [        C:/i/tmp/go/src/x/stdouterr.go:14 +0xe1]
panic: exit status 2

goroutine 1 [running]:
main.p(0x4fa200, 0xc04208a020)
        C:/i/tmp/go/src/x/stdouterr.go:44 +0x51
main.main()
        C:/i/tmp/go/src/x/stdouterr.go:39 +0x44b

[kisielk]

I think that in 99.99% of cases flagging missing error returns from fmt.Printf is a nuisance. For the other cases I think we could add a flag to discard to built-in list and only use explicitly-provided ignores.

[kisielk]

I propose calling the flag -exclude-only

[egonk]

I think that in 99.99% of cases flagging missing error returns from fmt.Printf is a nuisance. For the other cases I think we could add a flag to discard to built-in list and only use explicitly-provided ignores.

Maintained built-in list is very useful otherwise, we use buffers a lot for example. I would prefer not to lose it :)

[kisielk]

Sure, but you could always copy it to your custom list

[egonk]

Sure, but you could always copy it to your custom list

... or just vendor errcheck (I do that already) and change the default excludes so it's available out of the box, but that's what I wanted to avoid. I assumed there might be some interest in "standard" strict mode, but maybe there isn't. Instead of -strict flag, this would be more future-proof:

-builtin-excludes default (current behaviour, default)
-builtin-excludes strict
-builtin-excludes none

[dragonsinth]

I might argue that code writers should consider explicitly FPrint-ing to os.Stdout in these cases.

[amnonbc]

I recently had to fork this repo to stop it complaining about idiomatic code such as defer resp.Body.Close()

So I would urge the maintainers to avoid doing anything which increases false alarms,
and reduces the signal to noise ratio of the tool's output.

[dragonsinth]

We use a custom exclusion list for this, but generally speaking I think it'd be nice if defering io.Close() was ok by default.

[amnonbc]

I think it'd be nice if defering io.Close() was ok by default.

feel free to use my patch amnonbc@cabf3a5

or even to use my fork https://github.com/amnonbc/errcheck