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

proposal: cmd/vet: add mechanism to silence false positives #17058

Closed
minux opened this issue Sep 11, 2016 · 14 comments

Comments

Projects
None yet
9 participants
@minux
Copy link
Member

commented Sep 11, 2016

@josharian added an internal whitelist feature for vetting the standard
library. I think such a whitelisting feature is also useful for outside
packages.

(If cmd/vet could load project specific plugins, then this feature might
be less useful, but not all projects need custom vet checkers, so it
still makes sense to provide a simple whitelisting feature built in.)

Such a feature could be built upon @josharian's existing whitelist
feature (cmd/vet/all).

@minux minux added the Proposal label Sep 11, 2016

@quentinmit quentinmit added this to the Proposal milestone Sep 12, 2016

@adg

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2016

cc @robpike

Seems reasonable to me.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2016

Should be easy in principle but messy in detail. It's really up to Josh.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2016

Fine by me, although I'm not going to volunteer to do it. :)

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

It would be nice to have a general mechanism to disable false positives from vet (or at least eliminate them all). I think we don't know what that should be, but we should solve that more general problem. Suggestions welcome.

With that problem solved, we could promote cmd/vet to be a more integral part of the compile-edit-debug cycle, for example doing things like run the test binary and vet on the directory at the same time, and make a vet failure count as a test failure by default.

@rsc rsc changed the title Proposal: cmd/vet: add builtin whitelist feature proposal: cmd/vet: add mechanism to silence false positives Nov 7, 2016

@robpike

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2016

I am adamant that source code not be annotated to silence vet. That will reduce the options available but that is my position.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

//go:positionheard

// +solution !source_annotate

@minux

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2016

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2016

Seconded. Go code is already festooned with magic pragmas controlling the
interaction of variables and the garbage collector. Please let that be the
end of it.

On Tue, 8 Nov 2016, 14:30 Minux Ma notifications@github.com wrote:

I agree with Rob. No more magic comments (if vet set the precedence of
magic comments to silence false positives, then soon other static checkers
will follow suit and this will make Go code full of magic comments for
different tools.)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#17058 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA4l4_SeqD0uM_ZdyUyVfINhuARz6ks5q7-zRgaJpZM4J54tu
.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

We agree we don't want "vet:xxx" directives. We would like to fix the problem about false positives. But we don't have a solution. Any ideas?

It could be that we raise the bar for "on by default" in vet and fix up the noisy checks we want to keep. That would benefit everyone.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

Maybe another option is to define a subset of the "go vet" on by default checks to be run during 'go test'. A few of the noisier ones might be on by default if you run "go vet" but not in the implicit vet during test.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

The subset-of-vet-checks idea seems the most promising approach so far—no configuration plus no magic comments plus no unnecessary code changes pretty much requires zero false positives, and some of the existing, worthwhile vet checks don't meet that demanding bar.

It's worth considering separately whether we want a more general (configurable?) run-before-test hook, to inject other analysis tools, instead of something vet-specific.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2016

Isn't that the purpose of an IDE or build wrapper? It seems wrong to build general layers of tooling into go test, which is already so complex it beggars belief.

As the person who suggested the subset approach, I can support calling out to go vet in go test, preferably under some flag (tests take long enough as it is), but cannot support a general test plugin mechanism.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2016

Isn't that the purpose of an IDE or build wrapper?

You could make the same argument about vet, no?

The argument in favor of including vet in go test is that they serve a similar purpose (finding bugs) and that getting the extra checks as part of go test makes developers' lives easier. That applies, mutatis mutandis, to other tools.

A concrete suggestion: Pick a canonical executable name (go_test_exec? go_test_static?). When go test is run, check whether there is such an executable on the path. If present, execute it, passing it the package path(s), and use its return code to decide whether to fail. (That's for on-by-default. For off-by-default, instead of adding a -vet flag to go test, add a -check flag to go test that accepts an executable path.) Developers could then put whatever calls they want into a shell script to execute, or pick just a single tool. Vet could either be a special case, executed separately, or not, in which case, we should probably add a flag to vet to enable only the no-false-positive checks.

I'm not going to push this further.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2016

We've drifted slightly. The proposal was "cmd/vet: add mechanism to silence false positives". I think we agree that if there are noisy false positives then we need to fix those or turn them off by default. This suggests that many vet checks are not as precise as they really should be, and we should fix those. I've created #18084 and #18085 based on the discussion here, but we'll decline this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.