New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mvdan opened this Issue Nov 13, 2018 · 11 comments

Comments

Projects
None yet
6 participants
@mvdan
Copy link
Member

mvdan commented Nov 13, 2018

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 ".": kisielk/gotool@49706c2

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

@mvdan mvdan added the NeedsDecision label Nov 13, 2018

@gopherbot gopherbot added this to the Unreleased milestone Nov 13, 2018

@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor

alandonovan commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Member

mvdan commented Nov 13, 2018

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

This comment has been minimized.

Copy link

ianthehat commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Member

mvdan commented Nov 26, 2018

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

This comment has been minimized.

Copy link

ianthehat commented Nov 26, 2018

Yes, absolutely, we love patches :)

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Nov 26, 2018

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

@matloob

This comment has been minimized.

Copy link
Contributor

matloob commented Dec 4, 2018

I hereby gift this issue to you! Thanks!!

@matloob matloob assigned mvdan and unassigned matloob Dec 4, 2018

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 4, 2018

Christmas came early :)

@matloob

This comment has been minimized.

Copy link
Contributor

matloob commented Jan 10, 2019

@matloob matloob closed this Jan 10, 2019

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 10, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment