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 <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: adonovan

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
 

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
 

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

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
 

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) }

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

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"
 }

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()
-}
+}

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())
 			}
 		}
 	})

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.
 

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 {