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

x/tools/go/packages: permit separate LoadMode for deps #31699

Closed
zombiezen opened this issue Apr 26, 2019 · 3 comments

Comments

@zombiezen
Copy link
Contributor

commented Apr 26, 2019

I really like the new packages.LoadMode bits that were introduced to fix #29429, thanks! I'm using go/packages for a utility to gather example functions in the Go CDK. Part of what this tool does is to determine which imports an example uses, which I'm doing by walking the AST of the example function body and using type information to see whether an identifier is a package name. I've been using a LoadMode of NeedName | NeedFiles | NeedTypes | NeedSyntax | NeedTypesInfo.

However, I discovered this check doesn't work if the package is not in the stdlib or in the set of directly matched packages. In an upcoming change, I will add NeedImports | NeedDeps, but this results in a 10x slowdown. I assume this is because it is syntax + type-checking the dependencies, which is not what I need. I really only need NeedName for dependencies, but I can't see a way of specifying that. Similar to #28740, I would like to specify different behavior for dependencies versus directly referenced packages.

/cc @matloob @ianthehat

@gopherbot gopherbot added this to the Unreleased milestone Apr 26, 2019

@matloob

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

We're considering an option for this (below) let us know if it would work for you.

We want to make it possible to supply ids to Load to specify packages. Then you can gather the ids for all dependency packages and query their names in a second query.

@zombiezen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I don't think it would work: the TypesInfo for the package I'm walking does not have any information for any not-loaded package names in the AST. IIUC doing a second Load wouldn't help in this case, since I wouldn't have a way to associate a particular symbol with the package it represents.

jirfag added a commit to golangci/tools that referenced this issue Sep 9, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, golang/go#31930
jirfag added a commit to golangci/tools that referenced this issue Sep 9, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, fixes golang/go#31930
@gopherbot

This comment has been minimized.

Copy link

commented Sep 9, 2019

Change https://golang.org/cl/186337 mentions this issue: go/packages: allow types loading without NeedDeps

jirfag added a commit to golangci/tools that referenced this issue Sep 10, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, fixes golang/go#31930
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.