proposal: x/tools/packages: populate Module field for package loads using Gopackagesdriver

[rastenis]

packages.Load uses either the goListDriver or an external driver (Gopackagesdriver) to retrieve information about packages. However, the goListDriver response returns data that is missing in the Gopackagesdriver response.

Currently, the Module field is not parsed from the external driver response, implemented as an UnmarshalJSON override in packages.go:

	*p = Package{
                  ID:              flat.ID,
                  Name:            flat.Name,
                  PkgPath:         flat.PkgPath,
                  Errors:          flat.Errors,
                  GoFiles:         flat.GoFiles,
                  CompiledGoFiles: flat.CompiledGoFiles,
                  OtherFiles:      flat.OtherFiles,
                  EmbedFiles:      flat.EmbedFiles,
                  EmbedPatterns:   flat.EmbedPatterns,
                  ExportFile:      flat.ExportFile,
                  // Module is not parsed
	}

However, it is parsed from the goListDriver response, in golist.go:

	pkg := &Package{
	        Name:            p.Name,
	        ID:              p.ImportPath,
	        GoFiles:         absJoin(p.Dir, p.GoFiles, p.CgoFiles),
	        CompiledGoFiles: absJoin(p.Dir, p.CompiledGoFiles),
	        OtherFiles:      absJoin(p.Dir, otherFiles(p)...),
	        EmbedFiles:      absJoin(p.Dir, p.EmbedFiles),
	        EmbedPatterns:   absJoin(p.Dir, p.EmbedPatterns),
	        IgnoredFiles:    absJoin(p.Dir, p.IgnoredGoFiles, p.IgnoredOtherFiles),
	        forTest:         p.ForTest,
	        depsErrors:      p.DepsErrors,
	        Module:          p.Module,  // Module is parsed
	}

The proposal is to add the Module field to the UnmarshalJSON override in packages.go, so that packages.Load returns Module data when using either driver. This also involves adding the field to the flatPackage struct.

The fields required in the Module field for our usecase is as follows:

1. Module.Version
2. Module.Dir
3. Module.Path

Let me know if this sounds good, or if there are any alternatives to consider or suggestions about this approach.

[timothy-king]

From MarshalJSON's doc:

// This method exists to enable support for additional build systems.  It is
// not intended for use by clients of the API and we may change the format.

Is there a build system or tool that would benefit from populating Module information in Package.MarshalJSON? It is kinda clear we could be more consistent here, but I am wondering if there is another motivation. (It is not obvious to me bazel could take advantage of this for example.)

Also can you be a bit more clear about what you are proposing would be serialized for Module? As in which fields? We do not serialize all of Package for example. Does this include Replace? A prototype/CL could also clarify this.

[JamyDev]

We've been working on creating stored indices using sourcegraph/scip-go. It encodes version information from the module data, which is available when using the standard Go import system, but with Go Packages Driver we're not able to populate version information.

Info needed, mostly Module.Version I think, @rastenis correct me please.

It uses packages.Load to get the packages in the project.

[timothy-king]

Info needed, mostly Module.Version I think, @rastenis correct me please.

For a proposal to go through the proposal process https://github.com/golang/proposal#the-proposal-process, I would recommend updating the top comment to have a complete list of the fields from Modules you think should be included. I am guessing Module.Path and ModuleError are other requirements?

I presume when serializing a packages.Package, there will be a copy of the serialized *Packages.Module when available. This will be somewhat redundant as there can be many packages per module.

[rastenis]

I have added the required fields above. For our direct use case, Module.Version and Module.Dir are required, however I also added Module.Path since it is referenced multiple times in the project we are modifying for our use cases, so I included it as well to be safe.

[adonovan]

This proposal seems reasonable. It ought to be possible to proxy for a go/packages driver to produce anything 'go list' can produce--even if it doesn't correspond to an actual build system such as bazel, pants, buck, etc--so that it can be used to proxy a remote project, for example. So I would be in favor of adding a Module field, whose type is *packages.Module.