x/tools/go/analysis/passes/modernize: slicesbackward: invalid variable shadowing in loop

[creachadair]

Go version

go version go1.27.0 darwin/arm64

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE='on'
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN='<redacted>/go/bin'
GOCACHE='<redacted>/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='<redacted>/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ht/pthg90p54xx6vf8ktlv9nqbc0000gn/T/go-build432207319=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='<redacted>/go.mod'
GOMODCACHE='<redacted>/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPACKAGESDRIVER=''
GOPATH='<redacted>/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org'
GOROOT='/usr/local/go/current'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='<redacted>/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/current/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.27.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

• Updated to Go 1.27
• Run go fix ./...
• Run go test ./...

What did you see happen?

The fix applied the following diff, which no longer compiles:

-       for i := len(s.entries) - 1; i >= 0; i-- {
+       for i, v := range slices.Backward(s.entries) {
                col := s.table[i] // this entry's column in the solution table

                for j := range col {
                        // Value and gain of j shares of this entry.
-                       v := s.entries[i].Value * currency.Value(j)
-                       g := s.entries[i].Gain * currency.Value(j)
+                       v := v.Value * currency.Value(j)
+                       g := v.Gain * currency.Value(j)

Citing:

solver/solver.go:86:11: v.Gain undefined (type currency.Value has no field or method Gain)

The reuse of the name v would ordinarily be fine, since the inner scope has its own binding, but in this case the (existing) inner binding wound up shadowing the variable chosen by the fix, and the types no longer match.

What did you expect to see?

I would have been content with a diagnostic saying "I couldn't apply this fix". Ideally, the fix would have noticed the inner binding has a different type, and chosen a fresh name for the loop variable that would not conflict with that.

Such a collision won't always cause a problem, and could be avoided if the types match, but that may be too tedious for a fix to chase down.

[gabyhelp]

Related Issues

• cmd/fix: semantic conflict with nested (stditerators) fixes #78617
• cmd/fix: mapsloop analyzer emits = instead of := in maps.Collect path #78322 (closed)
• cmd/fix: `go fix` inline producing conflicting var names #78432
• cmd/fix,x/tools/go/analysis/passes/modernize: stringscut does not fully recognize variable definition reuse #77566 (closed)
• x/tools/go/cfg: loop variable declarations are misplaced for 1.22+ loops #73278
• cmd/fix, x/tools/go/analysis/passes/modernize: backwardloop makes incorrect fix in some situations #78868 (closed)
• x/tools/go/analysis/passes/modernize: slicesbackward suggested fix changes the value of assignment-form loop variables #80838 (closed)
• x/tools/gopls/internal/analysis/modernize: rangeint: transformation unsound when loop variable is defined outside loop #73009 (closed)
• x/tools/go/analysis/passes/modernize: slicesbackward: fix creates unreferenced local var #78629 (closed)
• cmd/fix,x/tools/go/analysis/passes/modernize: stringcut "foreshadows" an existing "ok" var #77770 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[kavix]

I’d be happy to work on this.

The issue appears to be in slicesbackward’s generated-name handling. The analyzer chooses v for selector expressions such as s.entries, then calls freshName using the loop scope and the loop position. That catches names already visible before the loop, but not declarations introduced inside the loop body. Consequently, the suggested fix can introduce a new range variable named v while the body already declares another v:

for i, v := range slices.Backward(s.entries) {
    // existing body declaration now shadows the generated range variable
    v := s.entries[i].Value * currency.Value(j)
    g := v.Gain * currency.Value(j)
}

A conservative fix would scan declarations in the rewritten body and choose a fresh element-variable name whenever the preferred name is declared there. This avoids requiring type-sensitive analysis and is safer than accepting a potentially invalid or semantically altered rewrite. The suggested fix should also be declined if a conflict-free name cannot be established.

I would add a regression fixture where the body contains declarations such as v := s[i].Value and g := s[i].Gain, with the golden output expecting something like:

for _, v0 := range slices.Backward(s) {
    v := v0.Value
    g := v0.Gain
}

[gopherbot]

Change https://go.dev/cl/815162 mentions this issue: cmd/fix: avoid shadowed variables in slicesbackward rewrite