wrapcheck icon indicating copy to clipboard operation
wrapcheck copied to clipboard

Named return values not reported

Open cornfeedhobo opened this issue 3 years ago • 3 comments

Try this out

package main

import (
	"errors"
)

func somethingDangerous() error {
	return errors.New("fake err")
}

func run() (err error) {
	err = somethingDangerous()
	return
}

func main() {
	if err := run(); err != nil {
		println(err)
	}
}

You'll see that wrap check does not detect that err is not being wrapped.

cornfeedhobo avatar Apr 12 '21 00:04 cornfeedhobo

G'day, thanks a lot for the report. This particular example is actually not expected to be wrapped, as calls to errors.New() are explicitly ignored as it is usually called directly as part of the return.

Secondly, the call to somethingDangerous() is in the same package, therefore does not satisfy the invariant of being either 1. a func in a separate package or 2. a method call on an interface.

However, you did bring up a good point with regards to named return values. I have been able to make a similar example which in fact should be wrapped and is not reported. The example is below:

package main

import "encoding/json"

func main() {
	do()
}

func do() (err error) {
	err = json.Unmarshal([]byte(""), nil)
	return // TODO want `error returned from external package is unwrapped`
}

You can see that the error returned by a call to a separate package func json.Unmarshal is returned directly without being wrapped. You're correct that this case is a false negative.

I've written a test case that fails. If you're interesting in contributing a solution that would be massively appreciated, as my time at the moment is a bit on the short side.

Cheers

tomarrell avatar Apr 16 '21 15:04 tomarrell

The test case was added in commit: e75cc2ae87e945ba7eb7a18a983e6c3ad78d2cf5 behind a TODO.

tomarrell avatar Apr 16 '21 15:04 tomarrell

@tomarrell Awesome! Thanks for taking a look. Indeed, if I find time, I'll take a crack at a PR, but my repos are demanding time too 😅. I guess we'll see who gets time first.

Cheers!

cornfeedhobo avatar Apr 16 '21 18:04 cornfeedhobo