x/tools/go/packages: add EmbedFiles and EmbedPatterns

[hugelgupf]

What version of Go are you using (go version)?

$ go version
1.16, 1.17, 1.18+

Feature Request

x/tools/go/packages has in the Package struct fields for GoFiles, OtherFiles, IgnoredFiles, etc, corresponding to (I think) the output of go list -json <package>.

Since go:embed support was added in Go 1.16, go list -json has also been listing EmbedPatterns and EmbedFiles in its output. I would like for those to be exported in Package as EmbedFiles []string and EmbedPatterns []string if possible.

[gopherbot]

Change https://golang.org/cl/379974 mentions this issue: x/tools/go/packages: add Embed support

[matloob]

I think this is reasonable and we should go forward with it. The only potential design question is whether the embeds should be guarded with their own Need flag to potentially allow for potentially doing less work in the go command. We probably should, just to be safe, even if it adds a bit of annoyance to the API.

[hugelgupf]

I think this is reasonable and we should go forward with it. The only potential design question is whether the embeds should be guarded with their own Need flag to potentially allow for potentially doing less work in the go command. We probably should, just to be safe, even if it adds a bit of annoyance to the API.

I'd be happy to do that. I saw in #38445 that an invariant of the packages API is that every build system should be able to produce this information. I assume EmbedFiles and EmbedPatterns are things that bazel can produce?

[findleyr]

Moving this to a proposal, following discussion on the associated CL: since this is an API addition, it must go through the proposal process.

With that said this seems like an unambiguous improvement to me, and would be useful for many tools. I think we should do it.

[timothy-king]

From x/tools/go/analysis perspective, EmbedFiles []string seems like an obvious win. These are a part of the program being stored elsewhere and "here is where to fine them" is useful. Any analyzer looking at string/[]byte constants can load their contents once this is available.

1. Do we have a use case for how EmbedPatterns []string would be used? (This may be obvious to others.)
2. Would EmbedFiles be all of the matching embedded files [all files that match all patterns] or all of the instances appearing in the code?

[hugelgupf]

From x/tools/go/analysis perspective, EmbedFiles []string seems like an obvious win. These are a part of the program being stored elsewhere and "here is where to fine them" is useful. Any analyzer looking at string/[]byte constants can load their contents once this is available.

1. Do we have a use case for how EmbedPatterns []string would be used? (This may be obvious to others.)

Maybe it's not as useful given that EmbedFiles contains all the files that are matched by EmbedPatterns? I personally don't have as much of a use for it, so I could drop it from the proposal. But I also don't see why not to add it.

2. Would EmbedFiles be all of the matching embedded files [all files that match all patterns] or all of the instances appearing in the code?

In my experiments with go list -json, EmbedFiles also always contains all files that match the patterns in EmbedPatterns. I think it's most useful that way, anyway, so that should be true for the API here as well.

[timothy-king]

Can EmbedPatterns be recovered easily from the .go files or the ast in the package? If so, that would be a somewhat strong argument for not including them. (Maybe it would be easy if an extra parsing function is exposed?)

[hugelgupf]

Can EmbedPatterns be recovered easily from the .go files or the ast in the package? If so, that would be a somewhat strong argument for not including them. (Maybe it would be easy if an extra parsing function is exposed?)

I saw this in go/ast: https://pkg.go.dev/go/ast#CommentGroup.Text

"Comment directives like "//line" and "//go:noinline" are also removed."

But I did not check this out in practice.

So I expect that it is not possible to get this information from the go/ast package.

[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

[rsc]

I don't see much harm in including both EmbedFiles and EmbedPatterns. They are both useful and already provided by the go/build API. It would be weird to throw away the info and make the caller recompute it.

One possibility would be a flag to get the patterns without the files, since getting the files is more expensive.

[timothy-king]

since getting the files is more expensive.

@rsc Did you mean getting the content of the matching files or the getting the paths to the matching files?

[rsc]

One possibility would be a flag to get the patterns without the files, since getting the files is more expensive.
Did you mean getting the content of the matching files or the getting the paths to the matching files?

I meant just deciding which files match the patterns. That can be quite expensive because it can in general require walking around the file system looking for matches. Consider //go:embed */*/*. Now that I write that, I'm pretty sure EmbedFiles should be behind a flag.

[hugelgupf]

One possibility would be a flag to get the patterns without the files, since getting the files is more expensive.
Did you mean getting the content of the matching files or the getting the paths to the matching files?

I meant just deciding which files match the patterns. That can be quite expensive because it can in general require walking around the file system looking for matches. Consider //go:embed */*/*. Now that I write that, I'm pretty sure EmbedFiles should be behind a flag.

I don't have a lot of insight into the other implementations, but at least in the go list based one, you don't get much of a choice -- go list lists the EmbedFiles whether you want them or not, and the flag would just influence whether it's parsed from JSON and given back in the API or not. But this is probably fair for other implementations.

Happy to have a NeedEmbedFiles flag. (Or call it NeedEmbed and guard both EmbedFiles and EmbedPatterns behind it?)

[ianthehat]

If we implement field based pruning in go list (#29666) then go/packages would use it based on its flags and gain the performance improvements.

[hugelgupf]

Then I'm all for the separate flag

[rsc]

OK, so it sounds like we are talking about:

1. In x/tools/go/packages.Package, add EmbedFiles and EmbedPatterns fields.
2. Add separate NeedEmbedFiles and NeedEmbedPatterns bits too (or maybe NeedEmbedPatterns would tag along with something else?)
3. Assuming go list gets the JSON-trimmer, make go/packages know not to ask for EmbedFiles if it will not use them.

Does that sounds right?

/cc @matloob

[rsc]

@matloob, does the previous comment sound right to you?

[matloob]

Yes, that sounds right. Thanks!

Separate NeedEmbedFiles and NeedEmbedPatterns seems like the right thing to do.

[kortschak]

"Comment directives like "//line" and "//go:noinline" are also removed."

But I did not check this out in practice.

So I expect that it is not possible to get this information from the go/ast package.

They are not in the comment groups as rendered by the Text method, but they are available in the raw comment lines.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[hugelgupf]

Thanks Russ and @matloob. Let me know when we're ready to go and I'll pick up my CL again :)

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[hugelgupf]

Thanks Russ! CL should be ready for review again: https://golang.org/cl/379974