From f62bfb5415380414cb751ad4556504f5eac6179d Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Wed, 14 Nov 2018 21:31:18 -0500
Subject: [PATCH] go/analysis/passes/printf: fix regression in "recursive
 stringer" logic

The recursive stringer check should report cases such as

   func (x T) String() string { return fmt.Sprint(x) }

in which the receiver x (or possibly &x) was passed into a fmt print call.
However, in translating it from the go/ast to the go/types representation,
I inadvertently made it report any situation in which a value of type T
was passed to fmt, even when the value is not x, as in:

   func (cons *cons) String() string {
	... fmt.Sprint(cons.cdr) ...
   }

Fixed and tested.

Change-Id: I57e88755c9989deaaad45cc306a604f3db4ee269
Reviewed-on: https://go-review.googlesource.com/c/149616
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
---
 go/analysis/passes/printf/printf.go           | 17 ++++++++++++++++-
 go/analysis/passes/printf/testdata/src/a/a.go | 13 +++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/go/analysis/passes/printf/printf.go b/go/analysis/passes/printf/printf.go
index 80db95c8386..9fa0a1c603c 100644
--- a/go/analysis/passes/printf/printf.go
+++ b/go/analysis/passes/printf/printf.go
@@ -843,7 +843,22 @@ func recursiveStringer(pass *analysis.Pass, e ast.Expr) bool {
 	}
 
 	// Is the expression e within the body of that String method?
-	return stringMethod.Pkg() == pass.Pkg && stringMethod.Scope().Contains(e.Pos())
+	if stringMethod.Pkg() != pass.Pkg || !stringMethod.Scope().Contains(e.Pos()) {
+		return false
+	}
+
+	// Is it the receiver r, or &r?
+	recv := stringMethod.Type().(*types.Signature).Recv()
+	if recv == nil {
+		return false
+	}
+	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 false
 }
 
 // isFunctionValue reports whether the expression is a function as opposed to a function call.
diff --git a/go/analysis/passes/printf/testdata/src/a/a.go b/go/analysis/passes/printf/testdata/src/a/a.go
index 603179bccb0..970b5122ec8 100644
--- a/go/analysis/passes/printf/testdata/src/a/a.go
+++ b/go/analysis/passes/printf/testdata/src/a/a.go
@@ -514,6 +514,19 @@ func (p *recursivePtrStringer) String() string {
 	return fmt.Sprintln(p) // want "Sprintln arg p causes recursive call to String method"
 }
 
+type cons struct {
+	car int
+	cdr *cons
+}
+
+func (cons *cons) String() string {
+	if cons == nil {
+		return "nil"
+	}
+	_ = fmt.Sprint(cons.cdr)                            // don't want "recursive call" diagnostic
+	return fmt.Sprintf("(%d . %v)", cons.car, cons.cdr) // don't want "recursive call" diagnostic
+}
+
 type BoolFormatter bool
 
 func (*BoolFormatter) Format(fmt.State, rune) {