From ee04797aa0b6be5ce3d5f7ac0f91e34716b3acdf Mon Sep 17 00:00:00 2001 From: Tim King Date: Wed, 6 Oct 2021 14:30:54 -0700 Subject: [PATCH] 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 Run-TryBot: Zvonimir Pavlinovic gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley Trust: Zvonimir Pavlinovic --- go/types/objectpath/objectpath.go | 49 +++++++++++++---- go/types/objectpath/objectpath_test.go | 76 ++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 11 deletions(-) diff --git a/go/types/objectpath/objectpath.go b/go/types/objectpath/objectpath.go index aa374c5363c..7e96fc234e5 100644 --- a/go/types/objectpath/objectpath.go +++ b/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 +} diff --git a/go/types/objectpath/objectpath_test.go b/go/types/objectpath/objectpath_test.go index 1e335ef7a37..39e7b1bcdf6 100644 --- a/go/types/objectpath/objectpath_test.go +++ b/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()) + } + } +}