-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
cmd/vet: report use of too-new standard library symbols #46136
Comments
@jayconrod thanks very much for writing this up, and apologies for being slow at reading it through. I'm a little confused regarding the following conclusion:
AFAIU, one of the roles of the
Gives (using tip):
That is, my module needs to set This also means that modules are not forced to bump their Another conclusion were the My understanding regarding this proposal is that it only sought to introduce a new vet rule, a vet rule that would help answer the question "do I need to bump the Unless I missed the point, and instead this proposal is looking to change the advice regarding the My feeling is that the Go version policy for a module is something sufficiently nuanced as to require specific CI testing of those versions, as well as human commentary, and not something we should try and communicate via the But as I said, I'm late at replying to this and so might well be missing context 😄 In which case I would very much appreciate your thoughts on where my thinking has gone off the rails! Thanks Critical edit: |
Thinking about this behaviour now in the context of this proposal, I think that this behaviour should be changed. In particular, I think that if the For example, I think this example should run OK:
Currently it produces the error:
Then, as has always been the case, you can support older versions while using new features in Go versions that provide them and, as this proposal suggests, the I'd also suggest that non-stdlib changes such as embed or generics should be checked by the compiler rather than |
@myitcv @rogpeppe You both bring up some excellent questions. I'll take some more time to think and read through the One thought that occurs to me: in Android, there's a separate concept of minimum and target SDK versions. The minimum version is the lowest version of Android an app works with. The target version is the version an app was compiled against. An app may use APIs from the target version that aren't in the minimum version, but it must do so conditionally. Because of release tags in build constraints, we have a similar split here, but we haven't acknowledged or defined it. The |
In further discussions offline with @rogpeppe and @mvdan we concluded the phrase "the minimum version of Go supported" is ambiguous at best, because the word "support" has connotations as far as users of a module are concerned. So I think we need to be careful with the wording, because "minimum" is not meant to signal anything about good support to end users. I think we therefore also concluded that it should be documented as best practice that module authors declare a policy for Go version support separately from the |
Lately I've tended to think of the |
I wonder if we want some machine-readable way to distinguish the minimum and target Go versions though? It would be useful for this proposal. It could be displayed by pkgsite and any other generated documentation. It would also reduce the need for the |
As a real-world example, in https://stackoverflow.com/q/68752103 a user was confused about build errors stemming from references to |
This proposal was targeting Go 1.18, but it is still in Incoming column, and there's no assignee, so it doesn't seem that it can make it to 1.18 given the release timing. I'll move this to Proposal milestone (according to https://github.com/golang/proposal#accepted, proposals are moved to a release milestone after being accepted). Please feel free to undo if needed. |
I'll voice my support for this proposal and share an issue that seems to be covered by the proposal: |
Thanks @bcmills for pointing me here from #50689. Gopls will need the analyzer described here in any case, as we want to better support using gopls built with a recent Go to develop for older versions of Go. In #50689, I mention that we'd probably need explicit configuration for gopls, precisely because we want to be consistent with the If this proposal were accepted, then gopls could simply read the Of course, if we implement this for gopls (initially in |
In the absence of this proposal, is there a way to check if "this module can be built by Go 1.X" short of setting up a CI environment that uses a Go 1.X toolchain? |
I think this proposal is mostly overtaken by the forwards compatibility proposal #57001? That proposal is accepted and mostly implemented. |
If #57001 was supposed to cover what this proposal addresses, it does not. On multiple occasions, I've pushed code that compiled, tested, and ran correctly for me but used stdlib APIs which were newer than the go.mod version and so failed to build for a teammate who was on that version exactly. I've talked to several other people who've had similar experiences. I imagined a vet check against api/*.txt as an effective solution and found this proposal. The advantage we have now is that #57001 formalized the machinery for module- and constraint-determined language versions as mentioned in the proposal. |
Hi @zephyrtronium, during the proposal discussion for #57001, my personal expectation was that it would handle the standard library as well, but it turns out my expectation was incorrect. 😅 As far as I understand it, this issue here is still considered a valid candidate solution. |
Change https://go.dev/cl/567837 mentions this issue: |
This change is the counterpart to CL 569435 for completions in imported packages. Updates golang/go#46136 Change-Id: I57011897c395d37a89a8e3a99e8c3511de017ad3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/569796 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com>
This change enables RunDespiteErrors, after auditing the code. This should give more timely feedback while editing. Also, it moves the vet/gopls common code (DisallowedSymbols) to typesinternal.TooNewStdSymbols, out of the gopls module, in anticipation of adding this analyzer to vet. Updates golang/go#46136 Change-Id: I8d742bf543c9146376d43ae94f7adae3b453e471 Reviewed-on: https://go-review.googlesource.com/c/tools/+/570138 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Alan Donovan <adonovan@google.com> Reviewed-by: Robert Findley <rfindley@google.com>
This change causes the stdversion checker not to report any diagnostics in modules with go versons before go1.21, because the semantics of the go declaration were not clearly defined to correspond to toolchain requirements at that point. Also, those Go versions are now unsupported, so reporting diagnostics just creates unnecessary churn. Updates golang/go#46136 Change-Id: I323f704c4d4f1f0fe5fc8b5680824bc07d3c4112 Reviewed-on: https://go-review.googlesource.com/c/tools/+/570140 Reviewed-by: Robert Findley <rfindley@google.com> Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This is what I implemented. However, a counterpoint: this behavior nullifies any value this checker has for projects that strive to maintain compatibility with multiple older versions of Go. For example, Perhaps the analyzer should be modified so that, when run within gopls, it does not disable the check on pre-go1.21 modules. The behavior within vet would be unchanged. |
@adonovan presumably for x/tools we're going to increase the go.mod go directive following #65917, so I don't expect this to be a problem for long. It does not seem particularly worthwhile to me to take extra action to accommodate Go 1.19 and Go 1.20. Eventually, most projects will use a Go directive version that is 1.21 or later, at which point the current check will be sufficient. |
No change in consensus, so accepted. 🎉 The proposal is to add a new ‘stdversion’ analyzer that reports when the go line is too old for something used from the standard library. A sample error would be:
|
Change https://go.dev/cl/572035 mentions this issue: |
This CL moves the stdversion analyzer out of gopls, into the exported analyzer tree, whence it will be vendored into cmd/vet in a follow-up. Updates golang/go#46136 Change-Id: I039ef78ecdcfd6bc64d5b089017a9b8635cf9aa5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/572035 Auto-Submit: Alan Donovan <adonovan@google.com> Reviewed-by: Tim King <taking@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change https://go.dev/cl/572777 mentions this issue: |
Change https://go.dev/cl/571276 mentions this issue: |
CL 567837 extracted the stdlib manifest from x/tools/internal/imports to x/tools/internal/stdlib. That is the new package that will need to be regenerated every 6 months after each major Go release. Update the relui task that automated that recurring work accordingly. For golang/go#46136. For golang/go#38706. Change-Id: I9705ac881a18a5e7b03188228eadfaccf816aaca Reviewed-on: https://go-review.googlesource.com/c/build/+/572777 Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
CL 567837 deleted mkstdlib.go (a generator) and zstdlib.go (its output). For golang/go#46136. For golang/go#38706. Change-Id: If5623e3ab014c2fee63b1d058c726ea5acb06aa2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/571276 Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Change https://go.dev/cl/582556 mentions this issue: |
Change https://go.dev/cl/582995 mentions this issue: |
Thank you very much for this analysis! Already finding it useful as part of gopls :) |
I am not sure the logic that unsupported versions are irrelevant should be applied to checks that are related to version support. For example, x/crypto (mistakenly) has an old go.mod version of 1.18, but uses a Go 1.20 symbol now. This caused confusion (#68147, #68035, and #68011) because it didn't print the "note: module requires Go 1.20" hint. This vet check would have caught my mistake if it wasn't limited to the previous two versions. In practice, for better or worse, a lot of our users run old Go versions (for example Debian stable has Go 1.19). We don't have to support it, but we should try to set things up so they get helpful and correct error messages. |
Author(s): Jay Conrod based on discussion with Daniel Martí, Paul Jolly, Roger
Peppe, Bryan Mills, and others.
Last updated: 2021-05-12
Proposal: https://go.googlesource.com/proposal/+/refs/heads/master/design/46136-vet-std-references.md
Abstract
With this proposal,
go vet
(andgo test
) would report an error if a packageimports a standard library package or references an exported standard library
definition that was introduced in a higher version of Go than the version
declared in the containing module's
go.mod
file.This makes the meaning of the
go
directive clearer and more consistent. Aspart of this proposal, we'll clarify the reference documentation and make a
recommendation to module authors about how the
go
directive should be set.Specifically, the
go
directive should indicate the minimum version of Go thata module supports. Authors should set their
go
version to the minimum versionof Go they're willing to support. Clients may or may not see errors when using a
lower version of Go, for example, when importing a module package that imports a
new standard library package or uses a new language feature.
Background
The
go
directive was introduced in Go 1.12, shortly after modules wereintroduced in Go 1.11.
At the time, there were several proposed language changes that seemed like they
might be backward incompatible (collectively, "Go 2"). To avoid an incompatible
split (like Python 2 and 3), we needed a way to declare the language version
used by a set of packages so that Go 1 and Go 2 packages could be mixed together
in the same build, compiled with different syntax and semantics.
We haven't yet made incompatible changes to the language, but we have made some
small compatible changes (binary literals added in 1.13). If a developer using
Go 1.12 or older attempts to build a package with a binary literal (or any other
unknown syntax), and the module containing the package declares Go 1.13 or
higher, the compiler reports an error explaining the problem. The developer also
sees an error in their own package if their
go.mod
file declaresgo 1.12
orlower.
In addition to language changes, the
go
directive has been used to opt in tonew, potentially incompatible module behavior. In Go 1.14, the
go
version wasused to enable automatic vendoring. In 1.17, the
go
version will control lazymodule loading.
One major complication is that access to standard library packages and features
has not been consistently limited. For example, a module author might use
errors.Is
(added in 1.13) orio/fs
(added in 1.16) while believing theirmodule is compatible with a lower version of Go. The author shouldn't be
expected to know this history, but they can't easily determine the lowest
version of Go their module is compatible with.
This complication has made the meaning of the
go
directive very murky.Proposal
We propose adding a new
go vet
analysis to report errors in packages thatreference standard library packages and definitions that aren't available
in the version of Go declared in the containing module's
go.mod
file. Theanalysis will cover imports, references to exported top-level definitions
(functions, constants, etc.), and references to other exported symbols (fields,
methods).
The analysis should evaluate build constraints in source files (
// +build
and
//go:build
comments) as if thego
version in the containing module'sgo.mod
were the actual version of Go used to compile the package. Theanalysis should not consider imports and references in files that would only
be built for higher versions of Go.
This analysis should have no false positives, so it may be enabled by default
in
go test
.Note that both
go vet
andgo test
report findings for packages named onthe command line, but not their dependencies.
go vet all
may be used to checkpackages in the main module and everything needed to build them.
The analysis would not report findings for standard library packages.
The analysis would not be enabled in GOPATH mode.
For the purpose of this proposal, modules lacking a
go
directive (includingprojects without a
go.mod
file) are assumed to declare Go 1.16.Rationale
When writing this proposal, we also considered restrictions in the
go
commandand in the compiler.
The
go
command parses imports and applies build constraints, so it can reportan error if a package in the standard library should not be imported. However,
this may break currently non-compliant builds in a way that module authors
can't easily fix: the error may be in one of their dependencies. We could
disable errors in packages outside the main module, but we still can't easily
re-evaluate build constraints for a lower release version of Go. The
go
command doesn't type check packages, so it can't easily detect references
to new definitions in standard library packages.
The compiler does perform type checking, but it does not evaluate build
constraints. The
go
command provides the compiler with a list of files tocompile, so the compiler doesn't even know about files excluded by build
constraints.
For these reasons, a vet analysis seems like a better, consistent way to
find these problems.
Compatibility
The analysis in this proposal may introduce new errors in
go vet
andgo test
for packages that reference parts of the standard library that aren't available
in the declared
go
version. Module authors can fix these errors by increasingthe
go
version, changing usage (for example, using a polyfill), or guardingusage with build constraints.
Errors should only be reported in packages named on the command line. Developers
should not see errors in packages outside their control unless they test with
go test all
or something similar. For those tests, authors may use-vet=off
or a narrower set of analyses.
We may want to add this analysis to
go vet
without immediately enabling it bydefault in
go test
. While it should be safe to enable ingo test
(no falsepositives), we'll need to verify this is actually the case, and we'll need
to understand how common these errors will be.
Implementation
This proposal is targeted for Go 1.18. Ideally, it should be implemented
at the same time or before generics, since there will be a lot of language
and library changes around that time.
The Go distribution includes files in the
api/
directory that track whenpackages and definitions were added to the standard library. These are used to
guard against unintended changes. They're also used in pkgsite documentation.
These files are the source of truth for this proposal.
cmd/vet
will accessthese files from
GOROOT
.The analysis can determine the
go
version for each package by walking upthe file tree and reading the
go.mod
file for the containing module. If thepackage is in the module cache, the analysis will use the
.mod
file for themodule version. This file is generated by the
go
command if nogo.mod
file was present in the original tree.
Each analysis receives a set of parsed and type checked files from
cmd/vet
.If the proposed analysis detects that one or more source files (including
ignored files) contain build constraints with release tags (like
go1.18
),the analysis will parse and type check the package again, applying a corrected
set of release tags. The analysis can then look for inappropriate imports
and references.
The text was updated successfully, but these errors were encountered: