GO-S2307 False Positives

Hello!

GO-S2307 regularly shows up for conventional well written code when read-only closers are involved. Since read-only closers are the majority case (by far), this analyzer produces false positives a seeming majority of the time, and should be tweaked to have better understanding of the underlying code.

Consider some common cases:

  1. I’m opening a file for reading: if file corruption exists, I will find out before closing, and thus any error that occurs on file close is of no consequence (I merely wish to free the resource).
  2. I’ve got a ReadCloser pertaining to the read-side of a network connection: similarly, I will encounter an EOF or find out about any other issues merely through Read calls; there is no concept of a “read flush on close” and I, likewise, merely wish to free the resource when I’m done with it.

This is not worth signaling via a skipcq since code performing such read-only operations is not in the wrong. defer rc.Close() is perfectly conventional, and forcing error checking is also unreasonable in these cases.

Please improve this analyzer to only raise issues for io.WriteCloser (or io.Closer instances that aren’t identifiable as io.ReadCloser).

Special care should also be taken around os.File and other common use-cases. An os.File opened with os.Open should not be considered a true-positive for this check, while other constructors (such as os.Create) would be true-positives.

Hey @kgillette! Thanks for taking the time to provide feedback for GO-S2307. Yes, I do agree with the points you have written. GO-S2307 needs to be improved. We’ll pick it up as soon as possible and let you know once it is fixed.