Skip to content
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

Jump to bottom

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

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

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

mvdan opened this issue Nov 13, 2018 · 11 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unreleased

Comments

@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 Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 13, 2018
@gopherbot gopherbot added this to the Unreleased milestone Nov 13, 2018
@rogpeppe
Copy link
Contributor

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
Copy link
Contributor

alandonovan commented Nov 13, 2018 •
edited

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
Copy link
Member Author

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
Copy link

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 mvdan mentioned this issue Nov 13, 2018
Closed
@mvdan
Copy link
Member Author

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
Copy link

Yes, absolutely, we love patches :)

@mvdan
Copy link
Member Author

mvdan commented Nov 26, 2018

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

@matloob
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
Copy link
Member Author

mvdan commented Dec 4, 2018

Christmas came early :)

@matloob
Copy link
Contributor

matloob commented Jan 10, 2019

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

@matloob matloob closed this as completed Jan 10, 2019
@mvdan
Copy link
Member Author

mvdan commented Jan 10, 2019

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

@golang golang locked and limited conversation to collaborators Jan 10, 2020
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@rogpeppe @ianthehat @mvdan @alandonovan @gopherbot @matloob