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>
diff --git a/go/callgraph/cha/cha.go b/go/callgraph/cha/cha.go
index 68ffffe..215ff17 100644
--- a/go/callgraph/cha/cha.go
+++ b/go/callgraph/cha/cha.go
@@ -47,23 +47,36 @@
// 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 @@
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 7e56c06..cb2d585 100644
--- a/go/callgraph/cha/cha_test.go
+++ b/go/callgraph/cha/cha_test.go
@@ -30,6 +30,7 @@
"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 0000000..59eaf18
--- /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
