x/tools/go/analysis/passes/sigchanyzer: alteration of the AST

[ldez]

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

$ go version
go version go1.16.4 linux/amd64

Does this issue reproduce with the latest release?

yes, but the issue is not related to Go itself.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ldez/.cache/go-build"
GOENV="/home/ldez/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ldez/sources/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ldez/sources/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/ldez/.gvm/gos/go1.16.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/ldez/.gvm/gos/go1.16.4/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/ldez/sources/go/src/golang.org/x/tools/go.mod"
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-build184925693=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I use go vet passes into golangci-lint.
sigchanyzer produces an unexpected behavior with the buildssa.Analyzer, I think it's because it alters the AST.

The problem comes from the following lines:
https://github.com/golang/tools/blob/be4aaae4cf865bd1e65ae3a22df28c1f47b6ccfd/go/analysis/passes/sigchanyzer/sigchanyzer.go#L62-L65

More details here: golangci/golangci-lint#1973
Steps to reproduce: golangci/golangci-lint#1973 (comment)

What did you expect to see?

No impact on the buildssa.Analyzer.

What did you see instead?

Impact on the buildssa.Analyzer.

[bcmills]

sigchanyzer is new as of Go 1.17. Does this affect other analyses in cmd/vet?
(Marking as release-blocker until we have an answer to the above question.)

CC @cuonglm @odeke-em @stamblerre

[ldez]

I created a potential fix: https://go-review.googlesource.com/c/tools/+/319211

[cuonglm]

@bcmills No, the other two passes that require buildssa.Analyzer are nilness and unusedwrite.

[gopherbot]

Change https://golang.org/cl/319211 mentions this issue: go/analysis/passes/sigchanyzer: fix alteration of the AST

[cuonglm]

It's unclear to me though why modifying AST in one pass (sigchanyzer) will affect the other buildssa? Is there any documentation for this behavior?

[timothy-king]

sigchanyzer is new as of Go 1.17. Does this affect other analyses in cmd/vet?

Yes.

tl;dr Don't modify the ast from an Analyzer.**

Modifying anything reachable from a Pass.Files during an Analyzer.Run(*Pass), let's call this A, races with every Analyzer B where A and B are not related by an Analyzer.Requires field. These are not working on unique copies of the AST for each pass but a shared one for each package.Package loaded.

Definition of action.pkg:
https://github.com/golang/tools/blob/v0.1.1/go/analysis/internal/checker/checker.go#L534
Example place analysis forwards action.pkg.Syntax: https://github.com/golang/tools/blob/v0.1.1/go/analysis/internal/checker/checker.go#L640
Where inspector gets the ast from pass.Files: https://github.com/golang/tools/blob/v0.1.1/go/analysis/passes/inspect/inspect.go#L48

There is nothing special about buildssa it just happens to be independently runnable from sigchanyzer.

** Unless you know all of the Analyzers being run and are in a Requires relationship to all of them.

[gopherbot]

Change https://golang.org/cl/321389 mentions this issue: cmd: go get golang.org/x/tools/analysis@49064d23 && go mod vendor