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/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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

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
@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label May 8, 2019
@letientai299
Copy link
Contributor

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

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

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

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

qbradq commented Jul 16, 2019

@gopherbot please add label NeedsDecision

@gopherbot gopherbot added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jul 16, 2019
@ianlancetaylor
Copy link
Contributor

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

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 The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. 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
Copy link

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

@ccressent ccressent mentioned this issue Feb 12, 2020
Closed
@dnephin dnephin mentioned this issue Aug 21, 2020
Merged
@golang golang locked and limited conversation to collaborators Aug 27, 2020
@rsc rsc unassigned matloob 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 NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
8 participants
@rsc @andybons @qbradq @ianthehat @ianlancetaylor @letientai299 @gopherbot @matloob