From 36563e24a2627da92566d43aa1c7a2dd895fc60d Mon Sep 17 00:00:00 2001
From: Reilly Watson <reillywatson@gmail.com>
Date: Wed, 27 Feb 2019 14:53:29 -0500
Subject: [PATCH] cmd/vet: verify potentially-recursive Stringers are actually
 Stringers

The printf recursiveStringer check was checking for a function called String(),
but wasn't checking that it matched the actual function signature of Stringer.

Fixes golang/go#30441

Change-Id: I09d5fba035bb717036f7edf57efc63e2e3fe51d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/164217
Reviewed-by: Alan Donovan <adonovan@google.com>
---
 go/analysis/passes/printf/printf.go           | 16 ++++++++++++----
 go/analysis/passes/printf/testdata/src/a/a.go | 14 ++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go
index d4697eac..f59e95dc 100644
--- a/go/analysis/passes/printf/printf.go
+++ b/go/analysis/passes/printf/printf.go
@@ -856,20 +856,28 @@ func recursiveStringer(pass *analysis.Pass, e ast.Expr) bool {
 		return false
 	}
 
-	// Is it the receiver r, or &r?
-	recv := stringMethod.Type().(*types.Signature).Recv()
-	if recv == nil {
+	sig := stringMethod.Type().(*types.Signature)
+	if !isStringer(sig) {
 		return false
 	}
+
+	// Is it the receiver r, or &r?
 	if u, ok := e.(*ast.UnaryExpr); ok && u.Op == token.AND {
 		e = u.X // strip off & from &r
 	}
 	if id, ok := e.(*ast.Ident); ok {
-		return pass.TypesInfo.Uses[id] == recv
+		return pass.TypesInfo.Uses[id] == sig.Recv()
 	}
 	return false
 }
 
+// isStringer reports whether the method signature matches the String() definition in fmt.Stringer.
+func isStringer(sig *types.Signature) bool {
+	return sig.Params().Len() == 0 &&
+		sig.Results().Len() == 1 &&
+		sig.Results().At(0).Type() == types.Typ[types.String]
+}
+
 // isFunctionValue reports whether the expression is a function as opposed to a function call.
 // It is almost always a mistake to print a function value.
 func isFunctionValue(pass *analysis.Pass, e ast.Expr) bool {
diff --git a/go/analysis/passes/printf/testdata/src/a/a.go b/go/analysis/passes/printf/testdata/src/a/a.go
index 4aeecd56..b783f10b 100644
--- a/go/analysis/passes/printf/testdata/src/a/a.go
+++ b/go/analysis/passes/printf/testdata/src/a/a.go
@@ -516,6 +516,20 @@ func (p *recursivePtrStringer) String() string {
 	return fmt.Sprintln(p) // want "Sprintln arg p causes recursive call to String method"
 }
 
+// implements a String() method but with non-matching return types
+type nonStringerWrongReturn int
+
+func (s nonStringerWrongReturn) String() (string, error) {
+	return "", fmt.Errorf("%v", s)
+}
+
+// implements a String() method but with non-matching arguments
+type nonStringerWrongArgs int
+
+func (s nonStringerWrongArgs) String(i int) string {
+	return fmt.Sprintf("%d%v", i, s)
+}
+
 type cons struct {
 	car int
 	cdr *cons