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

[jbert]

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]

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]

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]

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

PR link is here: https://github.com/golang/tools/pull/287