x/tools/go/loader: importers should just work

[josharian]

Apologies in advance that this is vague.

Symptoms of the problem:

• @randall77's frustrations with stringer in x/tools/cmd/stringer: can't find packages #10249.
• While working on CL 7360, I had to use cmd/internal/gc but ./cmd/?g.
• The author of a tool picks an importer, but it is the user of a tool that needs to do the dependency resolution. What happens when the author picks gcimporter but the user uses gccgo?
• I personally use go/build instead of go/loader in my little one-off tools because go/loader doesn't ever seem to do what I mean but go/build always does. (And I haven't invested the time to dig into why.)

I'm not familiar enough with go/loader to make a concrete detailed suggestion, but perhaps go/loader could do something like: automatically start with gcimporter (if the object files aren't stale), then fall back to a source importer, and then fall back to other importers (gccgo, llgo, etc.)? And provide hooks so that advanced users can insert their own importers (as I presume Google does internally).

/cc @randall77 @alandonovan

[griesemer]

FWIW: The godex command tries various importers. But I don't think it's a "good" solution.

[josharian]

What is wrong with the godex approach? (Genuine question.)

[griesemer]

The "trial-and-error" approach seems not very disciplined. A standardized export format would be nice but may be more than compiler writers are willing to sign up for. The importer/exporter written for go/types could be plugged in. We have a prototype hooked up to gccgo (work currently paused due to manpower constraints).

[minux]

for the x/tools packages, we just need to support gc, gccgo and source.
gc and gccgo package file formats are different, so the we don't need
to guess which kind of importer to use. (I hope that if go/loader always
use gc, gccgo and then source importer, authors of other toolchains will
try to stick to one of the available export file formats.)

I think many of the frustration for tools like stringer comes from the fact
that they can't import from source where source is available (whenever
I see a report against x/tools/cmd/* that package can't be imported, I will
reply the same: have you tried "go install" first? And 100% of time, that
solved the problem.)

We should do something here.

[alandonovan]

Hi Josh. I agree that go/loader is far from satisfactory. There are a number of separate issues here.

@randall77's frustrations with stringer in #10249.

The stringer tool has its own logic for loading a type-checked package from an import path. For consistency with other tools it should probably use go/loader. I don't know why it doesn't; most likely its author didn't know about go/loader.

A second question is whether stringer should load the program from source or from export data. I think source is the right answer, since it's the ground truth; the extra cost is small and infrequently paid. Loading export data assumes that the user did a 'go install' recently (unlikely) and that the tool author guessed correct the right compiler.

The author of a tool picks an importer, but it is the user of a tool that needs to do the dependency resolution. What happens when the author picks gcimporter but the user uses gccgo?

I agree that each tool should not make the choice of which kind of import data to use; it should be deduced from the workspace state, or offered to the user as a standard flag. Using source where possible finesses the problem.

While working on CL 7360, I had to use cmd/internal/gc but ./cmd/?g.

I'm not sure what you meant by this. Could you clarify?

perhaps go/loader could do something like: automatically start with gcimporter (if the object files aren't stale), then fall back to a source importer, and then fall back to other importers (gccgo, llgo, etc.)?

This would mean duplicating yet more of the go/build logic (computeStale et al) in go/loader, but perhaps it would be worthwhile if it would simplify the configuration API. Ideally you would simply tell the loader which packages must be loaded from source and it would take care of the rest.

To date, most of go/loader's clients want source position information for dependencies, which is not recorded in the export data, so there's been little demand for the export data use case.

And provide hooks so that advanced users can insert their own importers (as I presume Google does internally).

We already have such a hook, but users shouldn't have to think that hard to get sensible behavior. There are too many moving parts involved in correctly loading a program.

[josharian]

I meant that eg was able to correctly interpret cmd/internal/gc, but not (e.g.) cmd/5g. It could resolve ./cmd/5g, but not ./cmd/internal/gc. However, the circumstances are strange here, since we're working from within GOROOT.

Thanks, I'll look into it.

As a consumer of these packages, I can say that the go/loader API would benefit from simplification and/or more examples.

I agree. The API has a lot of moving parts and not enough documentation, and the implementation is more complex than it looks.

I'm tempted to just remove support for loading binary export data entirely, since supporting it well will require the addition of a lot more complexity: the addition of staleness-checking logic as described above, and the fixing of a subtle bug about loading a mixture of source and binary.

[I seem to have misunderstood the Github UI here and edited Josh's comment while trying to respond to it. The quoted text is Josh, the responses are me. --adonovan]

[alandonovan]

Josh: I can't reproduce the problems you mentioned w.r.t. 'eg' and the relative cmd directories. Please let me know if you see this again.

go/loader no longer supports loading from binary, which simplifies the API slightly and the internals rather more.

I've moved the API documentation into doc.go, and added a number of Example tests showing basic usage of the API.

Please let me know if you find further problems with this package, or have ideas for improving it.