cmd/vet: -shadow false positive

[thinkeridea]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.1 darwin/amd64

A sample code:

package main

import (
	"fmt"

	"github.com/pkg/errors"
)

func main() {
	var err error

	defer func() {
		// Rear the mistake here is just a local error in processing logic, does not belong to the end to unified handling errors
		if err := errors.New(""); err != nil {
			fmt.Println(err)
		}

		// Business logic mistakes need to deal with here
		fmt.Println(err)
	}()

	if err = errors.New(""); err != nil {
		return
	}

	if err = errors.New(""); err != nil {
		return
	}

}

Run the command go tool vet -shadow *.go to get :

sample.go:14: declaration of "err" shadows declaration at sample.go:10

I think there is no question of such code, in the main function in the defer if caused by using the local err statement.

[josharian]

Thanks for the report. The -shadow flag is known to have many false positives. That is why it is marked as experimental. I'm going to mark this as unplanned, meaning there are no concrete plans to fix it, but leave it open as a reference for any future efforts to reduce -shadow false positives.

[magodo]

Is it possible to somehow allow user to grant such false positives (such as the shellcheck disable directives)?

[LasTshaMAN]

Any plans to fix this yet ?

It really seems unnecessary to me checking for error shadowing ...

[ianlancetaylor]

Honestly I don't see why this is a false positive. The inner err really does shadow the outer one.

[LasTshaMAN]

It isn't a "false positive", and inner err indeed shadows the outer one,

it's just that it is very common to have code like from example above that isn't harmful in any way, and still one has to find out linter failing on it, and come up with another name for err to just satisfy the linter ...

have you seen any harmful shadowing with respect to error anywhere ?

[ianlancetaylor]

I'm fine with coming up with some rule for when we should not report shadowing, but what should that rule be? I don't think it should be "don't report shadowing if the variable is named err".

[hitzhangjie]

I agree with @ianlancetaylor , it is indeed a shadowing case. We can add a //nolint:govet to skip the checking.

In some other cases, like:

func() error {
  var err error
  {
     // Here maybe we want to use the error as the final returned error, if so, here is a bug
      err := doSomething()  
  }

  return err
}

Reporting shadowing err is useful, now I am using //nolint:govet to skip or use a different short name e instead of err.

[timothy-king]

@hitzhangjie This example do not build https://go.dev/play/p/KVsmDUICNui . It fails with ./prog.go:16:3: err declared but not used. Did you have a different example in mind?