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.

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

x/tools/go/analysis/passes/shadow/cmd/shadow: shadow inconsistent failures when var in code shadows a var in a test file #34557

Closed
leighmcculloch opened this issue Sep 26, 2019 · 4 comments

Comments

@leighmcculloch
Copy link
Contributor

@leighmcculloch leighmcculloch commented Sep 26, 2019

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

$ go version
go version go1.13 linux/amd64

The docker image that was in use was: circleci/golang:1.13-stretch and with digest sha256:728d4283948f6f2666c6dbdcb56648a5db9056431265d296175449463a82c072.

You can get that specific version of the image with:

$ docker pull circleci/golang@sha256:728d4283948f6f2666c6dbdcb56648a5db9056431265d296175449463a82c072

Does this issue reproduce with the latest release?

I was not able to reproduce locally with go1.13 or go1.13.1.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/circleci/.cache/go-build"
GOENV="/home/circleci/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build650242782=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran shadow ./... in the root repository of this revision of stellar/go@c638a26.

I first installed shadow using go modules in a tmp directory, then changed back to the project and run shadow ./....

What did you expect to see?

Consistently either report no errors, or consistently report an error.

What did you see instead?

Some runs pass the shadow checker, some runs fail.

An example of a failing run:

/tmp/tmp.AztFEDruDz /go/src/github.com/stellar/go
go: creating new go.mod: module tool
go: finding golang.org/x/tools latest
go: downloading golang.org/x/tools v0.0.0-20190925230517-ea99b82c7b93
go: extracting golang.org/x/tools v0.0.0-20190925230517-ea99b82c7b93
/go/src/github.com/stellar/go
/go/src/github.com/stellar/go/keypair/main.go:95:2: declaration of "seed" shadows declaration at line 19

See https://app.circleci.com/jobs/github/stellar/go/8686.

The version of shadow being used is from commit golang/tools@ea99b82.

Line 95 of main.go defines seed in a function:

$ sed '94,95!d' keypair/main.go
func FromRawSeed(rawSeed [32]byte) (*Full, error) {
        seed, err := strkey.Encode(strkey.VersionByteSeed, rawSeed[:])

Line 19 of main.go is a comment:

$ sed '19!d' keypair/main.go
        // through malformation or if it does not verify the message against the

However as @bartekn points out in stellar/go#1778 (comment), seed is defined in main_test.go.

$ sed '17,19!d' keypair/main_test.go
var (
        address   = "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H"
        seed      = "SDHOAMBNLGCE2MV5ZKIVZAQD3VCLGP53P3OBSBI6UN5L5XZI5TKHFQL4"

Issues here:

  • The output of the message is confusing because it doesn't mention that the shadowed variable is in another file, i.e. main_test.go.
  • The failure is inconsistent. Most of the time there is no failure, then sometimes there is.
@bcmills bcmills changed the title cmd/vet: shadow inconsistent failures when var in code shadows a var in a test file x/tools/go/analysis/passes/shadow/cmd/shadow: shadow inconsistent failures when var in code shadows a var in a test file Sep 26, 2019
@gopherbot gopherbot added this to the Unreleased milestone Sep 26, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Sep 26, 2019

leighmcculloch added a commit to stellar/go that referenced this issue Oct 1, 2019
Change the `govet.sh` script so that it runs the shadows check the same way for Go 1.12 and Go 1.13 builds, wrapping the shadow analyzer in a tiny main function that catches and ignores the `unsafeptr` flag that gets passed through by `go vet` in Go 1.13+.

On a couple occasions we've had builds break after #1778 was merged. The failures reported have only been on Go 1.13. The specific failure seems to be inconsistent and was reported to golang/go#34557. The fact that we've seen this failure only occur on Go 1.13 is curious. It's possible that there's a change in the language that is contributing to that behavior, or it's possible that different way we run the shadow checker for our 1.13 builds is causing tests to be included in some different way.

When I implemented #1778 I had the option of making the 1.12 and 1.13 builds execute the binary the same way like in this PR, or to execute differently like how it was done in #1778. I mostly flipped a coin.

I'm not sure if doing it this other way will yield more consistent go vet runs, but I figure worth a shot before we remove it.

Known limitations & issues:
- This might not make a difference to the failures.
- This doesn't address the actual cause of the failures, or the inconsistency, and is just attempting to make our own two builds more consistent with each other.
@leighmcculloch
Copy link
Contributor Author

@leighmcculloch leighmcculloch commented Jul 7, 2020

@ianthehat @matloob @bcmills Is this intended behavior of the shadow analyzer? Is the analyzer still intended for use or no longer maintained? Thanks!

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 7, 2020

Inconsistent failures definitely don't sound like intended behavior, so my guess is that it's not intended. However, the analyzer isn't part of the go vet suite or gopls, so it's probably not very well-maintained. We've been considering adding it as an opt-in analyzer for gopls, so perhaps we can take a look at this then.

@matloob
Copy link
Contributor

@matloob matloob commented Jul 9, 2020

The shadow analyzer is marked experimental. My guess is that the problem is that running shadow on the test variant of a package produces different results than running shadow on the 'production' variant of the package. Adding the test files produces a shadowing.

In any case I don't think we can enable shadow as is: it's hard to detect when a shadowing is intended and we certainly don't want to trigger for all shadowings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants