cmd/vet: enable fuzz checks in 'tests' analysis pass

[katiehockman]

There are several things that vet can do to support native fuzzing. This issue tracks all of the potential checks that could be added.

Vet should fail if...

• [ ] the inputs to any f.Add call don't match the ones in f.Fuzz
• [ ] there is any code after f.Fuzz
• [x] the f.Fuzz function is missing a *testing.T as its first parameter
• [x] there is any call to a *testing.F method from within the f.Fuzz function

[timothy-king]

Maybe a dumb question, but can "the f.Fuzz function is missing a *testing.T as its first parameter" be enforced by the typechecker/compiler instead?

[zpavlinovic]

There are several things that vet can do to support native fuzzing. This issue tracks all of the potential checks that could be added.

Vet should fail if...

• the inputs to any f.Add call don't match the ones in f.Fuzz

Could you perhaps elaborate on this one? Do you have a specific pattern in mind?

[katiehockman]

Maybe a dumb question, but can "the f.Fuzz function is missing a *testing.T as its first parameter" be enforced by the typechecker/compiler instead?

@timothy-king Not a dumb question! I don't have an answer to this question though. Possibly? The use of generics might make things easier here as well, but I haven't looked too deeply into this yet. However, in general, the more issues like this that we can detect at compile-time the better.

Could you perhaps elaborate on this one? Do you have a specific pattern in mind?

@zpavlinovic Yes, I'm thinking of something like this:

func FuzzFoo(f *testing.F) {
  f.Add("some string")
  f.Fuzz(func(*testing.T, int) { })
}

In the case above, f.Add is attempting to add a value to the corpus which is a single string. However, the f.Fuzz function accepts an int as its type. It would be nice if this can fail at compile time, but a vet check would be helpful if that's not feasible. That code should be corrected to something like this:

func FuzzFoo(f *testing.F) {
  f.Add(50)
  f.Fuzz(func(*testing.T, int) { })
}

or

func FuzzFoo(f *testing.F) {
  f.Add("some string")
  f.Fuzz(func(*testing.T, string) { })
}

[guodongli-google]

All these four checks can be covered by a simple vet checker. My main question is whether this checker mets the frequency requirement. After all, running a malformed target will result in panic, so there may be few malformed targets in the tested code.

I am more interested in the bugs that fail "silently", e.g. the expected mutations are not performed. Are there some patterns that are malformed but the related tests won't crash?

[jayconrod]

For anyone looking at this later, golang.org/x/tools/go/analysis/passes/tests is a vet analysis that detects problems with tests, examples, and benchmarks, for example, a missing *T parameter, or an // Output comment in the wrong place.

It's probably a good place to check fuzz tests here, rather than defining a new pass.

[findleyr]

@ansaba is going to look at this. Discussed with @timothy-king and we think it makes sense to start by adding this behavior to gopls alone, then migrate it to vet for 1.19, given that we're in the freeze.

We're not yet sure the best way to stage this; we could use a separate analyzer, or include it in the tests analyzer hidden behind a flag.

[ansaba]

Submitted review for some of the validations : https://go-review.googlesource.com/c/tools/+/374495

[gopherbot]

Change https://go.dev/cl/374495 mentions this issue: go/analysis/passes/tests: Check malformed fuzz target.

[findleyr]

The documentation here mentions that fuzz targets have no return values: https://go.dev/doc/fuzz/#glos-fuzz-target

However, that ~doesn't appear in the documentation for F.Fuzz, and~ doesn't seem to be enforced. Should we report a diagnostic for fuzz targets with result parameters?

(EDIT: it does appear in the docstring, I just missed it)

[findleyr]

Any objection to promoting this to a proposal, so that it can be decided upon for 1.19? We have already started implementing this for gopls, and it would just require a flag-flip to enable it for vet.

[rsc]

@findleyr, can you say exactly what the implemented check does? Also, there is already a vet 'tests' analysis module. It seems like probably the fuzz target checker should go in there instead of being a separate analysis?

Also @ianlancetaylor observes that the 'tests' analysis already does know about fuzz calls, although this appears to be undocumented in 'go tool vet help tests'. (#50198 seems to have been that support?)

[findleyr]

@rsc @ianlancetaylor vet does not yet know about Fuzz tests, because we guarded the new check behind an analysisinternal.DiagnoseFuzzTests bool. This lets us try out the new functionality in gopls without affecting vet. It is not documented because it does not yet apply to vet. We weighed various approached for adding this functionality to gopls without affecting vet, and this seemed easiest, with the expectation that this eventually belongs in x/tools/go/analysis/passes/tests anyway.

The additional checks currently implemented are (1) check for a well formed Fuzz test name, the same as for Test and Bench, and then (2) the checks documented by @ansaba here: https://go-review.googlesource.com/c/tools/+/374495/10/go/analysis/passes/tests/tests.go#86

// Check the arguments of f.Fuzz() calls :
// 1. f.Fuzz() should call a function and it should be of type (*testing.F).Fuzz().
// 2. The called function in f.Fuzz(func(){}) should not return result.
// 3. First argument of func() should be of type *testing.T
// 4. Second argument onwards should be of type []byte, string, bool, byte,
//    rune, float32, float64, int, int8, int16, int32, int64, uint, uint8, uint16,
//    uint32, uint64

We plan to additionally verify that the arguments to Add match arguments to Fuzz.

[rsc]

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

[rsc]

Thanks, retitled. This sounds fine, especially if you have experience with these working well in gopls.

[findleyr]

Well we don't have much experience yet, as most of our users are not writing fuzz tests. However, I believe the new diagnostics will be helpful, and have ~0 false positives, so they seem worth including in cmd/vet.

[rsc]

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

[gopherbot]

Change https://go.dev/cl/471295 mentions this issue: go/analysis/passes/tests: enable fuzz checks in 'tests' analysis pass for cmd/vet