From c0b6badc83baaf67ef8db0adb9b2539c39f09ed6 Mon Sep 17 00:00:00 2001
From: David Symonds <dsymonds@golang.org>
Date: Tue, 13 Aug 2013 13:47:58 +1000
Subject: [PATCH] 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          | 26 ++++++++++++++++++++++++++
 cmd/vet/testdata/print.go |  4 ++++
 cmd/vet/types.go          | 25 ++++++++++++++++++++-----
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/cmd/vet/print.go b/cmd/vet/print.go
index 87cbce33..b8f8b0a9 100644
--- a/cmd/vet/print.go
+++ b/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.
diff --git a/cmd/vet/testdata/print.go b/cmd/vet/testdata/print.go
index fb4f4276..7fe26193 100644
--- a/cmd/vet/testdata/print.go
+++ b/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.
diff --git a/cmd/vet/types.go b/cmd/vet/types.go
index df93f02e..93ca9036 100644
--- a/cmd/vet/types.go
+++ b/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]
 }