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

cmd/vet: identify 100% reliable checks for testing #18085

Closed
rsc opened this Issue Nov 28, 2016 · 26 comments

Comments

Projects
None yet
@rsc
Contributor

rsc commented Nov 28, 2016

For #18084 we need to find some way to run just the "100% reliable" vet checks. It's okay if a "100% reliable" check flags a false positive as long as there is a clean way to rewrite the code to avoid the false positive.

go test would need some way to invoke that set, like a new flag.

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 28, 2016

Out of curiosity - why not disable by default those that aren't reliable on vet directly? I assume this is what go tool vet does with -shadow:

  -shadow
        check for shadowed variables (experimental; must be set explicitly)

I think the reason why go vet isn't used regularly more often is because it's not conservative by default. A good example is the standard library itself with the whitelist.

@josharian

This comment has been minimized.

Contributor

josharian commented Nov 28, 2016

Perhaps so. The standard library is not a good test case though; it is unusual in virtue of being the standard library, and that accounts for much of the whitelist.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 29, 2016

I think the whitelists for the standard library could be reduced dramatically by making various checks more precise or off by default.

When I spoke to @robpike about this, he believed strongly that the "default on" set should be allowed to include reports that were only, say, 90% reliable. It's hard to evaluate in the abstract but that sounds plausible.

Probably the way forward is to work on reducing false positives and figuring out the "clean rewrites" (i.e. not a //vet:goaway comment) that apply. If we can get the current default set to all be "100% reliable", that's great. If not, we'll have a concrete check to talk about instead of hypotheticals.

@btracey

This comment has been minimized.

Contributor

btracey commented Jan 18, 2017

A common vet complaint in my packages is "composite literal uses unkeyed fields". If #18084 is accepted, would this be (effectively) considered a bug? One could argue that a clean rewrite is available (add the fields), but there are some good reasons not to use fields, legibility being one of them.

@rsc

This comment has been minimized.

Contributor

rsc commented Jan 18, 2017

Yes, code is expected to use composite literals with keyed fields whenever the literal is initializing a struct defined in another package. Otherwise the package cannot safely add a new field without breaking clients of that package. This puts some teeth behind the requirement listed in https://golang.org/doc/go1compat. Please file a separate issue if you think we should relax this, and explain under what circumstances that make sense that would handle your case. (For example I could imagine relaxing when the package being used is an internal package.)

@calmh

This comment has been minimized.

Contributor

calmh commented Jan 18, 2017

Are types from other packages that are really slices always recognized nowadays? I recall lots of issues with that warning on things like

package foo

type Vector []int
package bar

import "foo"

var v = foo.Vector{1, 2, 3}

At the very least it seemed unreliable, in that it depended on having a compiled version of foo available when vetting bar in order to avoid the warning, or something similar?

@josharian

This comment has been minimized.

Contributor

josharian commented Jan 18, 2017

@calmh related issues: #16086, #11394, #15408.

@gopherbot

This comment has been minimized.

gopherbot commented Nov 1, 2017

Change https://golang.org/cl/74356 mentions this issue: cmd/go: run vet automatically during go test

gopherbot pushed a commit that referenced this issue Nov 3, 2017

cmd/go: run vet automatically during go test
This CL adds an automatic, limited "go vet" to "go test".
If the building of a test package fails, vet is not run.
If vet fails, the test is not run.
The goal is that users don't notice vet as part of the "go test"
process at all, until vet speaks up and says something important.
This should help users find real problems in their code faster
(vet can just point to them instead of needing to debug a
test failure) and expands the scope of what kinds of things
vet can help with.

The "go vet" runs in parallel with the linking of the test binary,
so for incremental builds it typically does not slow the overall
"go test" at all: there's spare machine capacity during the link.

all.bash has less spare machine capacity. This CL increases
the time for all.bash on my laptop from 4m41s to 4m48s (+2.5%)

To opt out for a given run, use "go test -vet=off".

The vet checks used during "go test" are a subset of the full set,
restricted to ones that are 100% correct and therefore acceptable
to make mandatory. In this CL, that set is atomic, bool, buildtags,
nilfunc, and printf. Including printf is debatable, but I want to
include it for now and find out what needs to be scaled back.
(It already found one real problem in package os's tests that
previous go vet os had not turned up.)
Now that we can rely on type information it may be that printf
should make its function-name-based heuristic less aggressive
and have a whitelist of known print/printf functions.
Determining the exact set for Go 1.10 is #18085.

Running vet also means that programs now have to type-check
with both cmd/compile and go/types in order to pass "go test".
We don't start vet until cmd/compile has built the test package,
so normally the added go/types check doesn't find anything.
However, there is at least one instance where go/types is more
precise than cmd/compile: declared and not used errors involving
variables captured into closures.

This CL includes a printf fix to os/os_test.go and many declared
and not used fixes in the race detector tests.

Fixes #18084.

Change-Id: I353e00b9d1f9fec540c7557db5653e7501f5e1c9
Reviewed-on: https://go-review.googlesource.com/74356
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@rogpeppe

This comment has been minimized.

Contributor

rogpeppe commented Nov 14, 2017

Another example (reported in #22715):

An example from our code base; we have an errorf method defined:

func (i *idM) errorf(w http.ResponseWriter, status int, format string, a ...interface{})

and some code is calling it:

i.errorf(w, http.StatusNotFound, "not found", "%s not found", req.URL.Path)

This produces an error from vet: errorf call has arguments but no formatting directives.
The vet -printfuncs flag seems to have lost the argument position of the first argument involved in the print, and anyway it would be better not to have to individually configure printf-like functions to get Go code to compile.

@mibk

This comment has been minimized.

Contributor

mibk commented Nov 14, 2017

@rogpeppe Is the code calling it right? It seems to me that "not found" is used as the formatting parameter.

@rogpeppe

This comment has been minimized.

Contributor

rogpeppe commented Nov 15, 2017

@mibk Yes, the code is calling it right. The first "not found" is an error code, not the error description which what the format applies to.

@nathany

This comment has been minimized.

Contributor

nathany commented Nov 17, 2017

I've seen the same false positive as @rogpeppe, though it was some time ago. If I recall correctly, it was from the testify Errorf function: https://godoc.org/github.com/stretchr/testify/assert#Errorf. Vet seemed to be treating it as fmt.Errorf even though the package and signature are different.

func Errorf(t TestingT, err error, msg string, args ...interface{}) bool
@rogpeppe

This comment has been minimized.

Contributor

rogpeppe commented Nov 17, 2017

@nathany To do it right, I think the format checking needs to be type sensitive, but I'm guessing that would make vet unreasonably slow.

@gopherbot

This comment has been minimized.

gopherbot commented Nov 22, 2017

Change https://golang.org/cl/79398 mentions this issue: all: fix test breakage due to vet checks

@gopherbot

This comment has been minimized.

gopherbot commented Nov 29, 2017

Change https://golang.org/cl/80596 mentions this issue: all: few tweaks to vet fixes

gopherbot pushed a commit to golang/text that referenced this issue Dec 4, 2017

all: few tweaks to vet fixes
See golang/go#18085. And CL 80143.

Change-Id: I2ac1eaf1d64a451fbdb7a1e32ad3774b3dabd2f5
Reviewed-on: https://go-review.googlesource.com/80596
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Dec 8, 2017

On #23062 @calmh says

This check ["Println arg list ends with redundant newline"] is included in the new vet tests run in go test in Go 1.10 beta 1. In my opinion this is too far away from a 100% check, and mostly a style one at that. It fails tests on a binary that includes a

fmt.Println(`Usage:
    somecommand <args>

Description of the command.
More text here.
`)

and while it's correct (the string ends with a newline, and Println will add one more) this is the intended effect. You could argue that we should use cmd.Print instead and add the newline manually, and we could do that, but honestly I'm fine with it as is and think it should pass tests.

@calmh

This comment has been minimized.

Contributor

calmh commented Dec 8, 2017

Seeing the initial description ("It's okay if a "100% reliable" check flags a false positive as long as there is a clean way to rewrite the code to avoid the false positive") I guess that falls under "there is a clean way to rewrite the code".

But why be so particular about the newlines? I'm skeptical about the code quality increase here.

fmt.Println("hello") // this is ok
fmt.Println("hello\n") // this is a test failure
fmt.Println("hello\n\n") // this is ok again...?

It's a trivial issue to be sure, but the inconsistency is annoying now that this is a hard failure and you start to wonder if fmt.Print("hello") should be a test failure as well because probably the programmer meant fmt.Print("hello\n") instead...

I think this test should be removed from the "100% reliable" set, both for being not 100% reliable and also for not being useful. Newline discrepancies from print are visible in the output and can be corrected accordingly by the programmer.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 13, 2017

Newline discrepancies from print are visible in the output and can be corrected accordingly by the programmer.

Misused % formats are also visible in the output and can be corrected accordingly. We think they are useful because many prints are for debugging output that is not usually seen, so these bugs often linger unnoticed.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 13, 2017

Also, this claims seems false:

fmt.Println("hello") // this is ok
fmt.Println("hello\n") // this is a test failure
fmt.Println("hello\n\n") // this is ok again...?

Both of the last two are reported as incorrect.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 13, 2017

But why be so particular about the newlines?

If the only thing Println did was add a newline then I think it wouldn't matter much. But we made the (perhaps mistaken but locked in) decision that Println also applies a different heuristic to insert spaces around its arguments, so that fmt.Print("x", 1, "\n") might be converted to fmt.Println to get spaces around the 1, forgetting to drop the \n.

I am inclined to keep the \n check for now but I will also leave this open for a final decision later in the release cycle. /cc @robpike

@calmh

This comment has been minimized.

Contributor

calmh commented Dec 13, 2017

Also, this claims seems false:

I somehow made a mistake when testing this; not sure how, but you are of course right, it's consistent.

I still think it's a silly thing to make a build error and wish we would not do this, as failing the build because someone added a newline in a non approved manner is something I'd mock a language/compiler for, but I'll concede the point if the consensus is that enforcing this is somehow desirable.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 13, 2017

It's not failing the build - it's failing one test. There may be a tiny amount of initial pain here but in the steady state you only get hit by prints you just wrote.

@calmh

This comment has been minimized.

Contributor

calmh commented Dec 13, 2017

No, it's not failing one test. It fails before it starts running any tests at all. While I can still compile my code without tests, exiting with a failure status before any tests are run is a build failure in my book - logically one more serious than any of the tests themselves could be. Which seems totally disproportionate for this thing. Which is why it should not be done. But I feel that I'm not saying anything you don't already know and that this is a matter of opinion, so I'm exiting here.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 13, 2017

@nathany, @rogpeppe, others, the printf-check precision problem is #22936 and CLs are forthcoming.

klauspost added a commit to klauspost/compress that referenced this issue Dec 30, 2017

Fix "Println arg list ends with redundant newline"
Even though this is still being debated in golang/go#18085 we might as well change it.
@rsc

This comment has been minimized.

Contributor

rsc commented Jan 5, 2018

Going to close this as done for Go 1.10. Hopefully in future releases we will expand the set of checks enabled during tests, but that doesn't need to be tracked in this issue.

@rsc rsc closed this Jan 5, 2018

klauspost added a commit to klauspost/compress that referenced this issue Jan 10, 2018

Fix "Println arg list ends with redundant newline" (#83)
Even though this is still being debated in golang/go#18085 we might as well change it.

nvanbenschoten added a commit to nvanbenschoten/rksql that referenced this issue Mar 23, 2018

Minor fixes for go1.10 vet
go vet is a little stricter in go1.10. This fixes two new classes
of issues that it picks up:
- "Println arg list ends with redundant newline"
  (dicussed in golang/go#18085)
- "Errorf format %d arg res.RowsAffected is a func value, not called"
@gopherbot

This comment has been minimized.

gopherbot commented Apr 17, 2018

Change https://golang.org/cl/107077 mentions this issue: cmd/go: document which vet tests are enabled during go test

gopherbot pushed a commit that referenced this issue Apr 20, 2018

cmd/go: document which vet tests are enabled during go test
Update #18085
Fixes #24009

Change-Id: I655ad76284480002f8d49da269be92ded306128b
Reviewed-on: https://go-review.googlesource.com/107077
Reviewed-by: Rob Pike <r@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment