Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/vet: -shadow false positive #19490

Open
thinkeridea opened this issue Mar 10, 2017 · 8 comments
Open

cmd/vet: -shadow false positive #19490

thinkeridea opened this issue Mar 10, 2017 · 8 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@thinkeridea
Copy link

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 josharian changed the title cmd/vet Variable scope inspection problems cmd/vet: -shadow false positive Mar 10, 2017
@josharian
Copy link
Contributor

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.

@josharian josharian added this to the Unplanned milestone Mar 10, 2017
@Vavilen Vavilen mentioned this issue Dec 21, 2017
Closed
quasilyte added a commit to quasilyte/go-contributing-ru that referenced this issue Apr 1, 2018
@quasilyte
tasks: add several new tracked tasks
11e19f1 
New tasks include:
golang/go#19675 cmd/vet: report uses of -0 in float32/64 context
golang/go#19683 cmd/compile: eliminate usages of global lineno
golang/go#19670 x/tools/go/ssa: make opaqueType less annoying to use
golang/go#19636 encoding/base64: decoding is slow
golang/go#23471 x/perf/cmd/benchstat: tips or quickstart for newcomers
golang/go#19577 test: errorcheck support for intraline errors
golang/go#19490 cmd/vet: reduce the amount of false positives for -shadow mode
golang/go#19042 cmd/internal/obj: optimize wrapper method prologue for branch prediction
golang/go#19013 cmd/compile: add tool for understanding/debugging SSA rules
@ALTree ALTree added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 22, 2018
@magodo
Copy link

magodo commented Mar 27, 2019

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

@bamarni bamarni mentioned this issue Sep 4, 2019
Merged
@LasTshaMAN
Copy link

Any plans to fix this yet ?

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

@ianlancetaylor
Copy link
Contributor

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

@LasTshaMAN
Copy link

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
Copy link
Contributor

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

@dtitov dtitov mentioned this issue Jun 10, 2020
Merged
@hitzhangjie
Copy link
Contributor

hitzhangjie commented Jan 7, 2023 •
edited

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
Copy link
Contributor

@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?

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
9 participants
@josharian @timothy-king @ianlancetaylor @ALTree @hitzhangjie @adonovan @LasTshaMAN @magodo @thinkeridea