bogus io.EOF error when error var is predeclared

[kolyshkin]

I have previously submitted #12 hoping it would fix to eliminate a bogus error in my code.

Today I found it did not.

The following code is fine (i.e. no warnings):

func UseBufioReadLine(r io.Reader) error {
       rd := bufio.NewReader(r)
       for {
               _, _, err := rd.ReadLine()
               if err != nil {
                       if err == io.EOF {
                               err = nil
                       }
               }
               return err
       }
}

but if we modify it in this way:

 func UseBufioReadLine(r io.Reader) error {
        rd := bufio.NewReader(r)
+       var err error
        for {
-               _, _, err := rd.ReadLine()
+               _, _, err = rd.ReadLine()
                if err != nil {
                        if err == io.EOF {

we will have something like

func UseBufioReadLine(r io.Reader) error {
          rd := bufio.NewReader(r)
          var err error
          for {
                  _, _, err = rd.ReadLine()
                  if err != nil {
                          if err == io.EOF {
                                  err = nil
                          }
                  }
                  return err
          }
  }

and this code results in a warning:

comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error

Alas, I don't know internals good enough to try to fix it.

[kolyshkin]

Similarly, for existing code in errorlint/testdata/src/allowed/allowed.go, the following change results in a warning.

@@ -113,7 +114,9 @@ type CompressedFile struct {
 }
 
 func (c CompressedFile) Read(p []byte) (int, error) {
-       n, err := c.reader.Read(p)
+       var n int
+       var err error
+       n, err = c.reader.Read(p)
        if err == io.EOF {
                return n, io.EOF
        }

[kolyshkin]

Happens because callExpr in isAllowedErrorComparison is nil. This is where my debug abilities stop 🤡

[polyfloyd]

The allowed error set is implemented by checking comparisons for whether an error is returned from a known safe function.

The current implementation uses the declaration of the error variable to see whether it comes from such a function. In your case, the declaration is a variable declaration without initialisation for which no logic is implemented yet.

We can fix this by not only checking the declaration, but also the last assignment to the error variable. Checking only the last assignment also requires understanding the control flow, which would make this linter way more complex than I am able to support. So I would propose an implementation which just checks all assigning expressions instead.

Will do some hacking when I have time, unless someone beats me to it :P

[polyfloyd]

@kolyshkin I have pushed a fix to branch issue-15. Could you please test whether this solves your issue?

[polyfloyd]

I have done some more testing on my own projects and it seems to work well. Merged my changes :)