search

dominikh / go-tools

Staticcheck - The advanced Go linter
https://staticcheck.dev
MIT License
6.13k stars 373 forks source link

Multiple opportunities missed with a shadowed map variable. (shadowed variable, unused map writes, constant len(nil map)) #1520

Open jvmatl opened 5 months ago

jvmatl commented 5 months ago

Description

The following code (obviously extracted and minimized) contains a bug that was not detected/reported by staticcheck:

func MinimalRepro(option bool) int {
    var m map[int]struct{}

    if option {
        m := make(map[int]struct{}) // BUG -- intended to be =, not :=
        m[1] = struct{}{}
        // m is never read from within this block, and len(m) is never analyzed, so this  accomplishes nothing
    }

    return len(m)
}

The bug, of course, is that the shadowing of the map, m was unintentional - the map allocation/assignment happens inside the block because the programmer was trying to defer the map allocation until it was needed, but := was accidentally used instead of =

It seems to me that there are at least three reasons why this code should cause a warning/alert:

  1. the outer map variable, m, is shadowed inside the if option{ block -- I get that shadowing is legal and a stylistic issue, but I would think that this causes enough problems in general that there ought to be at least an optional 'style' flag to warn about shadowed variables? If there is such a flag, I did not see it in the documentation.

  2. the inner map, m is only ever assigned to: nobody ever tries to read from the map, and nobody examines len(m), so writes to the map are pointless, and this could have been flagged as 'code that has no effect'

  3. since m is shadowed, the return statement return len(m) ALWAYS evaluates to len(nil map), which is a constant 0, and taking len of any invariant expression is almost certainly a bug.

Obligatory boilerplate

The output of 'staticcheck -version'

staticcheck 2023.1.7 (v0.4.7)

The output of 'staticcheck -debug.version' (it is fine if this command fails)

staticcheck 2023.1.7 (v0.4.7)

Compiled with Go version: go1.22.2 Main module: honnef.co/go/tools@v0.4.7 (sum: h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=) Dependencies: github.com/BurntSushi/toml@v1.2.1 (sum: h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=) golang.org/x/exp/typeparams@v0.0.0-20221208152030-732eee02a75a (sum: h1:Jw5wfR+h9mnIYH+OtGT2im5wV1YGGDora5vTv/aa5bE=) golang.org/x/mod@v0.12.0 (sum: h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc=) golang.org/x/sys@v0.11.0 (sum: h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM=) golang.org/x/tools@v0.12.1-0.20230825192346-2191a27a6dc5 (sum: h1:Vk4mysSz+GqQK2eqgWbo4zEO89wkeAjJiFIr9bpqa8k=)

The output of 'go version'

go version go1.22.2 linux/amd64

The output of 'go env'

GO111MODULE='' GOARCH='amd64' GOBIN='' GOCACHE='/go/code/.cache/alpine-gobe/go-build' GOENV='/.config/go/env' GOEXE='' GOEXPERIMENT='' GOFLAGS='' GOHOSTARCH='amd64' GOHOSTOS='linux' GOINSECURE='' GOMODCACHE='/go/pkg/mod' GONOPROXY='' GONOSUMDB='' GOOS='linux' GOPATH='/go' GOPRIVATE='' GOPROXY='https://proxy.golang.org,direct' GOROOT='/usr/local/go' GOSUMDB='sum.golang.org' GOTMPDIR='' GOTOOLCHAIN='local' GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64' GOVCS='' GOVERSION='go1.22.2' GCCGO='gccgo' GOAMD64='v1' AR='ar' CC='gcc' CXX='g++' CGO_ENABLED='1' GOMOD='/go/code/go.mod' GOWORK='' CGO_CFLAGS='-O2 -g' CGO_CPPFLAGS='' CGO_CXXFLAGS='-O2 -g' CGO_FFLAGS='-O2 -g' CGO_LDFLAGS='-O2 -g' PKG_CONFIG='pkg-config' GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3883420805=/tmp/go-build -gno-record-gcc-switches'

Exactly which command you ran

staticcheck -tags osusergo,netgo ./...

Output of the command and what's wrong with the output

no output - expected some sort of error

Where we can read the code you're running Staticcheck on

See embedded code, above. go playground shortcut for convenience: https://go.dev/play/p/r4YrMQpmmbj

dominikh commented 5 months ago

I get that shadowing is legal and a stylistic issue, but I would think that this causes enough problems in general that there ought to be at least an optional 'style' flag to warn about shadowed variables? If there is such a flag, I did not see it in the documentation

That is out of scope for Staticcheck.

the inner map, m is only ever assigned to: nobody ever tries to read from the map, and nobody examines len(m), so writes to the map are pointless, and this could have been flagged as 'code that has no effect'

This is something we could add a check for.

since m is shadowed, the return statement return len(m) ALWAYS evaluates to len(nil map), which is a constant 0, and taking len of any invariant expression is almost certainly a bug.

This is something we could add a check for, as well.