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
Open

Comments

@jbert
Copy link

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

@timothy-king timothy-king commented Mar 13, 2021

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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants