From 2c624306892e9c0e84204c42a08978beb847b185 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Tue, 20 Feb 2018 19:33:11 -0500
Subject: [PATCH] go/callgraph/cha: fix bug computing correspondence of
 abstract/concrete methods

In a dynamic interface method call x.f() where x has type I, it is not sound
to assume that the receiver of the types.Signature for types.Func f has type I, as
Func objects for abstract methods may be shared by multiple interfaces.

Fixes golang/go#23925

Change-Id: I755e3010d1310480c46855e072946346626b3e59
Reviewed-on: https://go-review.googlesource.com/95697
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 go/callgraph/cha/cha.go                 | 30 +++++++++++++------
 go/callgraph/cha/cha_test.go            |  1 +
 go/callgraph/cha/testdata/issue23925.go | 38 +++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 8 deletions(-)
 create mode 100644 go/callgraph/cha/testdata/issue23925.go

diff --git a/go/callgraph/cha/cha.go b/go/callgraph/cha/cha.go
index 68ffffeb..215ff173 100644
--- a/go/callgraph/cha/cha.go
+++ b/go/callgraph/cha/cha.go
@@ -47,23 +47,36 @@ func CallGraph(prog *ssa.Program) *callgraph.Graph {
 
 	// methodsByName contains all methods,
 	// grouped by name for efficient lookup.
+	// (methodsById would be better but not every SSA method has a go/types ID.)
 	methodsByName := make(map[string][]*ssa.Function)
 
-	// methodsMemo records, for every abstract method call call I.f on
-	// interface type I, the set of concrete methods C.f of all
+	// An imethod represents an interface method I.m.
+	// (There's no go/types object for it;
+	// a *types.Func may be shared by many interfaces due to interface embedding.)
+	type imethod struct {
+		I  *types.Interface
+		id string
+	}
+	// methodsMemo records, for every abstract method call I.m on
+	// interface type I, the set of concrete methods C.m of all
 	// types C that satisfy interface I.
-	methodsMemo := make(map[*types.Func][]*ssa.Function)
-	lookupMethods := func(m *types.Func) []*ssa.Function {
-		methods, ok := methodsMemo[m]
+	//
+	// Abstract methods may be shared by several interfaces,
+	// hence we must pass I explicitly, not guess from m.
+	//
+	// methodsMemo is just a cache, so it needn't be a typeutil.Map.
+	methodsMemo := make(map[imethod][]*ssa.Function)
+	lookupMethods := func(I *types.Interface, m *types.Func) []*ssa.Function {
+		id := m.Id()
+		methods, ok := methodsMemo[imethod{I, id}]
 		if !ok {
-			I := m.Type().(*types.Signature).Recv().Type().Underlying().(*types.Interface)
 			for _, f := range methodsByName[m.Name()] {
 				C := f.Signature.Recv().Type() // named or *named
 				if types.Implements(C, I) {
 					methods = append(methods, f)
 				}
 			}
-			methodsMemo[m] = methods
+			methodsMemo[imethod{I, id}] = methods
 		}
 		return methods
 	}
@@ -109,7 +122,8 @@ func CallGraph(prog *ssa.Program) *callgraph.Graph {
 				if site, ok := instr.(ssa.CallInstruction); ok {
 					call := site.Common()
 					if call.IsInvoke() {
-						addEdges(fnode, site, lookupMethods(call.Method))
+						tiface := call.Value.Type().Underlying().(*types.Interface)
+						addEdges(fnode, site, lookupMethods(tiface, call.Method))
 					} else if g := call.StaticCallee(); g != nil {
 						addEdge(fnode, site, g)
 					} else if _, ok := call.Value.(*ssa.Builtin); !ok {
diff --git a/go/callgraph/cha/cha_test.go b/go/callgraph/cha/cha_test.go
index 7e56c063..cb2d5852 100644
--- a/go/callgraph/cha/cha_test.go
+++ b/go/callgraph/cha/cha_test.go
@@ -30,6 +30,7 @@ var inputs = []string{
 	"testdata/func.go",
 	"testdata/iface.go",
 	"testdata/recv.go",
+	"testdata/issue23925.go",
 }
 
 func expectation(f *ast.File) (string, token.Pos) {
diff --git a/go/callgraph/cha/testdata/issue23925.go b/go/callgraph/cha/testdata/issue23925.go
new file mode 100644
index 00000000..59eaf183
--- /dev/null
+++ b/go/callgraph/cha/testdata/issue23925.go
@@ -0,0 +1,38 @@
+package main
+
+// Regression test for https://github.com/golang/go/issues/23925
+
+type stringFlagImpl string
+
+func (*stringFlagImpl) Set(s string) error { return nil }
+
+type boolFlagImpl bool
+
+func (*boolFlagImpl) Set(s string) error { return nil }
+func (*boolFlagImpl) extra()             {}
+
+// A copy of flag.boolFlag interface, without a dependency.
+// Must appear first, so that it becomes the owner of the Set methods.
+type boolFlag interface {
+	flagValue
+	extra()
+}
+
+// A copy of flag.Value, without adding a dependency.
+type flagValue interface {
+	Set(string) error
+}
+
+func main() {
+	var x flagValue = new(stringFlagImpl)
+	x.Set("")
+
+	var y boolFlag = new(boolFlagImpl)
+	y.Set("")
+}
+
+// WANT:
+// Dynamic calls
+//   main --> (*boolFlagImpl).Set
+//   main --> (*boolFlagImpl).Set
+//   main --> (*stringFlagImpl).Set