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

Jump to bottom

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
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

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
@gopherbot gopherbot added this to the Unreleased milestone Feb 5, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 5, 2020
@dmitshur dmitshur mentioned this issue Feb 5, 2020
Closed
@bcmills
Copy link
Contributor

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
Contributor

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
Contributor

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 as completed Feb 6, 2020
@bcmills bcmills added the Testing An issue that has been verified to require only test changes, not just a test failure. label Feb 6, 2020
@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 7, 2020 •
edited

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).

@golang golang locked and limited conversation to collaborators Feb 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot