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

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

Open
2 of 4 tasks
katiehockman opened this issue May 18, 2021 · 18 comments
Open
2 of 4 tasks

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

katiehockman opened this issue May 18, 2021 · 18 comments
Assignees
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Milestone

Comments

@katiehockman
Copy link
Contributor

katiehockman commented May 18, 2021

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
  • the f.Fuzz function is missing a *testing.T as its first parameter
  • there is any call to a *testing.F method from within the f.Fuzz function
@katiehockman katiehockman added the fuzz Issues related to native fuzzing support label May 18, 2021
@katiehockman katiehockman added this to the Backlog milestone May 18, 2021
@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 18, 2021
@timothy-king
Copy link
Contributor

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 zpavlinovic added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label May 25, 2021
@zpavlinovic
Copy link
Contributor

zpavlinovic commented May 25, 2021

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

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

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?

@rsc rsc changed the title [dev.fuzz] cmd/vet: checks for malformed fuzz targets cmd/vet: checks for malformed fuzz targets Sep 21, 2021
@jayconrod
Copy link
Contributor

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

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

ansaba commented Jan 3, 2022

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

@gopherbot
Copy link

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

@findleyr
Copy link
Contributor

findleyr commented Feb 15, 2022

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

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.

@findleyr findleyr changed the title cmd/vet: checks for malformed fuzz targets proposal: cmd/vet: checks for malformed fuzz targets Mar 2, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Mar 2, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Mar 2, 2022
@rsc
Copy link
Contributor

rsc commented Mar 2, 2022

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

findleyr commented Mar 2, 2022

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

rsc commented Mar 2, 2022

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 rsc moved this from Incoming to Active in Proposals (old) Mar 2, 2022
@rsc rsc changed the title proposal: cmd/vet: checks for malformed fuzz targets proposal: cmd/vet: enable fuzz checks in 'tests' analysis pass Mar 9, 2022
@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

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

@findleyr
Copy link
Contributor

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

rsc commented Mar 16, 2022

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

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Mar 16, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Mar 23, 2022
@rsc
Copy link
Contributor

rsc commented Mar 23, 2022

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

@rsc rsc changed the title proposal: cmd/vet: enable fuzz checks in 'tests' analysis pass cmd/vet: enable fuzz checks in 'tests' analysis pass Mar 23, 2022
@rsc rsc modified the milestones: Proposal, Backlog Mar 23, 2022
@gopherbot
Copy link

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

gopherbot pushed a commit to golang/tools that referenced this issue Mar 30, 2023
… for cmd/vet

This will remove the flag analysisinternal.DiagnoseFuzzTests created during golang/go#50198.Malformed fuzz target check will be enabled for cmd/vet.

For golang/go#46218

Change-Id: I5cc8d685a57060f8dd84c1957f0d296a6205ddb6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/471295
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Nooras Saba‎ <saba@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Projects
Status: No status
Status: Accepted
Development

No branches or pull requests

10 participants