61175 added go/types.Package.GoVersion
, which reports the Go version used to type-check the package. One of the motivators for this change was to make the GoVersion
available in the cgocall
analyzer.
But Package.GoVersion
is not recorded in export data, nor can it be populated by an importer, because there is no corresponding setter. ~Therefore one of the goals of this new API is not achieved.~ (EDIT: perhaps it's still OK, because the version will still be set for the package currently being analyzed).
Tentatively marking this as a release blocker (at least to make a decision), because #61175 was a release blocker.
CC @rsc @adonovan @mdempsky @griesemer
Comment From: findleyr
Unfortunately it is probably too late to do anything about this, so IMO we should probably just move this to 1.22, and add a setter API that can be used by importers/exporters.
We don't have enough time to make an export data change at this point.
Comment From: findleyr
Tentative proposal for 1.21: document that the Package.GoVersion
method may be inaccurate (unfortunately rendering it mostly useless), and then add a setter for 1.22 along with a change to the export data format. Tools can still figure out the correct package version by keeping track of their inputs.
Comment From: adonovan
Exactly: let the documentation frame it as a required non-empty property, but give a warning that the initial go1.21-era implementation is temporarily broken, with a link to the issue. Then we can fix the bug and remove the warning without making a breaking API change (optional -> required) to the field.
Comment From: rsc
The reason you need to know the package version is to discern the meaning of source code. I believe that goal has been achieved. (If not, please correct me!)
It seems fine to me to document that if the package version is unknown, which may happen when you are just looking at export data, then the GoVersion can be empty.
Comment From: bcmills
The documentation does already say:
If the minimum version is unknown, GoVersion returns the empty string.
Comment From: findleyr
Actually, yes, the cgocall analyzer will get the version type-checked from source. But it will not know the version of its dependencies. Perhaps that's OK.
Updated the top comment.
Comment From: gopherbot
Change https://go.dev/cl/511096 mentions this issue: go/types, types2: update documentation for GoVersion
Comment From: heschi
From the release meeting: this is a documentation-only change and can go in whenever it's ready.
Comment From: rsc
Is there even a documentation change given what Bryan pointed out?
Comment From: findleyr
Is there even a documentation change given what Bryan pointed out?
Not for this issue, no, though the associated CL updates the documentation slightly to leave more wiggle room in the future.
Comment From: findleyr
We should update the importer/exporter to preserve GoVersion, for 1.22. Moved the milestone.
Comment From: mdempsky
With the addition of go/ast.File.GoVersion, will we also eventually need to preserve a go/types.Object.GoVersion?