proposal: x/tools/go/gcexportdata: Read to support only 2 Go releases + tip

[adonovan]

The gcexportdata package defines functions to export and import types.Packages as binary messages. They serve two purposes:

1. gcexportdata.{Write,Read} permit applications to serialize and deserialize type information. For example, gopls uses it for its index (database) of types.
2. gcexportdata.Read permits applications to read export data written by the Go compiler. For example, x/tools/go/packages uses this when NeedExport is set.

For (1), all that matters is that the Write and Read functions (at the exact same version) can faithfully round-trip all aspects of a types.Package; the form of the message isn't important. Currently, it uses the "indexed" (i) form, which the compiler used prior to its support for inlining and generics. This form is obsolete within GOROOT and is mentioned only in a few stale references, including documentation at cmd/compile/internal/typecheck/iexport.go. We plan to purge GOROOT of all mention to the i format, and move the docs to x/tools/internal.

For (2), what matters is that Read can read export data written by supported versions of the compiler. The compiler currently uses the "unified" (u) form. The gcexportdata package currently promises to "support[s] go1.7 export data format and all later versions". Given that almost all other promises of toolchain compatibility are limited to the last two Go releases (plus tip GOROOT), we propose to reduce Read's support window to match. (As a matter of fact, the code actually supports only as far back as go1.11.)

@timothy-king @griesemer @findleyr

[gabyhelp]

Related Issues and Documentation

• x/tools/go/gcexportdata: improved API to replace gcimporter15 #15651 (closed)
• x/tools/go/gcexportdata: support export using indexed format #28260 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[gopherbot]

Change https://go.dev/cl/607495 mentions this issue: internal/gcimporter: move indexed format docs

[rsc]

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

[adonovan]

After discussion with @griesemer, we agree that the support policy proposed here seems appropriate.

Furthermore, we agree that purpose (1), a serialization format for types.Packages, should really move into the go/types package where it will be easier to maintain, since it won't need all the cleverness each time the toolchain upgrades (witness: aliases). We'll file a separate proposal for that:

• https://github.com/golang/go/issues/69491

[aclements]

Is there ever a reason a tool using gcexportdata would need to read old export data? I can't think of any, but want to put the question out there.

[timothy-king]

Is there ever a reason a tool using gcexportdata would need to read old export data? I can't think of any, but want to put the question out there.

You may want to ship a single tool that is compatible with analyzing the output of a large range of go toolchains. Today this would be something like creating a single binary that supports working with builds of both the 1.18 and 1.24 (tip) toolchains.

[adonovan]

You may want to ship a single tool that is compatible with analyzing the output of a large range of go toolchains.

This is more a description of the hypothetical tool than a motivation for it. In any case, you could still build such a tool by forking the old importer.

[aarzilli]

If for some reason you are working with multiple versions of go you wouldn't want to have, for example, multiple copies of staticcheck, each compiled with a different version of go.

[aclements]

Thanks for playing devil's advocate everyone. :) I think we can move this forward.

[aclements]

Have all remaining concerns about this proposal been addressed?

The proposal is for x/tools/go/gcexportdata.Read to only support the export data produced by the Go compiler from the last 2 releases plus tip, bringing it in line with our general Go version support policy.

[timothy-king]

Have all remaining concerns about this proposal been addressed?

I have one remaining somewhat theoretical concern.

There are three relevant copies of the indexed format export data reader.

1. $GOROOT/src/go/internal/gcimporter/iimport.go
2. $GOROOT/src/cmd/compile/internal/importer/iimport.go
3. x/tools/internal/gcimporter/iimport.go

What this proposal means today, is that if it is accepted x/tools/go/gcexportdata.Read does not need to provide a path to x/tools/internal/gcimporter/iimport.go. However, the two copies I would be the happiest to stop supporting are the two in GOROOT. In particular, I would be the happiest to drop support for $GOROOT/src/go/internal/gcimporter/iimport.go. So I want to make sure we can remove $GOROOT/src/go/internal/gcimporter/iimport.go before we concretely try to limit usage of x/tools/internal/gcimporter/iimport.go. It may be easier to drop this copy, if we keep the x/tools copy so it can be used in combination with go/types/Config.Importer? Not sure. I have not deeply looked into this yet.

So this might just be a theoretical concern, but it is what I am worried about.

[adonovan]

I think we could probably delete $GOROOT/src/go/internal/gcimporter/iimport.go today. It is only used as the implementation of go/importer.ForCompiler("gc"), aka importer.Default(), when it encounters an 'indexed' file, but since it only reads files emitted by cmd/compile, which have not used indexed in years, this control path will never be taken.

[gopherbot]

Change https://go.dev/cl/620135 mentions this issue: go/internal/gcimporter: drop indexed import

[adonovan]

@timothy-king I've sent a CL for the deletion. In any case I don't think the concern has any bearing on this proposal.

[gopherbot]

Change https://go.dev/cl/620036 mentions this issue: internal/gcimporter: remove test of unsupported "goroot" iimport

[adonovan]

Have all remaining concerns about this proposal been addressed?

The iimport implementation in GOROOT that @timothy-king mentioned has now been deleted.

@aarzilli If for some reason you are working with multiple versions of go you wouldn't want to have, for example, multiple copies of staticcheck, each compiled with a different version of go.

That's true, but this would only be a problem if "multiple versions" means "more than two Go releases plus tip". I don't know of any tools that fit this description, and given that the 2+tip support window would be--after this proposal--essentially uniform across all the Go project's modules, it doesn't seem like grounds for an exception here.

[aclements]

Based on the discussion above, this proposal seems like a likely accept.

The proposal is for x/tools/go/gcexportdata.Read to only support the export data produced by the Go compiler from the last 2 releases plus tip, bringing it in line with our general Go version support policy.

[aclements]

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal.

The proposal is for x/tools/go/gcexportdata.Read to only support the export data produced by the Go compiler from the last 2 releases plus tip, bringing it in line with our general Go version support policy.

[gopherbot]

Change https://go.dev/cl/623638 mentions this issue: go/gcexportdata: document 2 releases + tip policy

[gopherbot]

Change https://go.dev/cl/633115 mentions this issue: internal/gcimporter: require archive files

[gopherbot]

Change https://go.dev/cl/633708 mentions this issue: internal/gcimporter: require archive files