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

x/tools/go/analysis/cmd/vet: delete this program #31886

Closed
rsc opened this issue May 7, 2019 · 8 comments
Closed

x/tools/go/analysis/cmd/vet: delete this program #31886

rsc opened this issue May 7, 2019 · 8 comments
Assignees
Labels
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented May 7, 2019

go tool vet and golang.org/x/tools/go/analysis/cmd/vet should have the same set of checks but today do not. It looks like the x/tools version adds atomicalign, errorsas, and nilness.

If any of these are not ready to go into std (and nilness seems to be called out explicitly as not for std), they should not be in the x/tools vet either. (If the x/tools vet is going to intentionally diverge from cmd/vet, we should avoid confusion by naming it something different from vet.)

$ go tool vet help >vet.help 2>&1
$ /tmp/xvet help >xvet.help 2>&1
$ diff vet.help xvet.help
1c1
< vet is a tool for static analysis of Go programs.
---
> xvet is a tool for static analysis of Go programs.
3c3
< vet examines Go source code and reports suspicious constructs,
---
> xvet examines Go source code and reports suspicious constructs,
12a13
>     atomicalign  check for non-64-bits-aligned arguments to sync/atomic functions
17a19
>     errorsas     report passing non-pointer or non-error values to errors.As
21a24
>     nilness      check for redundant or impossible nil comparisons
46a50,51
>   -atomicalign
>     	enable atomicalign analysis
64a70,75
>   -cpuprofile string
>     	write CPU profile to this file
>   -debug string
>     	debug flags, any subset of "fpstv"
>   -errorsas
>     	enable errorsas analysis
74a86,87
>   -memprofile string
>     	write memory profile to this file
78a92,93
>   -nilness
>     	enable nilness analysis
96a112,113
>   -trace string
>     	write trace log to this file
111c128
< To see details and flags of a specific analyzer, run 'vet help name'.
---
> To see details and flags of a specific analyzer, run 'xvet help name'.
$ 

/cc @matloob @ianthehat

@rsc rsc added this to the Go1.13 milestone May 7, 2019
@rsc rsc added the release-blocker label May 7, 2019
@andybons andybons added the NeedsFix label May 8, 2019
@letientai299

This comment has been minimized.

Copy link
Contributor

@letientai299 letientai299 commented Jun 11, 2019

@andybons I would like to help unblock this release-blocker. Right now, compare between master of go and tools report, go vet don't have atomicalign and errorsas. Does the decision is to add the 2 checks to go vet?

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Jun 11, 2019

I believe the decision was to just delete golang.org/x/tools/go/analysis/cmd/vet.

Adding new checks to go vet at this point in the release process is not something we should do without a very compelling reason, especially as we have not done any analysis to see if they are reliable enough.

@letientai299

This comment has been minimized.

Copy link
Contributor

@letientai299 letientai299 commented Jun 11, 2019

I believe the decision was to just delete golang.org/x/tools/go/analysis/cmd/vet.

I don't think so. This CL 181717 is merged today, for adding errorsas to vet.

@matloob

This comment has been minimized.

Copy link
Contributor

@matloob matloob commented Jun 11, 2019

I agree with the hat.

We've planned to add errorsas a while ago. I think it's better to proceed with caution than to add a new analyzer.

We should delete the vet tool in tools: it's a safer thing to do, especially in the freeze.

@qbradq

This comment has been minimized.

Copy link
Contributor

@qbradq qbradq commented Jul 16, 2019

@gopherbot please add label NeedsDecision

@gopherbot gopherbot added NeedsDecision and removed NeedsFix labels Jul 16, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 16, 2019

@ianthehat @matloob It sounds like you feel that someone should delete this program. Could one of you do that soon, so that it can be in the 1.13 release? Thanks.

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Jul 16, 2019

x/tools is not actually part of the release, and no changes are needed in the vet that is part of the release, so this is not really a release blocker given the decision to just delete the one in x/tools. I agree we should just get it done anyway though.
Assigning to @matloob

@andybons andybons added NeedsFix and removed NeedsDecision labels Jul 22, 2019
@ianlancetaylor ianlancetaylor changed the title cmd/vet: different set of checks from x/tools/go/analysis/cmd/vet x/tools/go/analysis/cmd/vet: delete this program Aug 2, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Go1.13, Unreleased Aug 2, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 28, 2019

Change https://golang.org/cl/192177 mentions this issue: go/analysis/cmd/vet: remove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.