proposal: go/types: allow (*Package).Imports to be precise for imported packages #54096
Comments
|
Change https://go.dev/cl/419596 mentions this issue: |
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>
|
Didn't changing this break a test inside Google until we rolled it back? We could add a (*Package).DirectImports of course. |
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. |
|
If I'm understanding correctly, the current behavior is somewhat ill-defined but could still reasonably be relied upon, because the result of Furthermore, even the new definition is not as useful as it could be, because I don't feel strongly though. This does fall into a gray area, and it may be better to endure a small amount of breakage to avoid API cruft. Aside: if we don't already, we should expose an API in x/tools that provides a more accurate set of packages reachable from export data. Users should not rely on the current behavior. |
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 golang#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>
|
Note that even if we add a DirectImports method, legacy importers still won't be able to implement it, because the export data formats themselves lack the necessary information. |
Understood. But that may be an easier limitation to document. |
|
Currently, the result of I'd be helpful to hear from clients of As is, I am mildly in favor of adjusting the documentation as suggested. |
|
Change https://go.dev/cl/467896 mentions this issue: |
This CL is a port of go.dev/cl/465936 from the x/tools importer, which changes the unified importer to (1) only call Package.SetImports on the main package being imported (not any transitively imported packages), and (2) to only populate it with any packages that were referenced by the exported API. With these changes, it should behave identically to how the indexed importer worked in Go 1.19. It will also allow eventually dropping the serialized import DAG from the export data format, which should help with export data file sizes somewhat. Updates #54096. Updates #58296. Change-Id: I70d252a19cada3333ed59b16d1df2abc5a4cff73 Reviewed-on: https://go-review.googlesource.com/c/go/+/467896 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This CL is a port of go.dev/cl/465936 from the x/tools importer, which changes the unified importer to (1) only call Package.SetImports on the main package being imported (not any transitively imported packages), and (2) to only populate it with any packages that were referenced by the exported API. With these changes, it should behave identically to how the indexed importer worked in Go 1.19. It will also allow eventually dropping the serialized import DAG from the export data format, which should help with export data file sizes somewhat. Updates golang#54096. Updates golang#58296. Change-Id: I70d252a19cada3333ed59b16d1df2abc5a4cff73 Reviewed-on: https://go-review.googlesource.com/c/go/+/467896 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
The documentation for
go/types.(*Package).Importssays: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.
The text was updated successfully, but these errors were encountered: