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/build: investigate why vet failures on tip weren't caught by builders #37053

Closed
dmitshur opened this issue Feb 5, 2020 · 4 comments
Closed

x/build: investigate why vet failures on tip weren't caught by builders #37053

dmitshur opened this issue Feb 5, 2020 · 4 comments
Labels
Milestone

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 5, 2020

go vet std is reporting problems and exiting with a non-zero code on latest master (see #37030):

src $ go version
go version devel +a864cc7560 Wed Feb 5 14:32:50 2020 +0000 darwin/amd64
src $ go vet std
# sync/atomic
sync/atomic/value.go:59:44: possible misuse of unsafe.Pointer
# runtime/internal/atomic_test
runtime/internal/atomic/atomic_test.go:94:7: possible misuse of unsafe.Pointer
# strings
strings/builder.go:30:9: possible misuse of unsafe.Pointer
# runtime
runtime/alg.go:56:22: possible misuse of unsafe.Pointer
runtime/atomic_pointer.go:63:9: possible misuse of unsafe.Pointer
runtime/cgocall.go:200:13: possible misuse of unsafe.Pointer
runtime/cgocall.go:282:16: possible misuse of unsafe.Pointer
runtime/cgocall.go:287:16: possible misuse of unsafe.Pointer
[...]
src $ echo $?
2

Need to investigate why the builders didn't catch this. We used to have a misc-vet-vetall builder but that has changed in #31916. /cc @rsc @cagedmantis @toothrot

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 5, 2020

CL 176439 explicitly disabled the unsafeptr check for the standard library, while enabling all others.

(See cmd/go/internal/work/exec.go, line 1006.)

An explicit go vet invocation, on the other hand, runs all of the default vet checks.

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 5, 2020

This turned out to be an undetected regression in the behavior of go vet itself.

When we turned down misc-vet-vetall, we made the implicit vet checks for standard-library packages during go test more aggressive, so that they run all vet checks except for unsafeptr instead of the only the high-confidence checks.

And that's why it wasn't caught: we were testing a (slightly) different property from the one we actually wanted to hold. The testing verified that “all vet checks except for unsafeptr pass for packages in the standard library”, but the property we want is “go vet reports no failures for packages in the standard library”. That property only holds if go vet runs exactly “all vet checks except for unsafeptr”.

We didn't have a regression test for that last part, so when it regressed we also didn't detect the regression in the desired property that it implies.

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 6, 2020

Closing this issue, since I think we now understand why the failures weren't caught. (@dmitshur, feel free to reopen if you think we should take any further action to prevent this sort of regression.)

@bcmills bcmills closed this Feb 6, 2020
@bcmills bcmills added the Testing label Feb 6, 2020
@dmitshur
Copy link
Member Author

@dmitshur dmitshur commented Feb 7, 2020

Thank you for the analysis @bcmills! /cc @cagedmantis @toothrot

I'm glad we caught and fixed this problem in go vet (thanks to @josharian for the original report in #37030).

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
3 participants
You can’t perform that action at this time.