x/tools/go/loader: improve and stabilize API

[alandonovan]

We should simplify and stabilize the go/loader package's API. Before that happens, there are a number of features to add and bugs to fix:

Features:

1. support ... patterns just like "go build" and "go list" do. (Ideally the logic would be shared. Russ suggested fork/execing "go list", but I don't like this approach: it uses the actual file system instead of the abstraction defined by build.Context.)

2. Allow packages specified on the command line to use cgo. Currently only imported packages are subject to cgo preprocessing.

3. Support pkg-config.

API simplifications:

4. The public fields of loader.Program are sometimes annoying to use. Study existing clients and come up with something more convenient.

5. Update the go/types tutorial at https://github.com/golang/example/tree/master/gotypes.

Once this is done, we should use go/loader in tools such as golint, govet and stringer.

cc: @griesemer

[josharian]

Alan, any chance of revisiting this early in 1.9? I'd be happy to be a sounding board.

I just found that even golang.org/x/tools/cmd/gotype doesn't use go/loader, and it pains me to invoke slow, heavy 'go install' just to get some code typechecked.

[alandonovan]

Hi Josh, thanks for the offer. I would like to finish off the loader but none of these changes are on my plate for Google project work in the near future so I can't promise any schedule.

Do you have a sense of how much churn loader clients outside x/tools can tolerate as we finalize the API? Since Google vendors everything we're insulated from it, but I got a panel question at GopherCon that was effectively "when will you stop breaking things?". Perhaps we need a mailing list of interested parties.

[josharian]

I think that churn during a dev cycle is expected. Once it gets frozen, then the Go 1 compatibility promise kicks in.

This issue could be the mailing list, for a first pass. Off the top of my head, the non-Google people that come to mind who work a lot with the go/* packages are: @dominikh @fatih @valyala

[myitcv]

Coming back to this in light of the the 1.10 build cache (and vgo), I still think there is a use case for loading from source, more specifically partially loading from source.

The use case is code that does not (yet) fully compile. In particular I have code generators in mind, but the same could apply to tools like @mdempsky's go/types-based gocode etc where partial results are desirable.

My thinking is that x/tools/go/loader could be changed such that either:

• x/tools/go/loader uses go/importer if a package is present in the build cache (determined by the changes that will land as part of the https://go-review.googlesource.com/c/go/+/107776 sequence), i.e. is current and compiles, and falls back to using source code loading if not
• x/tools/go/loader remains as is but uses the supplied Config.TypeChecker.Importer if it implements types.ImporterFrom (with that implementation doing the build cache check), falling back to the current behaviour if nil, nil is returned (or some such defined behaviour)

Thoughts?

[dominikh]

Loading from export data and loading from source give drastically different results. Export data doesn't provide enough information about function bodies, unexported objects, associations with the AST, and so on. People who use go/loader usually depend on it being a source importer, and changing that would break a lot of existing tools such as errcheck, staticcheck and many more. That renders your first option untenable.

[alandonovan]

We are (once again) actively working on this and should have something to share in a week or two. In brief, we are thinking along the following lines: The go/build.Package structure encodes too much of the 'go build' way of organizing projects, so we need a data type that describes a package of Go source code independent of the underlying build system. It should work equally well with go build and vgo, and also other build systems such as bazel & blaze, making it easy to construct analysis tools that work in all these environments. (Currently, for example, tools such as errcheck and staticcheck are essentially unavailable to the Go community at Google, and some of Google's internal tools for Go are unavailable externally.) We will provide a uniform way to obtain package metadata by querying each of these build systems, optionally supporting their preferred command-line notations for packages, so that tools integrate neatly with users' build environments. The metadata query function will shell out to an external query tool appropriate to the current workspace. The go/loader package currently assumes "source all the way down": it recursively parses, loads and type-checks all dependencies of the initial packages. Much more efficient is to load source code only for the requested packages, and to use export data (compiler-generated .a files) for their dependencies. This may require a build to materialize the .a files, but now that builds are incremental this cost is amortized. Building .a files avoids the historical problem of using them: that they often don't exist, as few people use "go install", and when they do exist they are often stale. The new x/tools/go/loader API, which we plan to call x/tools/go/packages, and eventually contribute to the standard library, will provide a single Package data type that describes a package's metadata (analogous to go/build.Package), and optionally its syntax trees and type information, if type-checking and/or parsing were requested for that package.

…

On 24 April 2018 at 11:36, Dominik Honnef ***@***.***> wrote: Loading from export data and loading from source give drastically different results. Export data doesn't provide enough information about function bodies, unexported objects, associations with the AST, and so on. People who use go/loader <https://goto.google.com/loader> usually depend on it being a source importer, and changing that would break a lot of existing tools such as errcheck, staticcheck and many more. That renders your first option untenable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AICt9DTlLxwjxWMDXQlDsTXyVS5FPRhTks5tr0ZsgaJpZM4HNty4> .

[myitcv]

Thanks @alandonovan, great to hear this is being worked on.

Much more efficient is to load source code only for the
requested packages, and to use export data (compiler-generated .a files)
for their dependencies.

This was exactly the thinking behind my second option. Sounds good.

The new x/tools/go/loader API, which we plan to call x/tools/go/packages

Is the plan still to have @rsc's go list -deps work-chain and preliminary module support land in Go 1.11 and have your changes build on top of that?

The metadata query function will shell out to an external
query tool appropriate to the current workspace.

This might not be the appropriate place, but I'm going to ask the question in any case and we can move discussion elsewhere as required.

I use godef extensively, as do many others. With the arrival of vgo I think editors are going to have to develop some concept of context for source code navigation. At the moment they can use godef context-free. With vgo this ceases to be the case; import resolution happens in the context of a go.mod file. I guess this point may also be true for other build systems or go.mod equivalents.

I make this point because I assume external tooling is going to have to supply this context to x/tools/go/packages and the like in order for resolution to properly function.

I might have totally missed something obvious here...

In the (unlikely) event I haven't, none of this is, I suspect, news to you, but I'm guessing/hoping what you share in a couple of weeks will cover this point?

Thanks

[dominikh]

@alandonovan I hope you can consider the following points for the design of the go/packages API:

For staticcheck I've been working on my own go/loader and my own export data format.

Much like your plans, my loader uses export data if available and up to date, and source code otherwise. My export data, however, is different from that of any existing Go toolchain in that it stores a full representation of the AST and all type information, so that the end result is identical to parsing from source. I would hope that your new API would allow me to use my own export data instead of being forced to use that produced by the build system (go, vgo, bazel, …)

I've been working on my entirely own loader because the go/loader API lacks certain associations that I need in my tools. Specifically, I need to map between all the different domains of go/token, go/ast, go/build, go/types and go/ssa. I need to be able to map from go/types.Package to loader.Package, from loader.Package to build.Package, from go/ast.File to loader.Package and even from token.File to ast.File.

go/loader doesn't maintain some of these mappings (for example it discards go/build.Packages), and relies on expensive operations like PathEnclosingInterval for others.

[josharian]

Excited to hear this is being worked on. I look forward to reviewing the API, for little things like being able to (optionally?) discover the size of a file prior to reading it.