-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
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.
@ianthehat @matloob @bcmills Is this intended behavior of the shadow analyzer? Is the analyzer still intended for use or no longer maintained? Thanks! |
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 |
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. |
What version of Go are you using (
go version
)?The docker image that was in use was:
circleci/golang:1.13-stretch
and with digestsha256:728d4283948f6f2666c6dbdcb56648a5db9056431265d296175449463a82c072
.You can get that specific version of the image with:
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
OutputWhat 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 runshadow ./...
.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:
See https://app.circleci.com/jobs/github/stellar/go/8686.
The version of
shadow
being used is from commit golang/tools@ea99b82c7b93.Line 95 of
main.go
definesseed
in a function:Line 19 of
main.go
is a comment:However as @bartekn points out in stellar/go#1778 (comment),
seed
is defined inmain_test.go
.Issues here:
main_test.go
.The text was updated successfully, but these errors were encountered: