GO-S2307 False Positives

Feature Requests
1

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).

1 Like
2

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.

3

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.

4

Hey @kgillette!

We have rolled out a fix for GO-S2307. Now it won’t be raised for identifiers satisfying io.ReadCloser and io.ReadSeekCloser. We have made some more improvements.

Thank you for helping us make GO-S2307 better.

1 Like
5

Hi, I’m still seeing warnings today on defer rc.Close() for an rc io.ReadCloser. Did the bug re-surface?

6

Hi @benwaffle

Can you please paste the snippet where the issue is being raised. It will help us investigate this further.

7

You can also mark an issue as False Positive on the DeepSource dashboard, and we can receive the required context that way.

8

This is still happening for files returned from os.Open which returns a read only os.File. e.g

func test(file string) error {
	f, err := os.Open(file)
	if err != nil {
		return err
	}

	defer f.Close()

	// Do something...

        return nil
}