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

cmd/vet: make -all -shadow mean all default checks and also -shadow #13020

Closed
kostya-sh opened this issue Oct 22, 2015 · 4 comments
Closed

cmd/vet: make -all -shadow mean all default checks and also -shadow #13020

kostya-sh opened this issue Oct 22, 2015 · 4 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted
Milestone
Go1.6

Comments

@kostya-sh
Copy link
Contributor

Currently the only way to run all vet checks in a single invocation is -test flag:

go tool vet -test *.go

However the -test flag documentation says that it is supposed to be used only for testing.

It is possible to achieve the same effect by invoking go tool vet multiple times:

go tool vet *.go
go tool vet -shadow *.go

The problem with this approach is that if more experimental checks are added in the future or shadow become a standard checker these commands will have to be changed.

I propose to

  • add -extra flag to go vet tool that enables all experimental checkers that are not enabled by -all flag (similar to -Wall and -Wextra in gcc).
  • remove (or deprecate) -test flag. Use combination of -all and -extra flags in the tests

Examples:

  • Run all standard checks (exactly the same as now): go tool vet *.go or go tool vet -all *.go
  • Run all extra checks (currently the only extra check is shadow): go tool vet -extra *.go
  • Run all checks (including extra checks): go tool vet -all -extra *.go

This change will also allow integrating shadow vet check into Emacs flycheck package. See the following discussion for more details: flycheck/flycheck#765
@robpike
Copy link
Contributor

robpike commented Oct 22, 2015

I think you shouldn't turn on -shadow by default in your plugin. That is the underlying motivation for this change, but -shadow just isn't reliable enough to depend on. It doesn't work well at all. That's why it's disabled.

If you really want it enabled always (which is a very bad idea), just have the plugin turn it on.

Whether another experimental feature comes along, and what its properties, reliability, and safety might be are all unknown at this point, so planning for them in a way that others may depend on seems unwise.

I vote against this proposal.

@kostya-sh
Copy link
Contributor Author

First of all while the shadow check is not accurate I still find it helpful especially when used interactively with an editor. From my experience accidental shadowing usually leads to unpredictable bugs that are hard to track (possibly because I am coming from Java where shadowed variables are not allowed) and any help from the tools is appreciated.

Secondly the main motivation for this proposal is not to enable shadow check by default but to make it easier to turn it on by plugins.

The only way to run all vet checks (including shadow) at the moment is to invoke go vet twice. It is possible to do it from the flycheck but the plugin author is reluctant to accept such change because it adds more complexity and such approach is different from the rest of the checkers. It would be good if it was possible to have a new flycheck configuration option "enable-shadow" that just translates to a go vet command line flag.

The concern about any upcoming experimental features is reasonable though. I'd like to propose an alternative approach to -extra flag: if -all flag is specified then any additional checks enabled via command line flags should run together with the checks enabled by -all flag. E.g.

  • go tool vet -all will run all standard checks (as it does now)
  • go tool vet -all -shadow will run all standard checks and shadow check (currently this command runs only shadow check)
  • go tool vet -shadow will run only shadow check (as it does now)

@robpike
Copy link
Contributor

robpike commented Oct 23, 2015

If go tool vet -all -shadow doesn't run -all, only runs -shadow, that's just a bug. Please file an issue.

@rsc rsc added this to the Proposal milestone Oct 24, 2015
@rsc rsc changed the title proposal: cmd/vet: make possible to run all (including shadow) vet checks in a single invocation cmd/vet: make -all -shadow mean all default checks and also -shadow Oct 24, 2015
@rsc rsc modified the milestones: Go1.6, Proposal Oct 24, 2015
@gopherbot
Copy link

CL https://golang.org/cl/16325 mentions this issue.

@rsc rsc closed this as completed in 3fb9e08 Dec 5, 2015
@golang golang locked and limited conversation to collaborators Dec 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
4 participants
@rsc @robpike @kostya-sh @gopherbot