go/types: should the 'newer Go version' error be reported to Config.Error? #66525
Comments
timothy-king
added
NeedsDecision
|
Change https://go.dev/cl/574255 mentions this issue: |
|
One option that came up in code review is to report the error as a OTOH it is not clear whether it is acceptable to do a best effort typing when we know the package is a future GoVersion. A minor complicating wrinkle is when all files are pinned to versions less than the version of |
Yes, all errors should be reported through Config.Error, and the error returned by Checker.Files should merely be the first of them. We should re-establish the invariant that a non-zero number of calls to Error is equivalent to the package being ill-typed. The two early error returns in Checker.Files both violate this invariant. The FakeImportC one was rarely exercised, but the version check (downgraded from a panic by https://go.dev/cl/507975) is much more important. Both these errors should be reported (e.g. against the first package declaration if no other location is appropriate), but should not abort type-checking of the package. We should document clearly in the code that early returns are not allowed. Clients of go/types make widespread assumptions that the type-checker does the best it can with the syntax, and that well-formed declarations are annotated with types even if the package contains some type errors. For example, it is common to evaluate I can send you a CL for the go/types change if you like. |
|
Change https://go.dev/cl/574495 mentions this issue: |
adonovan
removed
the
NeedsDecision
Ensure that when types.Checker.Files returns an error some error is always reported and the package is illTyped. Adds an additional hint to recompile the tool when types returns an error "package requires newer Go version 1.22" or similar. Updates golang/go#65608 Updates golang/go#66525 Change-Id: I131ee3e668815f88d16a18c6e92f002220284a03 Reviewed-on: https://go-review.googlesource.com/c/tools/+/574255 Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Matloob <matloob@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com>
Many tools (especially in the IDE) rely on type information being computed even for packages that have some type errors. Previously, there were two early (error) exits in checkFiles that violated this invariant, one related to FakeImportC and one related to a too-new Config.GoVersion. (The FakeImportC one is rarely encountered in practice, but the GoVersion one, which was recently downgraded from a panic by CL 507975, was a source of crashes due to incomplete type information.) This change moves both of those errors out of checkFiles so that they report localized errors and don't obstruct type checking. A test exercises the errors, and that type annotations are produced. Also, we restructure and document checkFiles to make clear that it is never supposed to stop early. Updates #66525 Change-Id: I9c6210e30bbf619f32a21157f17864b09cfb5cf2 Reviewed-on: https://go-review.googlesource.com/c/go/+/574495 Reviewed-by: Tim King <taking@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The type checker produces an error if the Go version is too new. When compiled with Go 1.21, this error is silently dropped on the floor and the type checked package is empty, due to golang/go#golang#66525. Guard against this very problematic failure mode by filtering out Go versions that are too new. We should also produce a diagnostic, but that is more complicated and covered by golang#61673. Also: fix a bug where sandbox cleanup would fail due to being run with a non-local toolchain. Fixes golang#66677 Change-Id: Ia66f17c195382c9c55cf0ef883e898553ce950e3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/576678 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Since go1.21,
(*go/types.Checker).Files()has returned an error when a package has aGoVersion(specified intypes.Config.GoVersion) that is newer than thego/typeslibrary it is compiled against. Error in question is:x/tools/go/packageswas assuming the onlyerrornot reported totypes.Config.Errorwastypes.errBadCgo. Example downstream issue: #65590.This issue is to decide whether this error should be reported to the
types.Config.Errorfield, and if so how?@griesemer @adonovan
The text was updated successfully, but these errors were encountered: