Skip to content

cmd/vet: false positive of self-assignment #54840

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.

Sign up for GitHub

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

Jump to bottom
Closed
afriza opened this issue Sep 2, 2022 · 7 comments
Closed

cmd/vet: false positive of self-assignment #54840

afriza opened this issue Sep 2, 2022 · 7 comments
Assignees
@timothy-king
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@afriza
Copy link

afriza commented Sep 2, 2022 •
edited
Loading

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

$ go version
go version go1.19 darwin/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="/Users/username/Library/Caches/go-build"
GOENV="/Users/username/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/username/go/pkg/mod"
GONOPROXY="gitlab.com/orgname"
GONOSUMDB="gitlab.com/orgname"
GOOS="darwin"
GOPATH="/Users/username/go"
GOPRIVATE="gitlab.com/orgname"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/username/Code/project/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/r3/9d9f8ggn5t1_vd8ht7425w9c0000gn/T/go-build3919061805=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://go.dev/play/p/TBs0FGA9Ry8

package main

func main() {
	m := make(map[string][]string)
	m["s"] = m["s"]
}
$ go vet
# test
./main.go:5:2: self-assignment of m["s"] to m["s"]

What did you expect to see?

nothing

What did you see instead?

./main.go:5:2: self-assignment of m["s"] to m["s"]

I thought this is supposed to be fixed. Looks similar to #22174
@seankhliao

This comment was marked as outdated.

Sign in to view
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2022
@randall77
Copy link
Contributor

No, this is a false positive. The thing that looks like a self-assignment modifies m to have length 1 (maps "s" to nil).

@randall77 randall77 reopened this Sep 2, 2022
@heschi
Copy link
Contributor

heschi commented Sep 2, 2022

cc @matloob @timothy-king

@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 2, 2022
@heschi heschi added this to the Go1.20 milestone Sep 2, 2022
@timothy-king
Copy link
Contributor

I do not find the example given in the issue all that compelling. m["s"] = nil is obviously better. Here is a slightly contrived, but more compelling example:

// AddKeys adds keys as keys in the map m. Returns true if any keys were added.
func AddKeys(m map[string]interface{}, keys []string) bool {
  l := len(m)
  for _, k := range keys {
    m[k] = m[k]
  }
  return l >= len(m)
}

The root cause is that analysisutils.HasSideEffect is applied the lhs and rhs independenly by the assign pass and does not take into account the side effect of an assign statement to an index on a map.

I think we do not want vet warning in this case.

@timothy-king timothy-king added NeedsFix The path to resolution is known, but the work has not been done. Analysis Issues related to static analysis (vet, x/tools/go/analysis) and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 2, 2022
@afriza
Copy link
Author

afriza commented Sep 2, 2022 •
edited
Loading

My example was minimum reproducible code..
My actual use case was: if map content exists keep it, if the key doesn't exist, use zero value (nil).

Alternative implementation for my case would be

if _, found := m["s"]; !found {
        m["s"] = nil
}

@timothy-king
Copy link
Contributor

Right. I just wanted an example where "if map content exists keep it, if the key doesn't exist, use [zero value]" is clearly the desired operation. This made being a "false positive" more obvious (at least to me).

@timothy-king timothy-king self-assigned this Sep 19, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/438796 mentions this issue: go/analysis/passes/assign: fix map assignment

@golang golang locked and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@timothy-king timothy-king

Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
6 participants
@afriza @timothy-king @randall77 @gopherbot @seankhliao @heschi