cmd/vet: flag redundant invocations of String and Error methods in printf calls.

R=r
CC=golang-dev
https://golang.org/cl/12811043

cmd/vet/print.go

@@ -385,9 +385,35 @@ func (f *File) okPrintfArg(call *ast.CallExpr, state *formatState) (ok bool) {
 		f.Badf(call.Pos(), "arg %s for printf verb %%%c of wrong type: %s", f.gofmt(arg), state.verb, typeString)
 		return false
 	}
+	// Check %v, %s, %q, %x, %X for Error/String calls.
+	switch state.verb {
+	case 'v', 's', 'q', 'x', 'X':
+		f.checkIfArgIsRedundant(arg)
+	}
 	return true
 }
 
+// checkIfArgIsRedundant reports whether arg, assumed to be the argument of
+// a string-like format verb, ends in a redundant method invocation.
+func (f *File) checkIfArgIsRedundant(arg ast.Expr) {
+	ce, ok := arg.(*ast.CallExpr)
+	if !ok || len(ce.Args) > 0 {
+		return
+	}
+	sel, ok := ce.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return
+	}
+	n := sel.Sel.Name
+	if n != "String" && n != "Error" {
+		return
+	}
+	if !f.returnsString(sel) {
+		return
+	}
+	f.Warnf(arg.Pos(), "redundant invocation of %s method of %s", n, f.gofmt(sel.X))
+}
+
 // argCanBeChecked reports whether the specified argument is statically present;
 // it may be beyond the list of arguments or in a terminal slice... argument, which
 // means we can't see it.

cmd/vet/testdata/print.go

@@ -7,6 +7,7 @@
 package testdata
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"unsafe" // just for test case printing unsafe.Pointer
@@ -188,6 +189,9 @@ func PrintfTests() {
 	et4.Error() // ok, not an error method.
 	var et5 errorTest5
 	et5.error() // ok, not an error method.
+	// Tests of redundant String and Error method invocations.
+	Printf("%s", stringerv.String())         // ERROR "redundant invocation of String method of stringerv"
+	Printf("%v", errors.New("whee").Error()) // ERROR "redundant invocation of Error method of errors.New..whee.."
 }
 
 // Printf is used by the test so we must declare it.

cmd/vet/types.go

@@ -273,11 +273,26 @@ func (f *File) isErrorMethodCall(call *ast.CallExpr) bool {
 	if sig.Params().Len() > 0 {
 		return false
 	}
-	// Finally the real questions.
-	// There must be one result.
-	if sig.Results().Len() != 1 {
+	// Finally the real question.
+	return hasStringReturn(sig)
+}
+
+// returnsString reports whether expr is a call of a function that returns a string.
+// If it cannot be type checked it returns true.
+func (f *File) returnsString(expr ast.Expr) bool {
+	typ := f.pkg.types[expr]
+	if typ == nil {
+		return true
+	}
+	sig, ok := typ.(*types.Signature)
+	if !ok {
 		return false
 	}
-	// It must have return type "string" from the universe.
-	return sig.Results().At(0).Type() == types.Typ[types.String]
+	return hasStringReturn(sig)
+}
+
+// hasStringReturn reports whether the function signature has exactly
+// one return value, of universe type string.
+func hasStringReturn(sig *types.Signature) bool {
+	return sig.Results().Len() == 1 && sig.Results().At(0).Type() == types.Typ[types.String]
 }