go/callgraph/cha: more precise resolution of unexported methods

Use (*go/types.Func).Id as the key for looking up which methods are
potential matches for a selector.

If a selector refers to an unexported method, it can only be a method
defined in the current package.  Previously the algorithm would
consider all methods of the same name to be potential matches.

SSA functions created by the user with (*ssa.Program).NewFunction may
not have a link to a go/types.Func, and so this would not work, but it
is unlikely that users of the cha library would also use NewFunction to
create a synthetic method on some type.

Fixes golang/go#66689

Change-Id: Ia7d4fcae56987f1d23fbefbf02251d76e5e951c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/574955
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Tim King <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: John Dethridge

go/callgraph/cha/cha.go

@@ -93,10 +93,14 @@ func lazyCallees(fns map[*ssa.Function]bool) func(site ssa.CallInstruction) []*s
 	// a dynamic call of a particular signature.
 	var funcsBySig typeutil.Map // value is []*ssa.Function
 
-	// 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)
+	// methodsByID contains all methods, grouped by ID for efficient
+	// lookup.
+	//
+	// We must key by ID, not name, for correct resolution of interface
+	// calls to a type with two (unexported) methods spelled the same but
+	// from different packages. The fact that the concrete type implements
+	// the interface does not mean the call dispatches to both methods.
+	methodsByID := make(map[string][]*ssa.Function)
 
 	// An imethod represents an interface method I.m.
 	// (There's no go/types object for it;
@@ -118,7 +122,7 @@ func lazyCallees(fns map[*ssa.Function]bool) func(site ssa.CallInstruction) []*s
 		id := m.Id()
 		methods, ok := methodsMemo[imethod{I, id}]
 		if !ok {
-			for _, f := range methodsByName[m.Name()] {
+			for _, f := range methodsByID[id] {
 				C := f.Signature.Recv().Type() // named or *named
 				if types.Implements(C, I) {
 					methods = append(methods, f)
@@ -138,8 +142,9 @@ func lazyCallees(fns map[*ssa.Function]bool) func(site ssa.CallInstruction) []*s
 			funcs, _ := funcsBySig.At(f.Signature).([]*ssa.Function)
 			funcs = append(funcs, f)
 			funcsBySig.Set(f.Signature, funcs)
-		} else {
-			methodsByName[f.Name()] = append(methodsByName[f.Name()], f)
+		} else if obj := f.Object(); obj != nil {
+			id := obj.(*types.Func).Id()
+			methodsByID[id] = append(methodsByID[id], f)
 		}
 	}
 

go/callgraph/cha/cha_test.go

@@ -13,6 +13,7 @@ import (
 	"bytes"
 	"fmt"
 	"go/ast"
+	"go/build"
 	"go/parser"
 	"go/token"
 	"go/types"
@@ -21,6 +22,7 @@ import (
 	"strings"
 	"testing"
 
+	"golang.org/x/tools/go/buildutil"
 	"golang.org/x/tools/go/callgraph"
 	"golang.org/x/tools/go/callgraph/cha"
 	"golang.org/x/tools/go/loader"
@@ -92,6 +94,75 @@ func TestCHAGenerics(t *testing.T) {
 	}
 }
 
+// TestCHAUnexported tests call resolution for unexported methods.
+func TestCHAUnexported(t *testing.T) {
+	// The two packages below each have types with methods called "m".
+	// Each of these methods should only be callable by functions in their
+	// own package, because they are unexported.
+	//
+	// In particular:
+	// - main.main can call    (main.S1).m
+	// - p2.Foo    can call    (p2.S2).m
+	// - main.main cannot call (p2.S2).m
+	// - p2.Foo    cannot call (main.S1).m
+	//
+	// We use CHA to build a callgraph, then check that it has the
+	// appropriate set of edges.
+
+	main := `package main
+		import "p2"
+		type I1 interface { m() }
+		type S1 struct { p2.I2 }
+		func (s S1) m() { }
+		func main() {
+			var s S1
+			var o I1 = s
+			o.m()
+			p2.Foo(s)
+		}`
+
+	p2 := `package p2
+		type I2 interface { m() }
+		type S2 struct { }
+		func (s S2) m() { }
+		func Foo(i I2) { i.m() }`
+
+	want := `All calls
+  main.init --> p2.init
+  main.main --> (main.S1).m
+  main.main --> p2.Foo
+  p2.Foo --> (p2.S2).m`
+
+	conf := loader.Config{
+		Build: fakeContext(map[string]string{"main": main, "p2": p2}),
+	}
+	conf.Import("main")
+	iprog, err := conf.Load()
+	if err != nil {
+		t.Fatalf("Load failed: %v", err)
+	}
+	prog := ssautil.CreateProgram(iprog, ssa.InstantiateGenerics)
+	prog.Build()
+
+	cg := cha.CallGraph(prog)
+
+	// The graph is easier to read without synthetic nodes.
+	cg.DeleteSyntheticNodes()
+
+	if got := printGraph(cg, nil, "", "All calls"); got != want {
+		t.Errorf("cha.CallGraph: got:\n%s\nwant:\n%s", got, want)
+	}
+}
+
+// Simplifying wrapper around buildutil.FakeContext for single-file packages.
+func fakeContext(pkgs map[string]string) *build.Context {
+	pkgs2 := make(map[string]map[string]string)
+	for path, content := range pkgs {
+		pkgs2[path] = map[string]string{"x.go": content}
+	}
+	return buildutil.FakeContext(pkgs2)
+}
+
 func loadProgInfo(filename string, mode ssa.BuilderMode) (*ssa.Program, *ast.File, *ssa.Package, error) {
 	content, err := os.ReadFile(filename)
 	if err != nil {