Check errors and enable errcheck in lint config

[c4rlo]

Following up to #2238, this un-disables the errcheck linter in the golangci-lint config, and addresses all complaints by that linter; partly by handling the error, partly by explicitly ignoring it (e.g. _ = f()), and partly through blanket-ignores in the config.

I've additionally taken the opportunity to add some missing error handling to usages of bufio.Scanner, where we were sometimes not calling .Err() to find out if an error happened. This was something I noticed myself.

[joelim-work]

I haven't gone through the changes in details, but I realize now that the list of errors to actually ignore is somewhat debatable in itself. I am leaning towards something like this:

linters:
  settings:
    errcheck:
      exclude-functions:
        # Close
        - (*github.com/fsnotify/fsnotify.Watcher).Close
        - (*net.TCPConn).CloseWrite
        - (*net.UnixConn).CloseWrite
        - (*os.File).Close
        - (io.Closer).Close
        - (net.Conn).Close
        - (net.Listener).Close
        # Flush
        - (*text/tabwriter.Writer).Flush
        # Print
        - fmt.Fprint
        - fmt.Fprintf
        - fmt.Fprintln
        # Remove
        - os.Remove
        # Setenv
        - os.Setenv

Note that for CloseWrite I had to change the code to list the types explicitly:

switch c := c.(type) {
case *net.TCPConn:
	c.CloseWrite()
case *net.UnixConn:
	c.CloseWrite()
}

[c4rlo]

Ok, happy to tune the exclusions. All the Close/CloseWrite methods you mention make sense to me. But regarding these:

    # Flush
    - (*text/tabwriter.Writer).Flush
    # Print
    - fmt.Fprint
    - fmt.Fprintf
    - fmt.Fprintln

I'm not convinced, because all of those act on an io.Writer that could represent various things, i.e. it's not necessarily stdout. Do we really want to ignore errors writing to… anything?

Re os.Remove, it feels like at least reporting such errors would be nice. For os.Setenv, that really should never fail, but again if it unexpectedly does, might be nice to know.

Anyway, it's all debatable as you say, so that's my contribution to the debate 😄

---

Regarding CloseWrite, while what you suggest makes sense for now, it also seems to me that simplify things by replacing net.Dial() by net.DialUnix(), since I don't think communicating with the server over TCP makes sense (and is not actually something the user can select with current command-line flags). I might simplify that in a future PR.

Nevermind, we need TCP mode for Windows.

[joelim-work]

DId you mean to use echoerrf here, or was this intentional to avoid log spam?

[c4rlo]

I did this intentionally because at this point, we've just failed to send a message to connection c, which is what echoerrf would also try to do, so that's unlikely to succeed.

[joelim-work]

echoerrf calls echoerr which writes the error back to the client but also logs the message. I think fmt.Printf won't be seen anywhere since the server runs in the background.

I could be wrong about this though.

[c4rlo]

Ok, makes sense. I've changed it to call log.Printf instead of fmt.Printf now. Does that work?

[joelim-work]

Ok, happy to tune the exclusions. All the Close/CloseWrite methods you mention make sense to me. But regarding these:

    # Flush
    - (*text/tabwriter.Writer).Flush
    # Print
    - fmt.Fprint
    - fmt.Fprintf
    - fmt.Fprintln

I'm not convinced, because all of those act on an io.Writer that could represent various things, i.e. it's not necessarily stdout. Do we really want to ignore errors writing to… anything?

Re os.Remove, it feels like at least reporting such errors would be nice. For os.Setenv, that really should never fail, but again if it unexpectedly does, might be nice to know.

I think these are fair points, there are cases where handling these errors would be useful. Excluding a function directly means all usages are ignored by the linter, while leaving it enabled means that you have to specify _ = foo() or //nolint for the 90% of cases where the error doesn't matter (or create a wrapper function like setenv just to get around the linter conveniently).

These days I try to be accepting of the views of other contributors, so I am fine either way. I will tag @CatsDeservePets to see if he has anything to say about this PR.

[CatsDeservePets]

Thanks for tagging me, @joelim-work.

To be honest, I think these rules are too harsh.
They force us to clutter the source code with explicit ignore statements like _, _ = fmt.Fprintln(c, msg) all over the place (flag.Usage is also a good example).
This makes some code actually harder to read in my opinion as it's visually distracting.

This also makes contributing for less-involved users much harder and more annoying. They might not use a linter and write perfectly fine code, only to get linting errors later one.
How does one explain to them that they should add _ soley to satisfy the linter?

I think it should be up to the contributor (or the reviewer, by extension) to decide whether certain errors can be ignored or not.

[c4rlo]

Ok, I believe I have addressed your comments now. We now have more blanket exclusions, and only a single remaining instance of the (previously admittedly somewhat noisy) _ = f() pattern to selectively exclude errors.

[CatsDeservePets]

Ok, I believe I have addressed your comments now. We now have more blanket exclusions, and only a single remaining instance of the (previously admittedly somewhat noisy) _ = f() pattern to selectively exclude errors.

So overall, it looks much better to me.

Is this still in there for a reason _, _ = os.Stdin.Read(b)?

[c4rlo]

Is this still in there for a reason _, _ = os.Stdin.Read(b)?

The alternative is to blanket-exclude (*os.File).Read. Do we want that? Maybe in future we'll call that function somewhere else (say when reading a config file), and then we probably want to ensure we handle any errors in that context. That's why it makes more sense to me to exclude just this one occurrence.

[CatsDeservePets]

I'd say we can keep it as it is for now.

[joelim-work]

Is this still in there for a reason _, _ = os.Stdin.Read(b)?

The alternative is to blanket-exclude (*os.File).Read. Do we want that? Maybe in future we'll call that function somewhere else (say when reading a config file), and then we probably want to ensure we handle any errors in that context. That's why it makes more sense to me to exclude just this one occurrence.

I am also a bit concerned about blanket-excluding functions like this, this is why _ = function() exists so that a single occurrence can be specifically ignored without affecting any others. For this particular _, _ = os.Stdin.Read(b) call, does it make sense to log errors instead?

[CatsDeservePets]

Is this still in there for a reason _, _ = os.Stdin.Read(b)?

The alternative is to blanket-exclude (*os.File).Read. Do we want that? Maybe in future we'll call that function somewhere else (say when reading a config file), and then we probably want to ensure we handle any errors in that context. That's why it makes more sense to me to exclude just this one occurrence.

I am also a bit concerned about blanket-excluding functions like this, this is why _ = function() exists so that a single occurrence can be specifically ignored without affecting any others. For this particular _, _ = os.Stdin.Read(b) call, does it make sense to log errors instead?

I was thinking about adding #nolint to make it more obvious what it does. But your suggestions is much better. I am in favour of this.

[c4rlo]

Is this still in there for a reason _, _ = os.Stdin.Read(b)?

The alternative is to blanket-exclude (*os.File).Read. Do we want that? Maybe in future we'll call that function somewhere else (say when reading a config file), and then we probably want to ensure we handle any errors in that context. That's why it makes more sense to me to exclude just this one occurrence.

I am also a bit concerned about blanket-excluding functions like this, this is why _ = function() exists so that a single occurrence can be specifically ignored without affecting any others. For this particular _, _ = os.Stdin.Read(b) call, does it make sense to log errors instead?

I was thinking about adding #nolint to make it more obvious what it does. But your suggestions is much better. I am in favour of this.

Done.

[joelim-work]

The changes look fine now, thanks for working on them.

I will give my approval - @CatsDeservePets if you are happy with this as well then please merge the PR.