Go: x/tools/go/packages: clarify behavior when given zero patterns

[mvdan]

I've ported a few tools over from a mix of https://github.com/kisielk/gotool and x/tools/go/loader over to go/packages. The transition was quite simple for most tools, and everything still worked fine.

Until today, when I happened to notice that running unparam was much faster than unparam .. The reason is that, of course, the former does nothing.

The gotool package was extracted from cmd/go, so it automatically treated zero patterns as ".": https://github.com/kisielk/gotool/commit/49706c2c283d7370cff083d236a3c55fbe912742

I presume it's fair enough if go/packages goes a different direction, but I think that something should be done to avoid confusion. I'm willing to bet that I'm not the only one that has ported a Go tool over to go/packages and not noticed that the zero argument case has silently broken.

Options that come to mind, in order of personal preference:

• Extend It may return an empty list of packages without an error, for instance for an empty expansion of a valid wildcard to also include the zero patterns case.
• Make Load(cfg) behave like Load(cfg, ".")
• Make packages.Load error when given zero patterns

If we go with the doc fix route, I'd suggest including the string "." somewhere in the docs. That's the main string I was using to search for this edge case's behavior.

/cc @ianthehat @matloob @alandonovan

[rogpeppe]

From the go/packages docs:

Most tools should pass their command-line arguments (after any flags) uninterpreted to the loader, so that the loader can interpret them according to the conventions of the underlying build system. See the Example function for typical usage.

By that light, I think that it should treat no packages as the same as []string{"."} to match the semantics of the go tool.

[alandonovan]

I agree there is a problem here. The tricky thing is that the underlying build systems vary in their treatment of an empty argument list. The go tool treats [ ] as ["."], but Blaze and Bazel treat [ ] as an empty request: a no-op. Furthermore "." is not a valid argument to Bazel; the closest concept is ":all", but users would not conventionally expect [ ] to mean [":all"].

If we follow the principle outlined in the docs ("the conventions of the underlying build system") then the interpretation of [ ] may vary, which is fine for interactive command-line users who know the conventions of their build system. If an application driving packages.Load needs a way to say "all the packages in the current directory", then perhaps we should add another special query (like name=..., file=...), but I have not yet come across the need.

[mvdan]

I imagined that this behavior might only concern the go list driver, and that's fine.

In that case, I presume we can simply change the driver's behavior, and leave the docs untouched.

[ianthehat]

I think we should leave this up to the driver but call out explicitly in the docs to Load that an empty pattern list is driver defined behavior, because it might surprise some people, and programmatic uses that are trying to avoid driver defined behavior need to know to avoid this case.
And then yes I think the golist driver should default to "." in this case.

[mvdan]

Would a patch be welcome for this? It would likely take me less time to send a fix to x/tools than to apply a workaround in a handful of tools, to then undo it later :)

[ianthehat]

Yes, absolutely, we love patches :)

[mvdan]

Ah cool. I wasn't sure, since the issue had been assigned.

[matloob]

I hereby gift this issue to you! Thanks!!

[mvdan]

Christmas came early :)

[matloob]

This was fixed by https://go-review.googlesource.com/c/tools/+/155898

[mvdan]

Agh, I keep referencing issues wrong in the subrepo CLs.