cmd/vet: disable checks based on go.mod go version

[rsc]

New vet checks can make old code's tests stop passing. An example is #57059, where the printf format checker got more precise in Go 1.18. Code that tested fine with Go 1.17 stops passing tests in Go 1.18.

Vet needs to know which Go version each package was written for anyway. Perhaps it should use that information to disable checks that are newer than that version, so that tests don't start failing due to new vet reports until you update the go.mod go line.

(Related to #56986.)

[timothy-king]

For the go command, we can forward the module information from go/packages. I am not really sure where one gets the go version of the package from bazel builds yet. sdk_version is on go_cross_binary.

It would be nice if knew we were going to use this before we commit to supporting it. I am not sure we would use this for #57059 at the moment. A range variable semantic change would require this for loopclosure (related #56010).

A somewhat related problem is rolling out precision improvements to existing checks incrementally. Perhaps using the version number would have been a good way to gate the t.Parallel improvement to loopclosure? It gives a good and consistent story of "as a part of updating to the new version, you must also fix new warning from new checks". A problem with doing this is that this particular check can still flag issues in older code. It seems a pity to not warn and get those unit tests fixed due to the go.mod file not being updated.

[rsc]

I would use it for any new analysis that we add in a particular version of Go. I think we could have used this for the Printf check when we did the expansion in Go 1.18 to look at constant values.

The analysis doesn't run when the go.mod go line says an earlier version than when it was introduced. That way if people update to a new Go toolchain and run 'go test ./...', the test failures don't happen until they update the go.mod to opt in to the new release semantics. I would have done the same for the loopclosure fix. If people want the warnings, they are welcome to update the Go version and get them. If they just want to build some old code with a new toolchain, we should make that the least disruptive we can. We want to prioritize compatibility over new test errors.

Everything is going to have to be updated to understand per-module language versions when it's time for the for loop fix. The go command and the compiler obviously already understand that. We will need to pass it to vet as well. And Bazel will need to understand it too, I assume by gazelle writing something per BUILD file or per Go rule. So the version is going to be there already.

[timothy-king]

I believe all of the work for this has been done. Okay to close?

edit: I was only focused on the part that forwards the package version information to vet. That part is done. Turning off checks in detail based on the version has not yet been done.

[ChrisHines]

I have a question about this change.

Background: I help maintain a large number of (private) Go modules that we generally upgrade the version of Go at the same time for all of them. In the past, prior to actually upgrading them I would run go vet across all the modules with the new tool chain to see how much we needed to update before bumping the Go version. We would usually fix those issues before actually upgrading Go to avoid a scramble.

Do I understand correctly that with this change, running a new version of go vet is no longer sufficient to discover what code needs updating in the above situation? We would first need to run go get go@1.N+1 in the module and then revert any changes that made if we aren't ready to upgrade.

Although that isn't terribly hard and it's something that only happens once every ~6 months, it would be nice to have a way to override the version specified in the go.mod file. Something like staticcheck has: https://staticcheck.io/docs/running-staticcheck/cli/#go.

I would also like to mention that anyone not aware of this change may be surprised by following the same procedure as before, see no warnings from go vet and falsely believe there are no issues to address before upgrading. This change should be prominently mentioned in the release notes and the go vet docs updated appropriately.

[timothy-king]

We would first need to run go get go@1.N+1 in the module and then revert any changes that made if we aren't ready to upgrade.

In some cases, yes. These are related loop variable scope at the moment.

Each file now has an associated GoVersion. There are fine grained controls with //go:build but most are expected to be derived from go.mod versions. To get the new loop variable scope changes the GoVersion >= 1.22 in the file. So some vet checks are pegged to specific GoVersions. An example is loopclosure no longer warning after an upgrade. So that is less warnings on go get go@1.N+1. In the other direction, to resolve #66387 would result in copylock giving more warnings because there is a new implicit assignment in 3-clause for loops with GoVersion >= 1.22. The new stdversions checker #46136 will start warning based on the file's GoVersion. This will issue fewer warnings when upgrading a go.mod file.

So so far most of what is there should be not be too visible to most people when upgrading the go.mod to go1.N+1. The exceptions mean you have a serious problem, and hopefully the bumpiness when updating in those cases is warranted.

I would also like to mention that anyone not aware of this change may be surprised by following the same procedure as before, see no warnings from go vet and falsely believe there are no issues to address before upgrading. This change should be prominently mentioned in the release notes and the go vet docs updated appropriately.

If we did start gating new vet checks (or improved precision) on the GoVersion, upgrading the go.mod to go1.N+1 would become more visible. Maybe that would require more documentation? Blog post?

Thank you for pointing this out.

[ChrisHines]

@timothy-king I've read your post a couple times and I am having trouble interpreting your argument. I think you are saying essentially that go vet checks rarely find new problems in old code so the practice of running go vet from tool chain vN+1 before upgrading isn't usually needed. Apologies if I've misinterpreted your comments.

I don't find that point persuasive, though. On more than one occasion go vet vN+1 has found more problems in our code than go vet vN. From that experience I have learned that it is prudent to look before we leap and work to fix the problematic code before we upgrade the tool chain. Keeping our CI builds rolling along smoothly is valuable to the developers on the project. So regardless of whether go vet vN+1 will actually find anything we don't know until we try. So my feedback here is about the extra steps to perform that check this change implies compared to the status quo.