Skip to content
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

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

Closed
ldez opened this issue May 12, 2021 · 7 comments
Closed

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

ldez opened this issue May 12, 2021 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Go1.17

Comments

@ldez
Copy link

ldez commented May 12, 2021

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.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 12, 2021
@gopherbot gopherbot added this to the Unreleased milestone May 12, 2021
@bcmills
Copy link
Contributor

bcmills commented May 12, 2021 •
edited

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

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 12, 2021
@bcmills bcmills modified the milestones: Unreleased, Go1.17 May 12, 2021
@ldez
Copy link
Author

ldez commented May 12, 2021

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

@cuonglm
Copy link
Member

cuonglm commented May 12, 2021

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

@gopherbot
Copy link

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

@cuonglm
Copy link
Member

cuonglm commented May 12, 2021

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
Copy link
Contributor

timothy-king commented May 12, 2021 •
edited

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
Copy link

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 20, 2021
gopherbot pushed a commit that referenced this issue May 20, 2021
@timothy-king
cmd: go get golang.org/x/tools/analysis@49064d23 && go mod vendor
baa934d 
This brings in CLs 312829, 317431, 319211.

Fixes #40356.
Fixes #46129.

Change-Id: I2ee1f858b2a41ffa60d88b0c17511ccad57f1816
Reviewed-on: https://go-review.googlesource.com/c/go/+/321389
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Tim King <taking@google.com>
Run-TryBot: Tim King <taking@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
6 participants
@timothy-king @dmitshur @cuonglm @bcmills @ldez @gopherbot