cmd/vet: -unusedresult: report calls of pure functions in expression statements

Change-Id: Ib8b6eaecd56ae15c44fb8ac11c781244964428f1
Reviewed-on: https://go-review.googlesource.com/6661
Reviewed-by: David Symonds <dsymonds@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

cmd/vet/doc.go

@@ -150,6 +150,15 @@ 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.
 
+Unused result of certain function calls
+
+Flag: -unusedresult
+
+Calls to well-known functions and methods that return a value that is
+discarded.  By default, this includes functions like fmt.Errorf and
+fmt.Sprintf and methods like String and Error. The flags -unusedfuncs
+and -unusedstringmethods control the set.
+
 Shifts
 
 Flag: -shift

cmd/vet/main.go

@@ -128,6 +128,7 @@ var (
 	binaryExpr    *ast.BinaryExpr
 	callExpr      *ast.CallExpr
 	compositeLit  *ast.CompositeLit
+	exprStmt      *ast.ExprStmt
 	field         *ast.Field
 	funcDecl      *ast.FuncDecl
 	funcLit       *ast.FuncLit
@@ -197,28 +198,8 @@ func main() {
 		}
 	}
 
-	if *printfuncs != "" {
+	initPrintFlags()
-		for _, name := range strings.Split(*printfuncs, ",") {
+	initUnusedFlags()
-			if len(name) == 0 {
-				flag.Usage()
-			}
-			skip := 0
-			if colon := strings.LastIndex(name, ":"); colon > 0 {
-				var err error
-				skip, err = strconv.Atoi(name[colon+1:])
-				if err != nil {
-					errorf(`illegal format for "Func:N" argument %q; %s`, name, err)
-				}
-				name = name[:colon]
-			}
-			name = strings.ToLower(name)
-			if name[len(name)-1] == 'f' {
-				printfList[name] = skip
-			} else {
-				printList[name] = skip
-			}
-		}
-	}
 
 	if flag.NArg() == 0 {
 		Usage()
@@ -294,6 +275,7 @@ type Package struct {
 	path      string
 	defs      map[*ast.Ident]types.Object
 	uses      map[*ast.Ident]types.Object
+	selectors map[*ast.SelectorExpr]*types.Selection
 	types     map[ast.Expr]types.TypeAndValue
 	spans     map[types.Object]Span
 	files     []*File
@@ -466,6 +448,8 @@ func (f *File) Visit(node ast.Node) ast.Visitor {
 		key = callExpr
 	case *ast.CompositeLit:
 		key = compositeLit
+	case *ast.ExprStmt:
+		key = exprStmt
 	case *ast.Field:
 		key = field
 	case *ast.FuncDecl:

cmd/vet/print.go

@@ -28,6 +28,32 @@ func init() {
 		funcDecl, callExpr)
 }
 
+func initPrintFlags() {
+	if *printfuncs == "" {
+		return
+	}
+	for _, name := range strings.Split(*printfuncs, ",") {
+		if len(name) == 0 {
+			flag.Usage()
+		}
+		skip := 0
+		if colon := strings.LastIndex(name, ":"); colon > 0 {
+			var err error
+			skip, err = strconv.Atoi(name[colon+1:])
+			if err != nil {
+				errorf(`illegal format for "Func:N" argument %q; %s`, name, err)
+			}
+			name = name[:colon]
+		}
+		name = strings.ToLower(name)
+		if name[len(name)-1] == 'f' {
+			printfList[name] = skip
+		} else {
+			printList[name] = skip
+		}
+	}
+}
+
 // printfList records the formatted-print functions. The value is the location
 // of the format parameter. Names are lower-cased so the lookup is
 // case insensitive.

cmd/vet/testdata/print.go

@@ -130,7 +130,7 @@ func PrintfTests() {
 	fmt.Println()                              // not an error
 	fmt.Println("%s", "hi")                    // ERROR "possible formatting directive in Println call"
 	fmt.Printf("%s", "hi", 3)                  // ERROR "wrong number of args for format in Printf call"
-	fmt.Sprintf("%"+("s"), "hi", 3)            // ERROR "wrong number of args for format in Sprintf call"
+	_ = fmt.Sprintf("%"+("s"), "hi", 3)        // ERROR "wrong number of args for format in Sprintf call"
 	fmt.Printf("%s%%%d", "hi", 3)              // correct
 	fmt.Printf("%08s", "woo")                  // correct
 	fmt.Printf("% 8s", "woo")                  // correct
@@ -172,7 +172,7 @@ func PrintfTests() {
 	Printf("%[xd", 3)                    // ERROR "illegal syntax for printf argument index"
 	Printf("%[x]d", 3)                   // ERROR "illegal syntax for printf argument index"
 	Printf("%[3]*s", "hi", 2)            // ERROR "missing argument for Printf.* reads arg 3, have only 2"
-	fmt.Sprintf("%[3]d", 2)              // ERROR "missing argument for Sprintf.* reads arg 3, have only 1"
+	_ = fmt.Sprintf("%[3]d", 2)          // ERROR "missing argument for Sprintf.* reads arg 3, have only 1"
 	Printf("%[2]*.[1]*[3]d", 2, "hi", 4) // ERROR "arg .hi. for \* in printf format not of type int"
 	Printf("%[0]s", "arg1")              // ERROR "index value \[0\] for Printf.*; indexes start at 1"
 	Printf("%[0]d", 1)                   // ERROR "index value \[0\] for Printf.*; indexes start at 1"
@@ -292,18 +292,18 @@ var percentSV percentSStruct
 type recursiveStringer int
 
 func (s recursiveStringer) String() string {
-	fmt.Sprintf("%d", s)
+	_ = fmt.Sprintf("%d", s)
-	fmt.Sprintf("%#v", s)
+	_ = fmt.Sprintf("%#v", s)
-	fmt.Sprintf("%v", s)   // ERROR "arg s for printf causes recursive call to String method"
+	_ = fmt.Sprintf("%v", s)  // ERROR "arg s for printf causes recursive call to String method"
-	fmt.Sprintf("%v", &s)  // ERROR "arg &s for printf causes recursive call to String method"
+	_ = fmt.Sprintf("%v", &s) // ERROR "arg &s for printf causes recursive call to String method"
-	fmt.Sprintf("%T", s)   // ok; does not recursively call String
+	_ = fmt.Sprintf("%T", s)  // ok; does not recursively call String
 	return fmt.Sprintln(s)    // ERROR "arg s for print causes recursive call to String method"
 }
 
 type recursivePtrStringer int
 
 func (p *recursivePtrStringer) String() string {
-	fmt.Sprintf("%v", *p)
+	_ = fmt.Sprintf("%v", *p)
 	return fmt.Sprintln(p) // ERROR "arg p for print causes recursive call to String method"
 }
 

cmd/vet/testdata/unused.go

@@ -0,0 +1,29 @@
+// 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"
+}

cmd/vet/types.go

@@ -52,6 +52,7 @@ func importType(path, name string) types.Type {
 func (pkg *Package) check(fs *token.FileSet, astFiles []*ast.File) error {
 	pkg.defs = make(map[*ast.Ident]types.Object)
 	pkg.uses = make(map[*ast.Ident]types.Object)
+	pkg.selectors = make(map[*ast.SelectorExpr]*types.Selection)
 	pkg.spans = make(map[types.Object]Span)
 	pkg.types = make(map[ast.Expr]types.TypeAndValue)
 	config := types.Config{
@@ -63,6 +64,7 @@ func (pkg *Package) check(fs *token.FileSet, astFiles []*ast.File) error {
 		Error: func(error) {},
 	}
 	info := &types.Info{
+		Selections: pkg.selectors,
 		Types:      pkg.types,
 		Defs:       pkg.defs,
 		Uses:       pkg.uses,

cmd/vet/unused.go

@@ -0,0 +1,95 @@
+// 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"
+	"strings"
+
+	"golang.org/x/tools/go/ast/astutil"
+	"golang.org/x/tools/go/types"
+)
+
+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, 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 := astutil.Unparen(n.(*ast.ExprStmt).X).(*ast.CallExpr)
+	if !ok {
+		return // not a call statement
+	}
+	fun := astutil.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)
+			}
+		}
+	}
+}