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

cmd/vet: vet cannot report certain unused results with -unusedresult #14972

Open
lgarron opened this issue Mar 26, 2016 · 2 comments
Open

cmd/vet: vet cannot report certain unused results with -unusedresult #14972

lgarron opened this issue Mar 26, 2016 · 2 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis)
Milestone
Unplanned

Comments

@lgarron
Copy link

lgarron commented Mar 26, 2016

See https://github.com/lgarron/pizza for the reproduction code that goes with this report.

Unexpected behaviour of go vet -unusedresult

Issue Summary

There are 4 functions in pizza.go and 4 functions in string.go that go vet cannot report as an unused result as of 21cc49bd (March 25, 2016).

As an example, the functions in pizza.go all share the same implementation:

return Pizza{
  toppings: append(pizza.toppings, topping),
}

... although they have four different signatures:

func (pizza Pizza) addToppingMethod(topping string) Pizza
func (pizza Pizza) PublicAddToppingMethod(topping string) Pizza
func addToppingBareFunction(pizza Pizza, topping string) Pizza
func PublicAddToppingBareFunction(pizza Pizza, topping string) Pizza

It is not possible to check for unused results of these functions using go vet with the -unusedresult flag.

Based on the commit that introduced -unusedresult, it seems this was originally designed to catch "pure" functions. However, the documentation does not make this clear at all.

Also:

  • It's not obvious which function types -unusedresult can check for, which functions to pass to -unusedfuncs or -unusedstringmethods, and how to format and qualify them.
  • It's not possible to get go vet to output the default values for -unusedfuncs and -unusedstringmethods and the documentation is vague ("By default, this includes functions like fmt.Errorf and fmt.Sprintf and methods like String and Error."). In addition, there is no way to use the default values in addition to custom values in a single invocation, which makes it easy to miss out on vetting the default values by accident. The only way to do this seems to be to run go vet multiple times` with different arguments.

Reproduction Steps

# Get the code
go get github.com/lgarron/pizza
cd "$GOPATH/src/github.com/lgarron/pizza"

# Get `go vet`
go get golang.org/x/tools/cmd/vet

# Sanity check that the code compiles and runs.
go test -v

# Run `go vet` with default settings.
go tool vet -unusedresult *_test.go

# Run `go vet` using qualified *and* unqualified versions for each function.
go tool vet -unusedresult -unusedfuncs="makeStringMethod,PublicMakeStringMethod,makeStringBareFunction,PublicMakeStringBareFunction,addToppingMethod,PublicAddToppingMethod,addToppingBareFunction,PublicAddToppingBareFunction,pizza.Pizza.makeStringMethod,pizza.Pizza.PublicMakeStringMethod,pizza.makeStringBareFunction,pizza.PublicMakeStringBareFunction,pizza.Pizza.addToppingMethod,pizza.Pizza.PublicAddToppingMethod,pizza.addToppingBareFunction,pizza.PublicAddToppingBareFunction,errors.New,fmt.Errorf,fmt.Sprintf,fmt.Sprint,sort.Reverse" -unusedstringmethods="makeStringMethod,PublicMakeStringMethod,makeStringBareFunction,PublicMakeStringBareFunction,addToppingMethod,PublicAddToppingMethod,addToppingBareFunction,PublicAddToppingBareFunction,pizza.Pizza.makeStringMethod,pizza.Pizza.PublicMakeStringMethod,pizza.makeStringBareFunction,pizza.PublicMakeStringBareFunction,pizza.Pizza.addToppingMethod,pizza.Pizza.PublicAddToppingMethod,pizza.addToppingBareFunction,pizza.PublicAddToppingBareFunction,Error,String" *_test.go

Debugging using manual traces:

The relevant file int he go vet source is called unused.go. We will use a modified version of unused.go to show us what is happening.

# Check out a known-bad version of the source for `go vet`.
cd "$GOPATH/src/golang.org/x/tools"
git checkout 21cc49bd030cf5c6ebaca2fa0e3323628efed6d8
cd "$GOPATH/src/github.com/lgarron/pizza"

# Overwrite the relevant `go vet` code with an annotated version.
cp modified-vet-src/unused.go "$GOPATH/src/golang.org/x/tools/cmd/vet/unused.go"

# Build a new version of `vet` in the current project directory.
go build -a golang.org/x/tools/cmd/vet

# View output.
./vet -unusedresult *_test.go

Here is the output I get:

unusedFuncsFlag: errors.New,fmt.Errorf,fmt.Sprintf,fmt.Sprint,sort.Reverse
unusedStringMethodsFlag: Error,String

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[checking for unusedFuncs]
REPORT: qualified name `fmt.Sprintf` is in unusedFuncs

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[pkg.uses was not okay]

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[pkg.uses was not okay]

[start]
[checking for conversion]
[checking for (not method + unqualified)]
NO REPORT: not method + unqualified

[start]
[checking for conversion]
[checking for (not method + unqualified)]
NO REPORT: not method + unqualified

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[checking for unusedFuncs]
REPORT: qualified name `fmt.Sprintf` is in unusedFuncs

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[checking for unusedFuncs]
REPORT: qualified name `fmt.Sprintf` is in unusedFuncs

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[pkg.uses was not okay]

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[pkg.uses was not okay]

[start]
[checking for conversion]
[checking for (not method + unqualified)]
NO REPORT: not method + unqualified

[start]
[checking for conversion]
[checking for (not method + unqualified)]
NO REPORT: not method + unqualified

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[checking for unusedFuncs]
REPORT: qualified name `fmt.Sprintf` is in unusedFuncs

[start]
[checking for conversion]
[checking for (not method + unqualified)]
[checking pkg.selectors]
[pkg.selectors not okay]
[checking pkg.uses]
[checking for unusedFuncs]
NO REPORT BUT REPORTABLE: qualified name `fmt.Printf` is not in unusedFuncs

Suggestions

  • 1a) Remove some of the restrictions and assumptions in the source to allow reporting these functions as unused, or 1b) explicitly document the limited behaviour.
    1. Document which functions can be passed in, with clear examples of how to format them.

I tried 1a), but I don't know the code well enough to contribute a fix yet.
@lgarron lgarron mentioned this issue Mar 26, 2016
Closed
@bradfitz bradfitz changed the title go vet cannot report certain unused results with -unusedresult cmd/vet: vet cannot report certain unused results with -unusedresult Apr 9, 2016
@bradfitz bradfitz changed the title cmd/vet: vet cannot report certain unused results with -unusedresult cmd/vet: vet cannot report certain unused results with -unusedresult Apr 9, 2016
@bradfitz bradfitz added this to the Unplanned milestone Apr 9, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Apr 9, 2016

/cc @adg @robpike

@bubaflub
Copy link

I would like to relax the restrictions on go vet -unusedresult -unusedfuncs=... so I can pass in any function regardless of function signature.

I hit an analogous issue when using *net/http.Request.WithContext(). I initially had erroneously assumed WithContext() modified the context in-place rather than returning a new copy. When we found this bug later I was able to find another instance of this in our code with some grep-ing. I tried to use go vet -unusedresult -unusedfuncs=... but the limitations on what function signatures we can check prevented that. I wrote a small single-use program at bubaflub/go-withcontext-checker and filed an issue in go-staticcheck to catch this issue which was fixed.

I think we should relax the restrictions on function signatures in go vet -unusedresult -unusedfuncs=... because I think this would catch a class of defects without requiring something similar to GCC's warn_unused_result .

Furthermore, I was also considering if go vet itself could generate a list of pure functions rather than requiring a whitelist. This might be far beyond the scope of this issue and I'd be happy to create a separate proposal to discuss that.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
@1f604 1f604 mentioned this issue Oct 23, 2023
Open
@janpfeifer janpfeifer mentioned this issue Feb 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis)
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@bradfitz @bubaflub @lgarron @adonovan @alandonovan