tomarrell/wrapcheck

Incorrect alert on io.Reader implementation

stevenh opened this issue · 5 comments

stevenh commented

If you have code which implements io.Reader that delegates the call, then currently wrapcheck throws a warning.

func (c *sshConnection) Read(p []byte) (n int, err error) {
	return c.channel.Read(p)
}

This caused us to implement wrapping, but unfortunately this causes issues with code such as bufio.Scanner.Err() as the error is now not io.EOF but a wrapped io.EOF causing it to incorrectly return an error instead of nil.

I originally raised this as bug in bufio.Scanner however that was rejected as io.Reader is documented as returning io.EOF and not a wrapped io.EOF.

Given this I believe delegated methods which form part of io and in particular io.Reader should not be reported as it will lead to similar issues.

tomarrell commented

In these specific examples, I would suggest using an ignore directive. If you're using golangci-lint, that would be //nolint:wrapcheck.

As it is not possible for wrapcheck to infer the intentioned use of the method (possibly imported as part of a completely separate package), it must be best effort.

If channel is an interface, then you can look at using an ignore directive on that interface. Wrapcheck will then ignore reporting errors which are returned by that particular interface. This may be a potential proxy for your specific codebase.

Let me know if you have any further concerns.

stevenh commented

Thanks for the response.

My concern is that folks take these warnings as gospel and hence wouldn't add a nolint directive, much like in the cases I've found. This means that instead of improving the code, warnings in these case are causing developers to introduce hard to debug problems.

Would it be possible for wrapcheck to compare the enclosing method signature against io.Reader and if detected either not warn or at least omit a warning which details the risk of wrapping a io.Reader method, so the developer can make an informed decision?

tomarrell commented

Thanks a lot for the description.

I understand the dilemma here. On one side, this is not an isolated issue, and there will be other interfaces out there which share the problem of having dependents on their concrete returned errors. I can also understand that this is one of the more likely occurrences.

If you have the time and want to open a PR which writes to stdout with a warning, I would be open to that option, but I don't think omitting the check entirely is desirable given I feel the nolint directive is more suitable for edge cases such as this.

  • 👍1
mitar commented

I was also bitten by this. Blindly wrapping all errors and then I discovered that io.EOF is a special case which should not be wrapped, see: golang/go#39155

I think this should be at least documented with large font in the README that io.EOF is an exception which should not be wrapped.

tomarrell commented