go/types/objectpath: canonical order for methods

The method ordering in types can depend on the parse order for types. In
order to make objectpath robust against this, simply encode all methods
with respect to a canonical lexicographical ordering.

Fixes golang/go#44195

Change-Id: I4177d9b4e094452f71d4db1813a5a36b54d0d70a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/354433
Run-TryBot: Tim King <taking@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>

go/types/objectpath/objectpath.go

@@ -23,11 +23,11 @@ package objectpath
 
 import (
 	"fmt"
+	"go/types"
+	"sort"
 	"strconv"
 	"strings"
 
-	"go/types"
-
 	"golang.org/x/tools/internal/typeparams"
 )
 
@@ -67,6 +67,8 @@ type Path string
 //   which is encoded as a string of decimal digits.
 //   These indices are stable across different representations
 //   of the same package, even source and export data.
+//   The indices used are implementation specific and may not correspond to
+//   the argument to the go/types function.
 //
 // In the example below,
 //
@@ -287,8 +289,12 @@ func For(obj types.Object) (Path, error) {
 		// Inspect declared methods of defined types.
 		if T, ok := o.Type().(*types.Named); ok {
 			path = append(path, opType)
-			for i := 0; i < T.NumMethods(); i++ {
-				m := T.Method(i)
+			// Note that method index here is always with respect
+			// to canonical ordering of methods, regardless of how
+			// they appear in the underlying type.
+			canonical := canonicalize(T)
+			for i := 0; i < len(canonical); i++ {
+				m := canonical[i]
 				path2 := appendOpArg(path, opMethod, i)
 				if m == obj {
 					return Path(path2), nil // found declared method
@@ -421,11 +427,6 @@ func Object(pkg *types.Package, p Path) (types.Object, error) {
 	type hasElem interface {
 		Elem() types.Type
 	}
-	// abstraction of *types.{Interface,Named}
-	type hasMethods interface {
-		Method(int) *types.Func
-		NumMethods() int
-	}
 	// abstraction of *types.{Named,Signature}
 	type hasTypeParams interface {
 		TypeParams() *typeparams.TypeParamList
@@ -563,10 +564,11 @@ func Object(pkg *types.Package, p Path) (types.Object, error) {
 			if !ok {
 				return nil, fmt.Errorf("cannot apply %q to %s (got %T, want interface or named)", code, t, t)
 			}
-			if n := hasMethods.NumMethods(); index >= n {
+			canonical := canonicalize(hasMethods)
+			if n := len(canonical); index >= n {
 				return nil, fmt.Errorf("method index %d out of range [0-%d)", index, n)
 			}
-			obj = hasMethods.Method(index)
+			obj = canonical[index]
 			t = nil
 
 		case opObj:
@@ -588,3 +590,28 @@ func Object(pkg *types.Package, p Path) (types.Object, error) {
 
 	return obj, nil // success
 }
+
+// hasMethods is an abstraction of *types.{Interface,Named}. This is pulled up
+// because it is used by methodOrdering, which is in turn used by both encoding
+// and decoding.
+type hasMethods interface {
+	Method(int) *types.Func
+	NumMethods() int
+}
+
+// canonicalize returns a canonical order for the methods in a hasMethod.
+func canonicalize(hm hasMethods) []*types.Func {
+	count := hm.NumMethods()
+	if count <= 0 {
+		return nil
+	}
+	canon := make([]*types.Func, count)
+	for i := 0; i < count; i++ {
+		canon[i] = hm.Method(i)
+	}
+	less := func(i, j int) bool {
+		return canon[i].Id() < canon[j].Id()
+	}
+	sort.Slice(canon, less)
+	return canon
+}

go/types/objectpath/objectpath_test.go

@@ -46,6 +46,10 @@ type M map[struct{x int}]struct{y int}
 
 func unexportedFunc()
 type unexportedType struct{}
+
+type S struct{t struct{x int}}
+type R []struct{y int}
+type Q [2]struct{z int}
 `},
 		"a": {"a.go": `
 package a
@@ -80,6 +84,9 @@ type T struct{x, y int}
 		{"b", "T.M0.RA0", "var  *interface{f()}", ""},       // parameter
 		{"b", "T.M0.RA0.EM0", "func (interface).f()", ""},   // interface method
 		{"b", "unexportedType", "type b.unexportedType struct{}", ""},
+		{"b", "S.UF0.F0", "field x int", ""},
+		{"b", "R.UEF0", "field y int", ""},
+		{"b", "Q.UEF0", "field z int", ""},
 		{"a", "T", "type a.T struct{x int; y int}", ""},
 		{"a", "T.UF0", "field x int", ""},
 
@@ -99,6 +106,13 @@ type T struct{x, y int}
 		{"b", "A.UF0", "", "cannot apply 'U' to struct{x int} (got *types.Struct, want named)"},
 		{"b", "M.UPO", "", "cannot apply 'P' to map[struct{x int}]struct{y int} (got *types.Map, want signature)"},
 		{"b", "C.O", "", "path denotes type a.Int int, which belongs to a different package"},
+		{"b", "T.M9", "", "method index 9 out of range [0-1)"},
+		{"b", "M.UF0", "", "cannot apply 'F' to map[struct{x int}]struct{y int} (got *types.Map, want struct)"},
+		{"b", "V.KO", "", "cannot apply 'K' to []*a.T (got *types.Slice, want map)"},
+		{"b", "V.A4", "", "cannot apply 'A' to []*a.T (got *types.Slice, want tuple)"},
+		{"b", "V.RA0", "", "cannot apply 'R' to []*a.T (got *types.Slice, want signature)"},
+		{"b", "F.PA4", "", "tuple index 4 out of range [0-4)"},
+		{"b", "F.XO", "", "invalid path: unknown code 'X'"},
 	}
 	conf := loader.Config{Build: buildutil.FakeContext(pkgs)}
 	conf.Import("a")
@@ -310,3 +324,65 @@ func objectString(obj types.Object) string {
 
 	return s
 }
+
+// TestOrdering uses objectpath over two Named types with the same method
+// names but in a different source order and checks that objectpath is the
+// same for methods with the same name.
+func TestOrdering(t *testing.T) {
+	pkgs := map[string]map[string]string{
+		"p": {"p.go": `
+package p
+
+type T struct{ A int }
+
+func (T) M() { }
+func (T) N() { }
+func (T) X() { }
+func (T) Y() { }
+`},
+		"q": {"q.go": `
+package q
+
+type T struct{ A int }
+
+func (T) N() { }
+func (T) M() { }
+func (T) Y() { }
+func (T) X() { }
+`}}
+	conf := loader.Config{Build: buildutil.FakeContext(pkgs)}
+	conf.Import("p")
+	conf.Import("q")
+	prog, err := conf.Load()
+	if err != nil {
+		t.Fatal(err)
+	}
+	p := prog.Imported["p"].Pkg
+	q := prog.Imported["q"].Pkg
+
+	// From here, the objectpaths generated for p and q should be the
+	// same. If they are not, then we are generating an ordering that is
+	// dependent on the declaration of the types within the file.
+	for _, test := range []struct {
+		path objectpath.Path
+	}{
+		{"T.M0"},
+		{"T.M1"},
+		{"T.M2"},
+		{"T.M3"},
+	} {
+		pobj, err := objectpath.Object(p, test.path)
+		if err != nil {
+			t.Errorf("Object(%s) failed in a1: %v", test.path, err)
+			continue
+		}
+		qobj, err := objectpath.Object(q, test.path)
+		if err != nil {
+			t.Errorf("Object(%s) failed in a2: %v", test.path, err)
+			continue
+		}
+		if pobj.Name() != pobj.Name() {
+			t.Errorf("Objects(%s) not equal, got a1 = %v, a2 = %v", test.path, pobj.Name(), qobj.Name())
+		}
+	}
+}