From 6d1dd1267464d856833cf617ac1a78df3ef48c22 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 27 Apr 2023 14:49:05 -0400 Subject: [PATCH] go/analysis: simplify unusedresult This change simplfies the unusedresult checker by using more modern library functions that also make it work in the presence of dot imports. It also adds a singlechecker command for it, cleans up various Doc constants, and avoids an allocation. Change-Id: I3af1844644356cbbeb2226b84063c8cdac487e00 Reviewed-on: https://go-review.googlesource.com/c/tools/+/492735 Run-TryBot: Alan Donovan TryBot-Result: Gopher Robot Reviewed-by: Robert Findley Auto-Submit: Alan Donovan --- go/analysis/passes/nilfunc/nilfunc.go | 4 - go/analysis/passes/timeformat/timeformat.go | 7 -- go/analysis/passes/unsafeptr/doc.go | 2 +- go/analysis/passes/unsafeptr/unsafeptr.go | 8 -- .../unusedresult/cmd/unusedresult/main.go | 14 ++++ go/analysis/passes/unusedresult/doc.go | 8 +- .../passes/unusedresult/testdata/src/a/a.go | 6 +- .../testdata/src/typeparams/typeparams.go | 8 +- .../passes/unusedresult/unusedresult.go | 76 ++++++++----------- gopls/doc/analyzers.md | 10 ++- gopls/internal/lsp/server_gen.go | 10 +-- gopls/internal/lsp/source/api_json.go | 8 +- 12 files changed, 75 insertions(+), 86 deletions(-) create mode 100644 go/analysis/passes/unusedresult/cmd/unusedresult/main.go diff --git a/go/analysis/passes/nilfunc/nilfunc.go b/go/analysis/passes/nilfunc/nilfunc.go index 51188507582..6df134399a3 100644 --- a/go/analysis/passes/nilfunc/nilfunc.go +++ b/go/analysis/passes/nilfunc/nilfunc.go @@ -19,10 +19,6 @@ import ( "golang.org/x/tools/internal/typeparams" ) -const Doc = `check for useless comparisons between functions and nil - -A useless comparison is one like f == nil as opposed to f() == nil.` - //go:embed doc.go var doc string diff --git a/go/analysis/passes/timeformat/timeformat.go b/go/analysis/passes/timeformat/timeformat.go index a1b5d2f9063..c45b9fa54bc 100644 --- a/go/analysis/passes/timeformat/timeformat.go +++ b/go/analysis/passes/timeformat/timeformat.go @@ -24,13 +24,6 @@ import ( const badFormat = "2006-02-01" const goodFormat = "2006-01-02" -const Doc = `check for calls of (time.Time).Format or time.Parse with 2006-02-01 - -The timeformat checker looks for time formats with the 2006-02-01 (yyyy-dd-mm) -format. Internationally, "yyyy-dd-mm" does not occur in common calendar date -standards, and so it is more likely that 2006-01-02 (yyyy-mm-dd) was intended. -` - //go:embed doc.go var doc string diff --git a/go/analysis/passes/unsafeptr/doc.go b/go/analysis/passes/unsafeptr/doc.go index 524fd04636b..de10804cb13 100644 --- a/go/analysis/passes/unsafeptr/doc.go +++ b/go/analysis/passes/unsafeptr/doc.go @@ -13,5 +13,5 @@ // to convert integers to pointers. A conversion from uintptr to // unsafe.Pointer is invalid if it implies that there is a uintptr-typed // word in memory that holds a pointer value, because that word will be -// invisible to stack copying and to the garbage collector.` +// invisible to stack copying and to the garbage collector. package unsafeptr diff --git a/go/analysis/passes/unsafeptr/unsafeptr.go b/go/analysis/passes/unsafeptr/unsafeptr.go index b1a32f2491a..e43ac20782e 100644 --- a/go/analysis/passes/unsafeptr/unsafeptr.go +++ b/go/analysis/passes/unsafeptr/unsafeptr.go @@ -18,14 +18,6 @@ import ( "golang.org/x/tools/go/ast/inspector" ) -const Doc = `check for invalid conversions of uintptr to unsafe.Pointer - -The unsafeptr analyzer reports likely incorrect uses of unsafe.Pointer -to convert integers to pointers. A conversion from uintptr to -unsafe.Pointer is invalid if it implies that there is a uintptr-typed -word in memory that holds a pointer value, because that word will be -invisible to stack copying and to the garbage collector.` - //go:embed doc.go var doc string diff --git a/go/analysis/passes/unusedresult/cmd/unusedresult/main.go b/go/analysis/passes/unusedresult/cmd/unusedresult/main.go new file mode 100644 index 00000000000..8116c6e06e9 --- /dev/null +++ b/go/analysis/passes/unusedresult/cmd/unusedresult/main.go @@ -0,0 +1,14 @@ +// Copyright 2023 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. + +// The unusedresult command applies the golang.org/x/tools/go/analysis/passes/unusedresult +// analysis to the specified packages of Go source code. +package main + +import ( + "golang.org/x/tools/go/analysis/passes/unusedresult" + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { singlechecker.Main(unusedresult.Analyzer) } diff --git a/go/analysis/passes/unusedresult/doc.go b/go/analysis/passes/unusedresult/doc.go index 0a713ca0d22..a1bf4cf9405 100644 --- a/go/analysis/passes/unusedresult/doc.go +++ b/go/analysis/passes/unusedresult/doc.go @@ -9,9 +9,11 @@ // // unusedresult: 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. +// Some functions like fmt.Errorf return a result and have no side +// effects, so it is always a mistake to discard the result. Other +// functions may return an error that must not be ignored, or a cleanup +// operation that must be called. This analyzer reports calls to +// functions like these when the result of the call is ignored. // // The set of functions may be controlled using flags. package unusedresult diff --git a/go/analysis/passes/unusedresult/testdata/src/a/a.go b/go/analysis/passes/unusedresult/testdata/src/a/a.go index 50b2f560fec..7a41f4a3601 100644 --- a/go/analysis/passes/unusedresult/testdata/src/a/a.go +++ b/go/analysis/passes/unusedresult/testdata/src/a/a.go @@ -8,6 +8,7 @@ import ( "bytes" "errors" "fmt" + . "fmt" ) func _() { @@ -20,8 +21,11 @@ func _() { 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` + 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" + + Sprint("") // want "result of fmt.Sprint call not used" + Sprintf("") // want "result of fmt.Sprintf call not used" } diff --git a/go/analysis/passes/unusedresult/testdata/src/typeparams/typeparams.go b/go/analysis/passes/unusedresult/testdata/src/typeparams/typeparams.go index c770ccd44e4..04d0e305842 100644 --- a/go/analysis/passes/unusedresult/testdata/src/typeparams/typeparams.go +++ b/go/analysis/passes/unusedresult/testdata/src/typeparams/typeparams.go @@ -23,7 +23,7 @@ func _[T any]() { 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` + 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" @@ -32,10 +32,10 @@ func _[T any]() { _ = userdefs.MustUse[int](2) s := userdefs.SingleTypeParam[int]{X: 1} - s.String() // want `result of \(typeparams/userdefs.SingleTypeParam\[int\]\).String call not used` + s.String() // want `result of \(\*typeparams/userdefs.SingleTypeParam\[int\]\).String call not used` _ = s.String() m := userdefs.MultiTypeParam[int, string]{X: 1, Y: "one"} - m.String() // want `result of \(typeparams/userdefs.MultiTypeParam\[int, string\]\).String call not used` + m.String() // want `result of \(\*typeparams/userdefs.MultiTypeParam\[int, string\]\).String call not used` _ = m.String() -} \ No newline at end of file +} diff --git a/go/analysis/passes/unusedresult/unusedresult.go b/go/analysis/passes/unusedresult/unusedresult.go index 051bc7c811d..cb487a21775 100644 --- a/go/analysis/passes/unusedresult/unusedresult.go +++ b/go/analysis/passes/unusedresult/unusedresult.go @@ -3,9 +3,16 @@ // license that can be found in the LICENSE file. // Package unusedresult defines an analyzer that checks for unused -// results of calls to certain pure functions. +// results of calls to certain functions. package unusedresult +// It is tempting to make this analysis inductive: for each function +// that tail-calls one of the functions that we check, check those +// functions too. However, just because you must use the result of +// fmt.Sprintf doesn't mean you need to use the result of every +// function that returns a formatted string: it may have other results +// and effects. + import ( _ "embed" "go/ast" @@ -18,17 +25,9 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/analysis/passes/internal/analysisutil" "golang.org/x/tools/go/ast/inspector" - "golang.org/x/tools/internal/typeparams" + "golang.org/x/tools/go/types/typeutil" ) -const 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.` - //go:embed doc.go var doc string @@ -56,15 +55,9 @@ func init() { // ignoringTheErrorWouldBeVeryBad() // oops // - // Also, it is tempting to make this analysis modular: one - // could export a "mustUseResult" fact for each function that - // tail-calls one of the functions that we check, and check - // those functions too. - // - // However, just because you must use the result of - // fmt.Sprintf doesn't mean you need to use the result of - // every function that returns a formatted string: - // it may have other results and effects. + // List standard library functions here. + // The context.With{Cancel,Deadline,Timeout} entries are + // effectively redundant wrt the lostcancel analyzer. funcs.Set("errors.New,fmt.Errorf,fmt.Sprintf,fmt.Sprint,sort.Reverse,context.WithValue,context.WithCancel,context.WithDeadline,context.WithTimeout") Analyzer.Flags.Var(&funcs, "funcs", "comma-separated list of functions whose results must be used") @@ -77,6 +70,14 @@ func init() { func run(pass *analysis.Pass) (interface{}, error) { inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + // Split functions into (pkg, name) pairs to save allocation later. + pkgFuncs := make(map[[2]string]bool, len(funcs)) + for s := range funcs { + if i := strings.LastIndexByte(s, '.'); i > 0 { + pkgFuncs[[2]string{s[:i], s[i+1:]}] = true + } + } + nodeFilter := []ast.Node{ (*ast.ExprStmt)(nil), } @@ -85,41 +86,26 @@ func run(pass *analysis.Pass) (interface{}, error) { if !ok { return // not a call statement } - fun := analysisutil.Unparen(call.Fun) - - if pass.TypesInfo.Types[fun].IsType() { - return // a conversion, not a call - } - x, _, _, _ := typeparams.UnpackIndexExpr(fun) - if x != nil { - fun = x // If this is generic function or method call, skip the instantiation arguments - } - - selector, ok := fun.(*ast.SelectorExpr) + // Call to function or method? + fn, ok := typeutil.Callee(pass.TypesInfo, call).(*types.Func) if !ok { - return // neither a method call nor a qualified ident + return // e.g. var or builtin } - sel, ok := pass.TypesInfo.Selections[selector] - if ok && sel.Kind() == types.MethodVal { + if sig := fn.Type().(*types.Signature); sig.Recv() != nil { // method (e.g. foo.String()) - obj := sel.Obj().(*types.Func) - sig := sel.Type().(*types.Signature) if types.Identical(sig, sigNoArgsStringResult) { - if stringMethods[obj.Name()] { + if stringMethods[fn.Name()] { pass.Reportf(call.Lparen, "result of (%s).%s call not used", - sig.Recv().Type(), obj.Name()) + sig.Recv().Type(), fn.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) - } + } else { + // package-level function (e.g. fmt.Errorf) + if pkgFuncs[[2]string{fn.Pkg().Path(), fn.Name()}] { + pass.Reportf(call.Lparen, "result of %s.%s call not used", + fn.Pkg().Path(), fn.Name()) } } }) diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index 3e7301bda7f..15eb2d957a8 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -584,7 +584,7 @@ The unsafeptr analyzer reports likely incorrect uses of unsafe.Pointer to convert integers to pointers. A conversion from uintptr to unsafe.Pointer is invalid if it implies that there is a uintptr-typed word in memory that holds a pointer value, because that word will be -invisible to stack copying and to the garbage collector.` +invisible to stack copying and to the garbage collector. **Enabled by default.** @@ -607,9 +607,11 @@ To reduce false positives it ignores: 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. +Some functions like fmt.Errorf return a result and have no side +effects, so it is always a mistake to discard the result. Other +functions may return an error that must not be ignored, or a cleanup +operation that must be called. This analyzer reports calls to +functions like these when the result of the call is ignored. The set of functions may be controlled using flags. diff --git a/gopls/internal/lsp/server_gen.go b/gopls/internal/lsp/server_gen.go index 3d736c6e5b0..33c70e29631 100644 --- a/gopls/internal/lsp/server_gen.go +++ b/gopls/internal/lsp/server_gen.go @@ -236,16 +236,16 @@ func (s *Server) SelectionRange(ctx context.Context, params *protocol.SelectionR return s.selectionRange(ctx, params) } -func (s *Server) SemanticTokensFull(ctx context.Context, p *protocol.SemanticTokensParams) (*protocol.SemanticTokens, error) { - return s.semanticTokensFull(ctx, p) +func (s *Server) SemanticTokensFull(ctx context.Context, params *protocol.SemanticTokensParams) (*protocol.SemanticTokens, error) { + return s.semanticTokensFull(ctx, params) } -func (s *Server) SemanticTokensFullDelta(ctx context.Context, p *protocol.SemanticTokensDeltaParams) (interface{}, error) { +func (s *Server) SemanticTokensFullDelta(context.Context, *protocol.SemanticTokensDeltaParams) (interface{}, error) { return nil, notImplemented("SemanticTokensFullDelta") } -func (s *Server) SemanticTokensRange(ctx context.Context, p *protocol.SemanticTokensRangeParams) (*protocol.SemanticTokens, error) { - return s.semanticTokensRange(ctx, p) +func (s *Server) SemanticTokensRange(ctx context.Context, params *protocol.SemanticTokensRangeParams) (*protocol.SemanticTokens, error) { + return s.semanticTokensRange(ctx, params) } func (s *Server) SetTrace(context.Context, *protocol.SetTraceParams) error { diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go index 2eb26d24934..cfe4c6e61e1 100644 --- a/gopls/internal/lsp/source/api_json.go +++ b/gopls/internal/lsp/source/api_json.go @@ -395,7 +395,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "\"unsafeptr\"", - Doc: "check for invalid conversions of uintptr to unsafe.Pointer\n\nThe unsafeptr analyzer reports likely incorrect uses of unsafe.Pointer\nto convert integers to pointers. A conversion from uintptr to\nunsafe.Pointer is invalid if it implies that there is a uintptr-typed\nword in memory that holds a pointer value, because that word will be\ninvisible to stack copying and to the garbage collector.`", + Doc: "check for invalid conversions of uintptr to unsafe.Pointer\n\nThe unsafeptr analyzer reports likely incorrect uses of unsafe.Pointer\nto convert integers to pointers. A conversion from uintptr to\nunsafe.Pointer is invalid if it implies that there is a uintptr-typed\nword in memory that holds a pointer value, because that word will be\ninvisible to stack copying and to the garbage collector.", Default: "true", }, { @@ -405,7 +405,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "\"unusedresult\"", - Doc: "check for unused results of calls to some functions\n\nSome functions like fmt.Errorf return a result and have no side effects,\nso it is always a mistake to discard the result. This analyzer reports\ncalls to certain functions in which the result of the call is ignored.\n\nThe set of functions may be controlled using flags.", + Doc: "check for unused results of calls to some functions\n\nSome functions like fmt.Errorf return a result and have no side\neffects, so it is always a mistake to discard the result. Other\nfunctions may return an error that must not be ignored, or a cleanup\noperation that must be called. This analyzer reports calls to\nfunctions like these when the result of the call is ignored.\n\nThe set of functions may be controlled using flags.", Default: "true", }, { @@ -1079,7 +1079,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "unsafeptr", - Doc: "check for invalid conversions of uintptr to unsafe.Pointer\n\nThe unsafeptr analyzer reports likely incorrect uses of unsafe.Pointer\nto convert integers to pointers. A conversion from uintptr to\nunsafe.Pointer is invalid if it implies that there is a uintptr-typed\nword in memory that holds a pointer value, because that word will be\ninvisible to stack copying and to the garbage collector.`", + Doc: "check for invalid conversions of uintptr to unsafe.Pointer\n\nThe unsafeptr analyzer reports likely incorrect uses of unsafe.Pointer\nto convert integers to pointers. A conversion from uintptr to\nunsafe.Pointer is invalid if it implies that there is a uintptr-typed\nword in memory that holds a pointer value, because that word will be\ninvisible to stack copying and to the garbage collector.", URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/unsafeptr", Default: true, }, @@ -1089,7 +1089,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "unusedresult", - Doc: "check for unused results of calls to some functions\n\nSome functions like fmt.Errorf return a result and have no side effects,\nso it is always a mistake to discard the result. This analyzer reports\ncalls to certain functions in which the result of the call is ignored.\n\nThe set of functions may be controlled using flags.", + Doc: "check for unused results of calls to some functions\n\nSome functions like fmt.Errorf return a result and have no side\neffects, so it is always a mistake to discard the result. Other\nfunctions may return an error that must not be ignored, or a cleanup\noperation that must be called. This analyzer reports calls to\nfunctions like these when the result of the call is ignored.\n\nThe set of functions may be controlled using flags.", URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/unusedresult", Default: true, },