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

x/tools: shadow analyzer does not consider variables declared in for-range statement #44986

Open
jbert opened this issue Mar 13, 2021 · 3 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@jbert
Copy link

jbert commented Mar 13, 2021

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

$ go version
go version go1.16.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/john/.cache/go-build"
GOENV="/home/john/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/john/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/john/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3861764262=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Installed and ran the shadow analyzer on a file witht the contents below:

$ go get -u -v golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
$ cat tt.go
package main

import (
        "fmt"
)

func main() {
        n := 10
        total := 0
        for _, n := range []int{1, 2, 3} {
                total += n
        }
        fmt.Printf("total is %d, n is %d\n", total, n)
}

$ shadow tt.go
$ 

No warning was generated

What did you expect to see?

A message similar to:

/home/john/tt.go:10:9: declaration of "n" shadows declaration at line 8

What did you see instead?

No output from the shadow tool.

@jbert
Copy link
Author

jbert commented Mar 13, 2021

I have a basic patch which considers range statements and successfully generates the warning above:

https://github.com/jbert/tools/tree/jb/shadow-check-range-vars

Is this appropriate to raise as a PR?

@timothy-king timothy-king added Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest labels Mar 13, 2021
@timothy-king timothy-king added this to the Unplanned milestone Mar 13, 2021
@timothy-king
Copy link
Contributor

Supporting range statements in shadow.Analyzer sounds reasonable to me. It is not super far from the example given in the doc comment for the checker: https://github.com/golang/tools/blob/fe37c9e135b934191089b245ac29325091462508/go/analysis/passes/shadow/shadow.go#L33-L43
It is not obvious to me that it is going to break something that is currently idiomatic. For the criteria https://golang.org/src/cmd/vet/README , this is roughly in line with the existing shadow Analyzer. My guess is that this will be less frequent than the existing shadow cases, but is frequent enough to warrant the incremental cost of examining more statements for folks already running shadow.

You can send the PR to me to review.

@jbert
Copy link
Author

jbert commented Mar 14, 2021

Sorry, unsure/don't have privs to assign to you.

PR link is here: golang/tools#287

@mdlayher mdlayher changed the title shadow analyzer does not consider variables declared in for-range statement x/tools: shadow analyzer does not consider variables declared in for-range statement Mar 14, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 14, 2021
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants