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

proposal: go/types: allow (*Package).Imports to be precise for imported packages #54096

Open
mdempsky opened this issue Jul 27, 2022 · 4 comments
Labels
Projects
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 27, 2022

The documentation for go/types.(*Package).Imports says:

Imports returns the list of packages directly imported by pkg; the list is in source order.

If pkg was loaded from export data, Imports includes packages that provide package-level objects referenced by pkg. This may be more or less than the set of packages directly imported by pkg's source code.

This proposal is to change the second paragraph from saying "Imports includes packages" to "Imports may instead return packages".

--

Rationale: The go/types type checker's semantics of reporting the actual packages that were directly imported seems the clearly preferable behavior for end users. The alternative behavior when a package is read from export data and Imports instead returns an unspecified subset of transitive dependencies seems to always be less useful.

The reason for the current behavior is that cmd/compile's existing export data formats (textual, binary, and then indexed) were designed for the compiler's own needs, which do not require serializing a precise representation of the import graph. So instead the go/types importer approximates this by setting the Imports list to all packages seen in the export data.

For unified IR, the new export data format takes a go/types-first approach, and so now it precisely serializes the import graph. However, this technically violates the current semantics codified in the documentation. It's my position that the documentation was wrong to mandate that behavior, rather than document it as a known limitation.

If users write code that correctly walks the transitive dependency graph themselves, then their code will work both with packages created by go/types (e.g., using the srcimporter implementation) or imported from compiler export data (modulo cases where packages are omitted due to not providing symbols relevant to export data).

Finally, I'll note that the current set of packages included in Imports is unstable across compiler changes, because it includes not only packages referenced by the package's exported API (visible through go/types), but also packages that are needed by inline bodies (which are opaque to go/types importers). The meaning of "package-level objects referenced by pkg" seems too imprecise for users to rely on it meaning any particular set of packages.

--

Aside: It's also the case that the existing gcimporter implementations only call SetImports on the package being directly imported, not any transitively imported packages. Also, gccgoimporter never calls SetImports. I'm not sure whether to attempt codifying/fixing these behaviors. E.g., "If pkg is incomplete, Imports may instead return nil." would seem reasonable to allow gcimporter's existing behavior, and then gccgoimporter could be updated to implement the same imported-packages list approximation as gcimporter.

@mdempsky mdempsky added this to the Proposal milestone Jul 27, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 27, 2022

Change https://go.dev/cl/419596 mentions this issue: [dev.unified] go/internal/gcimporter: flatten imports

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 28, 2022

CC @findleyr @adonovan

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 28, 2022
gopherbot pushed a commit that referenced this issue Jul 28, 2022
The current documentation for go/types.(*Packages).Imports requires
that the import graph be flattened when read from export data. I think
this is a documentation bug (incorrectly codifying the existing
behavior, rather than documenting it as a known bug), but until that's
decided, we can at least flatten imports ourselves.

Updates #54096.

Change-Id: Idc054a2efc908b3e6651e6567d0ea0e89bb0c54d
Reviewed-on: https://go-review.googlesource.com/c/go/+/419596
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@rsc
Copy link
Contributor

@rsc rsc commented Aug 3, 2022

Didn't changing this break a test inside Google until we rolled it back?
I'm reluctant to break external users too.

We could add a (*Package).DirectImports of course.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 3, 2022

Didn't changing this break a test inside Google until we rolled it back?

It caused a Google-internal documentation generator tool to start failing, yes. The team approved a change to make the tool walk the transitive dependency graph itself.

We haven't investigated yet if there are other tools depending upon the same behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Development

No branches or pull requests

4 participants