False positive if err variable is reused/redeclared

[kolyshkin]

I am seeing an false positive in the following code (example.go):

package main

import (
        "fmt"
        "io"
        "os"
)
  
func OsPipeFileReadReuseErrVar() {
        var buf [4096]byte
        r, _, err := os.Pipe()
        _, err = r.Read(buf[:])
        if err == io.EOF {
                fmt.Println(err)
        }
}

With the following added diagnostics:

diff --git a/errorlint/allowed.go b/errorlint/allowed.go
index d4274b8..faa0d61 100644
--- a/errorlint/allowed.go
+++ b/errorlint/allowed.go
@@ -126,6 +126,7 @@ func isAllowedErrorComparison(info *TypesInfoExt, binExpr *ast.BinaryExpr) bool
        // All assignments done must be allowed.
        for _, funcName := range functionNames {
                if !isAllowedErrAndFunc(errName, funcName) {
+                       fmt.Printf("err: %s fun: %s\n", errName, funcName)
                        return false
                }
        }

I get this result:

$ go build .
$ ./go-errorlint ./example.go 
err: io.EOF fun: os.Pipe
/home/kir/go/src/github.com/polyfloyd/go-errorlint/example.go:13:5: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error

So, the linter thinks err comes from os.Pipe while actually it is from (*os.File).Read.

NOTE this is not a regression in cd16050dfa0abd24e98f71308f3d6e5d19aeadb8 (reproduced on both git tip as well as v1.4.3).

[polyfloyd]

Hi!

If I recall correctly, the linter prefers the expression of the error's declaration to check with the allowlist. Reuse of the error variable by other assignments could be difficult as it would need to have knowledge of the control flow. Your example has the assignments happening in a predictable sequence, but what if branches or loops are involved?

I personally prefer writing function calls returning errors like this:

        if _, err := r.Read(buf[:]); err == io.EOF {
                fmt.Println(err)
        }

This should mitigate the reassignment problem as the error from r.Read now lives as a new variable. Not a solution, but it likely reduces the number of times one runs into this.

[kolyshkin]

Alas, in some cases this is not possible. Here's a real world example:

        pipeR, pipeW, err := os.Pipe()  
        if err != nil {  
                return err  
        }  

        // ...

       // Read a single byte expecting EOF.  
        var buf [1]byte 
        n, err := pipeR.Read(buf[:]) 
        if n != 0 || err == nil { 
                return errUnexpectedRead  
        } else if errors.Is(err, os.ErrDeadlineExceeded) {  
                // Probably the other end doesn't support the sd_notify_barrier protocol.  
                logrus.Warn("Timeout after waiting 30s for barrier. Ignored.")  
                return nil  
        } else if err == io.EOF {
                return nil  
        } else {  
                return err  
        }  

This is sort of hard to rewrite to avoid a warning (without using a new variable, like err2).

Anyway, I guess this is not something that can be solved easily by this linter.