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

go/importer: import/export does not preserve package versions #61444

Open
findleyr opened this issue Jul 19, 2023 · 12 comments
Open

go/importer: import/export does not preserve package versions #61444

findleyr opened this issue Jul 19, 2023 · 12 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Jul 19, 2023

#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

@findleyr findleyr added this to the Go1.21 milestone Jul 19, 2023
@findleyr
Copy link
Contributor Author

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.

@findleyr
Copy link
Contributor Author

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.

@adonovan
Copy link
Member

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.

@rsc
Copy link
Contributor

rsc commented Jul 19, 2023

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.

@bcmills
Copy link
Contributor

bcmills commented Jul 19, 2023

The documentation does already say:

If the minimum version is unknown, GoVersion returns the empty string.

@findleyr
Copy link
Contributor Author

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.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/511096 mentions this issue: go/types, types2: update documentation for GoVersion

@bcmills bcmills added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Jul 19, 2023
@heschi
Copy link
Contributor

heschi commented Jul 19, 2023

From the release meeting: this is a documentation-only change and can go in whenever it's ready.

@rsc
Copy link
Contributor

rsc commented Jul 19, 2023

Is there even a documentation change given what Bryan pointed out?

@findleyr
Copy link
Contributor Author

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.

@findleyr findleyr modified the milestones: Go1.21, Go1.22 Jul 19, 2023
@findleyr
Copy link
Contributor Author

We should update the importer/exporter to preserve GoVersion, for 1.22. Moved the milestone.

@mdempsky
Copy link
Member

With the addition of go/ast.File.GoVersion, will we also eventually need to preserve a go/types.Object.GoVersion?

@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants