cmd/vet: switch to golang.org/x/tools/go/analysis/cmd/vet-lite (tracking)

[alandonovan]

This is a tracking bug for all issues related to switching the implementation of cmd/vet to the new implementation golang.org/x/tools/go/analysis/cmd/vet-lite.

Known issues:

• flag compatibility. The new flags are named -a.f where a is the analysis name and f is the flag name. For example, -printf.funcs. This includes the tristate enable flag: -printf.enable. After discussion with Russ, I plan to make the enable flag just called -printf and add shims for all other legacy flags.
• There is no -all flag. The shadow analysis is disabled. There is no mechanism for experimental (off by default) analyses. These all need to be fixed.
• cmd/vet/all needs work to make it use go vet instead of driving cmd/vet directly.
• The x/tools fork of the analyzers is several CLs behind cmd/vet. Bring it up to date.
• The flags used by the go vet <=> vet protocol should have obscure names (e.g. -vet.flags to avoid conflict with an analyzer named "flags") and not appear in the -help message.

[mvdan]

@alandonovan once the merge into the main repository is done, please do let us know how we should write future vet changes.

[alandonovan]

After the switch, most vet changes should be made to x/tools, then brought into $GOROOT with a script that I will write and document.

[mvdan]

Is vendoring the long term plan for both vet and go/packages? I was imagining that the vendor mechanism was just temporary, ending in either x/tools being split into modules, or these packages being made a part of the main repository.

[alandonovan]

We've long been talking about go/packages ending up in the standard library, perhaps for go1.13. The go/analysis API might also belong there, though likely with all its implementations, in which case they would still have to be vendored from x/tools. Splitting x/tools into modules has itself been a complicated discussion and we still haven't decided on a definitive plan.

So, yes, vendoring will be part of the plan for at least the medium term.

[ainar-g]

The shadow analysis is disabled.

Just chiming in saying that we actively use -shadow -shadowstrict at $COMPANY, and it has found real bugs in the past, so please do fix it.

[dgryski]

@ainar-g Try https://github.com/gordonklaus/ineffassign . It catches the almost all the same issues shadow does without the noise.

[alandonovan]

@ainar-g Thanks for the information; it can be hard to tell who uses the tools sometimes, especially when they're working and no-one is complaining. :)

Since the only "experimental" checker is shadow, I would rather not introduce a new concept in the analysis API to deal with it. The beauty of the new design is that it's very easy to run any checks you want even if they're not in the core vet suite. See the description of CL https://go-review.googlesource.com/c/tools/+/148565. I appreciate that this is a minor usability regression for those users who explicitly enable the -shadow check.

[ainar-g]

@dgryski Here is a very simplified version of a real bug that has been found by shadowstrict. Neither ineffassign, nor the new vet, not even staticcheck (both the release and pre-release versions) found anything wrong with this code because it might seem like there isn't. But there is.

In general, I think that banning shadowing improves programmers' capability to reason about code. This is up for a debate, of course, but in my personal experience it is a good addition to one's arsenal of defensive practices.

---

@alandonovan First of all, thank you for all the work you're putting in. Trust me, it is not left unappreciated. Perhaps I am just a vocal minority, and if it is the case, I guess I'll take the tool over nothing and be grateful for that. But still, I can't fully approve a removal of a piece of functionality, experimental or not, with real-life uses and real bugs behind it. I would prefer the shadow pass to be in vet, just disabled by default.

Does the new pass cover only the old -shadow, or does it provide the full -shadow -shadowstrict experience?

[alandonovan]

@ainar-g The standalone shadow tool supports the -strict flag, as before. BTW, if the first log.Println in your example were replaced by a return statement, the golang.org/x/tools/go/analysis/passes/nilness checker would have caught the mistake. You might want to give it a try. I agree that control flow and shadowing, particularly around errors, can be confusing.

[ainar-g]

@alandonovan The nilness pass is great, and I've already successfully used it in my projects. IIRC, it is not in vet-lite at the moment? I hope the plan is to ship it.

[alandonovan]

@ainar-g Initially the plan is parity with legacy vet. The nilness checker depends on SSA, which makes it more expensive. We can measure the cost and benefit of adding it once the dust has settled.

[ugorji]

I just filed #28869 , and then stumbled upon this issue. Not sure if I should have just added this here.

Basically, "go tool vet ." throws an error saying:

vet: invalid command: want .cfg file (this reduced version of vet is intended to be run only by the 'go vet' command)

This is breaking flycheck used in my daily emacs development.

[mvdan]

@ugorji you should just use go vet . - have you encountered any issues with that command?

[ugorji]

@ugorji you should just use go vet . - have you encountered any issues with that command?

@mvdan I use go vet, and I love the changes @alandonovan is making to the tool. But go tool vet is documented, and depended on by flycheck, and while I am developing on tip, flycheck keeps giving me these error messages constantly on each save.

[mvdan]

Have you asked flycheck to switch to go vet? I think it should be fine to make breaking changes to go tool vet in this release, since it's not really meant to be known or used by developers directly.

If a tool depended on go tool vet pkgs... that's fine - it can just switch to go vet pkgs..., or be taught to use go vet pkgs... if the Go version is 1.12 or later.

[ugorji]

@mvdan

I don't control flycheck, but it is a very widely used tool within emacs community, and it supports many versions of go (not just the upcoming one) and all the other languages (C++, Java, Rust, Python, etc) and modes (XML, YAML, etc), and "go tool vet" has always been published as a tool for developers to use directly. I don't think we should break it - flycheck depending on it is a compelling reason already. Unless @alandonovan says there's a more compelling architectural reason why not to support it, we should IMO.

[mvdan]

"go tool vet" has always been published as a tool for developers to use directly

Could you clarify where that is documented? The closest I can find is https://golang.org/cmd/vet/#hdr-Using_vet_directly, which clearly says testing and debugging:

For testing and debugging vet can be run directly by invoking "go tool vet" or just running the binary. Run this way, vet might not have up to date information for imported packages.

[ugorji]

@mvdan I feel like this is a moot point. "go tool" is published, and is shown to be widely used by tools that do not disambiguate based on the version of go. Unless there is an architectural reason why something that has been supported forever cannot be anymore, we should maintain compatibility and save everyone the pain of updating their tools.

Feel free to try to get flycheck to change - I just wanted to raise this so @alandonovan can see a compelling reason to continue to support before we release (and this is if the breaking change was on purpose, as opposed to that his work is still in progress and not yet done).

[alandonovan]

Unless there is an architectural reason why something that has been supported forever cannot be anymore, we should maintain compatibility and save everyone the pain of updating their tools.

In this case there is an architectural reason. See #28869 (comment).

[ianlancetaylor]

@alandonovan Is there anything else to do on this issue?

[alandonovan]

No, I think we can close it.