From 088df9ca28c8cc0b3b115e80d4205ca29d174db1 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 9 Oct 2018 10:45:32 -0400 Subject: [PATCH] go/analysis/passes/unusedresult: split out of vet Change-Id: I4bdba827ab0a518d62dda85cfc66875a54aeda24 Reviewed-on: https://go-review.googlesource.com/c/140760 Reviewed-by: Michael Matloob Run-TryBot: Michael Matloob --- .../passes/unusedresult/testdata/src/a/a.go | 27 ++++ .../passes/unusedresult/unusedresult.go | 129 ++++++++++++++++++ .../passes/unusedresult/unusedresult_test.go | 13 ++ go/analysis/passes/vet/testdata/unused.go | 29 ---- go/analysis/passes/vet/unused.go | 95 ------------- 5 files changed, 169 insertions(+), 124 deletions(-) create mode 100644 go/analysis/passes/unusedresult/testdata/src/a/a.go create mode 100644 go/analysis/passes/unusedresult/unusedresult.go create mode 100644 go/analysis/passes/unusedresult/unusedresult_test.go delete mode 100644 go/analysis/passes/vet/testdata/unused.go delete mode 100644 go/analysis/passes/vet/unused.go diff --git a/go/analysis/passes/unusedresult/testdata/src/a/a.go b/go/analysis/passes/unusedresult/testdata/src/a/a.go new file mode 100644 index 00000000..50b2f560 --- /dev/null +++ b/go/analysis/passes/unusedresult/testdata/src/a/a.go @@ -0,0 +1,27 @@ +// Copyright 2015 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package a + +import ( + "bytes" + "errors" + "fmt" +) + +func _() { + fmt.Errorf("") // want "result of fmt.Errorf call not used" + _ = fmt.Errorf("") + + errors.New("") // want "result of errors.New call not used" + + err := errors.New("") + err.Error() // want `result of \(error\).Error call not used` + + var buf bytes.Buffer + buf.String() // want `result of \(bytes.Buffer\).String call not used` + + fmt.Sprint("") // want "result of fmt.Sprint call not used" + fmt.Sprintf("") // want "result of fmt.Sprintf call not used" +} diff --git a/go/analysis/passes/unusedresult/unusedresult.go b/go/analysis/passes/unusedresult/unusedresult.go new file mode 100644 index 00000000..3d969a4d --- /dev/null +++ b/go/analysis/passes/unusedresult/unusedresult.go @@ -0,0 +1,129 @@ +// Copyright 2015 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package unusedresult defines an analyer that checks for unused +// results of calls to certain pure functions. +package unusedresult + +import ( + "go/ast" + "go/token" + "go/types" + "sort" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/analysis/passes/internal/analysisutil" + "golang.org/x/tools/go/ast/inspector" +) + +// TODO(adonovan): make this analysis modular: export a mustUseResult +// fact for each function that tail-calls one of the functions that we +// check, and check those functions too. + +var Analyzer = &analysis.Analyzer{ + Name: "unusedresult", + Doc: `check for unused results of calls to some functions + +Some functions like fmt.Errorf return a result and have no side effects, +so it is always a mistake to discard the result. This analyzer reports +calls to certain functions in which the result of the call is ignored. + +The set of functions may be controlled using flags.`, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +// flags +var funcs, stringMethods stringSetFlag + +func init() { + // TODO(adonovan): provide a comment syntax to allow users to + // add their functions to this set using facts. + funcs.Set("errors.New,fmt.Errorf,fmt.Sprintf,fmt.Sprint,sort.Reverse") + Analyzer.Flags.Var(&funcs, "funcs", + "comma-separated list of functions whose results must be used") + + stringMethods.Set("Error,String") + Analyzer.Flags.Var(&stringMethods, "stringmethods", + "comma-separated list of names of methods of type func() string whose results must be used") +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.ExprStmt)(nil), + } + inspect.Preorder(nodeFilter, func(n ast.Node) { + call, ok := analysisutil.Unparen(n.(*ast.ExprStmt).X).(*ast.CallExpr) + if !ok { + return // not a call statement + } + fun := analysisutil.Unparen(call.Fun) + + if pass.TypesInfo.Types[fun].IsType() { + return // a conversion, not a call + } + + selector, ok := fun.(*ast.SelectorExpr) + if !ok { + return // neither a method call nor a qualified ident + } + + sel, ok := pass.TypesInfo.Selections[selector] + if ok && sel.Kind() == types.MethodVal { + // method (e.g. foo.String()) + obj := sel.Obj().(*types.Func) + sig := sel.Type().(*types.Signature) + if types.Identical(sig, sigNoArgsStringResult) { + if stringMethods[obj.Name()] { + pass.Reportf(call.Lparen, "result of (%s).%s call not used", + sig.Recv().Type(), obj.Name()) + } + } + } else if !ok { + // package-qualified function (e.g. fmt.Errorf) + obj := pass.TypesInfo.Uses[selector.Sel] + if obj, ok := obj.(*types.Func); ok { + qname := obj.Pkg().Path() + "." + obj.Name() + if funcs[qname] { + pass.Reportf(call.Lparen, "result of %v call not used", qname) + } + } + } + }) + return nil, nil +} + +// func() string +var sigNoArgsStringResult = types.NewSignature(nil, nil, + types.NewTuple(types.NewVar(token.NoPos, nil, "", types.Typ[types.String])), + false) + +type stringSetFlag map[string]bool + +func (ss *stringSetFlag) String() string { + var items []string + for item := range *ss { + items = append(items, item) + } + sort.Strings(items) + return strings.Join(items, ",") +} + +func (ss *stringSetFlag) Set(s string) error { + m := make(map[string]bool) // clobber previous value + if s != "" { + for _, name := range strings.Split(s, ",") { + if name == "" { + continue // TODO: report error? proceed? + } + m[name] = true + } + } + *ss = m + return nil +} diff --git a/go/analysis/passes/unusedresult/unusedresult_test.go b/go/analysis/passes/unusedresult/unusedresult_test.go new file mode 100644 index 00000000..65f22f3e --- /dev/null +++ b/go/analysis/passes/unusedresult/unusedresult_test.go @@ -0,0 +1,13 @@ +package unusedresult_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/unusedresult" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, unusedresult.Analyzer, "a") +} diff --git a/go/analysis/passes/vet/testdata/unused.go b/go/analysis/passes/vet/testdata/unused.go deleted file mode 100644 index d50f6594..00000000 --- a/go/analysis/passes/vet/testdata/unused.go +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2015 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// This file contains tests for the unusedresult checker. - -package testdata - -import ( - "bytes" - "errors" - "fmt" -) - -func _() { - fmt.Errorf("") // ERROR "result of fmt.Errorf call not used" - _ = fmt.Errorf("") - - errors.New("") // ERROR "result of errors.New call not used" - - err := errors.New("") - err.Error() // ERROR "result of \(error\).Error call not used" - - var buf bytes.Buffer - buf.String() // ERROR "result of \(bytes.Buffer\).String call not used" - - fmt.Sprint("") // ERROR "result of fmt.Sprint call not used" - fmt.Sprintf("") // ERROR "result of fmt.Sprintf call not used" -} diff --git a/go/analysis/passes/vet/unused.go b/go/analysis/passes/vet/unused.go deleted file mode 100644 index 913af9c3..00000000 --- a/go/analysis/passes/vet/unused.go +++ /dev/null @@ -1,95 +0,0 @@ -// +build ignore - -// Copyright 2015 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// This file defines the check for unused results of calls to certain -// pure functions. - -package main - -import ( - "flag" - "go/ast" - "go/token" - "go/types" - "strings" -) - -var unusedFuncsFlag = flag.String("unusedfuncs", - "errors.New,fmt.Errorf,fmt.Sprintf,fmt.Sprint,sort.Reverse", - "comma-separated list of functions whose results must be used") - -var unusedStringMethodsFlag = flag.String("unusedstringmethods", - "Error,String", - "comma-separated list of names of methods of type func() string whose results must be used") - -func init() { - register("unusedresult", - "check for unused result of calls to functions in -unusedfuncs list and methods in -unusedstringmethods list", - checkUnusedResult, - exprStmt) -} - -// func() string -var sigNoArgsStringResult = types.NewSignature(nil, nil, - types.NewTuple(types.NewVar(token.NoPos, nil, "", types.Typ[types.String])), - false) - -var unusedFuncs = make(map[string]bool) -var unusedStringMethods = make(map[string]bool) - -func initUnusedFlags() { - commaSplit := func(s string, m map[string]bool) { - if s != "" { - for _, name := range strings.Split(s, ",") { - if len(name) == 0 { - flag.Usage() - } - m[name] = true - } - } - } - commaSplit(*unusedFuncsFlag, unusedFuncs) - commaSplit(*unusedStringMethodsFlag, unusedStringMethods) -} - -func checkUnusedResult(f *File, n ast.Node) { - call, ok := unparen(n.(*ast.ExprStmt).X).(*ast.CallExpr) - if !ok { - return // not a call statement - } - fun := unparen(call.Fun) - - if f.pkg.types[fun].IsType() { - return // a conversion, not a call - } - - selector, ok := fun.(*ast.SelectorExpr) - if !ok { - return // neither a method call nor a qualified ident - } - - sel, ok := f.pkg.selectors[selector] - if ok && sel.Kind() == types.MethodVal { - // method (e.g. foo.String()) - obj := sel.Obj().(*types.Func) - sig := sel.Type().(*types.Signature) - if types.Identical(sig, sigNoArgsStringResult) { - if unusedStringMethods[obj.Name()] { - f.Badf(call.Lparen, "result of (%s).%s call not used", - sig.Recv().Type(), obj.Name()) - } - } - } else if !ok { - // package-qualified function (e.g. fmt.Errorf) - obj := f.pkg.uses[selector.Sel] - if obj, ok := obj.(*types.Func); ok { - qname := obj.Pkg().Path() + "." + obj.Name() - if unusedFuncs[qname] { - f.Badf(call.Lparen, "result of %v call not used", qname) - } - } - } -}