go/types/objectpath: remove method sorting

It was expensive, and unnecessary if we can rely on
go/types and export data preserving source order (we can).

Fixes golang/go#61443

Change-Id: I28d93c35f89eb751991c9d25aeb1c1904ba7b546
Reviewed-on: https://go-review.googlesource.com/c/tools/+/534139
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

go/analysis/unitchecker/unitchecker.go

@@ -319,7 +319,7 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re
 	analyzers = filtered
 
 	// Read facts from imported packages.
-	facts, err := facts.NewDecoder(pkg).Decode(false, makeFactImporter(cfg))
+	facts, err := facts.NewDecoder(pkg).Decode(makeFactImporter(cfg))
 	if err != nil {
 		return nil, err
 	}
@@ -418,7 +418,7 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re
 		results[i].diagnostics = act.diagnostics
 	}
 
-	data := facts.Encode(false)
+	data := facts.Encode()
 	if err := exportFacts(cfg, data); err != nil {
 		return nil, fmt.Errorf("failed to export analysis facts: %v", err)
 	}

go/types/objectpath/objectpath.go

@@ -26,13 +26,10 @@ package objectpath
 import (
 	"fmt"
 	"go/types"
-	"sort"
 	"strconv"
 	"strings"
-	_ "unsafe"
 
 	"golang.org/x/tools/internal/typeparams"
-	"golang.org/x/tools/internal/typesinternal"
 )
 
 // A Path is an opaque name that identifies a types.Object
@@ -123,20 +120,7 @@ func For(obj types.Object) (Path, error) {
 // An Encoder amortizes the cost of encoding the paths of multiple objects.
 // The zero value of an Encoder is ready to use.
 type Encoder struct {
-	scopeMemo         map[*types.Scope][]types.Object // memoization of scopeObjects
-	namedMethodsMemo  map[*types.Named][]*types.Func  // memoization of namedMethods()
-	skipMethodSorting bool
-}
-
-// Expose back doors so that gopls can avoid method sorting, which can dominate
-// analysis on certain repositories.
-//
-// TODO(golang/go#61443): remove this.
-func init() {
-	typesinternal.SkipEncoderMethodSorting = func(enc interface{}) {
-		enc.(*Encoder).skipMethodSorting = true
-	}
-	typesinternal.ObjectpathObject = object
+	scopeMemo map[*types.Scope][]types.Object // memoization of scopeObjects
 }
 
 // For returns the path to an object relative to its package,
@@ -328,31 +312,18 @@ func (enc *Encoder) For(obj types.Object) (Path, error) {
 		// Inspect declared methods of defined types.
 		if T, ok := o.Type().(*types.Named); ok {
 			path = append(path, opType)
-			if !enc.skipMethodSorting {
-				// Note that method index here is always with respect
-				// to canonical ordering of methods, regardless of how
-				// they appear in the underlying type.
-				for i, m := range enc.namedMethods(T) {
-					path2 := appendOpArg(path, opMethod, i)
-					if m == obj {
-						return Path(path2), nil // found declared method
-					}
-					if r := find(obj, m.Type(), append(path2, opType), nil); r != nil {
-						return Path(r), nil
-					}
+			// The method index here is always with respect
+			// to the underlying go/types data structures,
+			// which ultimately derives from source order
+			// and must be preserved by export data.
+			for i := 0; i < T.NumMethods(); i++ {
+				m := T.Method(i)
+				path2 := appendOpArg(path, opMethod, i)
+				if m == obj {
+					return Path(path2), nil // found declared method
 				}
-			} else {
-				// This branch must match the logic in the branch above, using go/types
-				// APIs without sorting.
-				for i := 0; i < T.NumMethods(); i++ {
-					m := T.Method(i)
-					path2 := appendOpArg(path, opMethod, i)
-					if m == obj {
-						return Path(path2), nil // found declared method
-					}
-					if r := find(obj, m.Type(), append(path2, opType), nil); r != nil {
-						return Path(r), nil
-					}
+				if r := find(obj, m.Type(), append(path2, opType), nil); r != nil {
+					return Path(r), nil
 				}
 			}
 		}
@@ -448,22 +419,13 @@ func (enc *Encoder) concreteMethod(meth *types.Func) (Path, bool) {
 	path = append(path, name...)
 	path = append(path, opType)
 
-	if !enc.skipMethodSorting {
-		for i, m := range enc.namedMethods(named) {
-			if m == meth {
-				path = appendOpArg(path, opMethod, i)
-				return Path(path), true
-			}
-		}
-	} else {
-		// This branch must match the logic of the branch above, using go/types
-		// APIs without sorting.
-		for i := 0; i < named.NumMethods(); i++ {
-			m := named.Method(i)
-			if m == meth {
-				path = appendOpArg(path, opMethod, i)
-				return Path(path), true
-			}
+	// Method indices are w.r.t. the go/types data structures,
+	// ultimately deriving from source order,
+	// which is preserved by export data.
+	for i := 0; i < named.NumMethods(); i++ {
+		if named.Method(i) == meth {
+			path = appendOpArg(path, opMethod, i)
+			return Path(path), true
 		}
 	}
 
@@ -576,12 +538,7 @@ func findTypeParam(obj types.Object, list *typeparams.TypeParamList, path []byte
 
 // Object returns the object denoted by path p within the package pkg.
 func Object(pkg *types.Package, p Path) (types.Object, error) {
-	return object(pkg, string(p), false)
-}
-
-// Note: the skipMethodSorting parameter must match the value of
-// Encoder.skipMethodSorting used during encoding.
-func object(pkg *types.Package, pathstr string, skipMethodSorting bool) (types.Object, error) {
+	pathstr := string(p)
 	if pathstr == "" {
 		return nil, fmt.Errorf("empty path")
 	}
@@ -747,12 +704,7 @@ func object(pkg *types.Package, pathstr string, skipMethodSorting bool) (types.O
 				if index >= t.NumMethods() {
 					return nil, fmt.Errorf("method index %d out of range [0-%d)", index, t.NumMethods())
 				}
-				if skipMethodSorting {
-					obj = t.Method(index)
-				} else {
-					methods := namedMethods(t) // (unmemoized)
-					obj = methods[index]       // Id-ordered
-				}
+				obj = t.Method(index)
 
 			default:
 				return nil, fmt.Errorf("cannot apply %q to %s (got %T, want interface or named)", code, t, t)
@@ -779,33 +731,6 @@ func object(pkg *types.Package, pathstr string, skipMethodSorting bool) (types.O
 	return obj, nil // success
 }
 
-// namedMethods returns the methods of a Named type in ascending Id order.
-func namedMethods(named *types.Named) []*types.Func {
-	methods := make([]*types.Func, named.NumMethods())
-	for i := range methods {
-		methods[i] = named.Method(i)
-	}
-	sort.Slice(methods, func(i, j int) bool {
-		return methods[i].Id() < methods[j].Id()
-	})
-	return methods
-}
-
-// namedMethods is a memoization of the namedMethods function. Callers must not modify the result.
-func (enc *Encoder) namedMethods(named *types.Named) []*types.Func {
-	m := enc.namedMethodsMemo
-	if m == nil {
-		m = make(map[*types.Named][]*types.Func)
-		enc.namedMethodsMemo = m
-	}
-	methods, ok := m[named]
-	if !ok {
-		methods = namedMethods(named) // allocates and sorts
-		m[named] = methods
-	}
-	return methods
-}
-
 // scopeObjects is a memoization of scope objects.
 // Callers must not modify the result.
 func (enc *Encoder) scopeObjects(scope *types.Scope) []types.Object {

gopls/internal/lsp/cache/analysis.go

@@ -1201,7 +1201,7 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 	// by "deep" export data. Better still, use a "shallow" approach.
 
 	// Read and decode analysis facts for each direct import.
-	factset, err := pkg.factsDecoder.Decode(true, func(pkgPath string) ([]byte, error) {
+	factset, err := pkg.factsDecoder.Decode(func(pkgPath string) ([]byte, error) {
 		if !hasFacts {
 			return nil, nil // analyzer doesn't use facts, so no vdeps
 		}
@@ -1343,7 +1343,7 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 		panic(fmt.Sprintf("%v: Pass.ExportPackageFact(%T) called after Run", act, fact))
 	}
 
-	factsdata := factset.Encode(true)
+	factsdata := factset.Encode()
 	return result, &actionSummary{
 		Diagnostics: diagnostics,
 		Facts:       factsdata,

internal/facts/facts.go

@@ -48,7 +48,6 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/types/objectpath"
-	"golang.org/x/tools/internal/typesinternal"
 )
 
 const debug = false
@@ -205,9 +204,7 @@ type GetPackageFunc = func(pkgPath string) *types.Package
 //
 // Concurrent calls to Decode are safe, so long as the
 // [GetPackageFunc] (if any) is also concurrency-safe.
-//
-// TODO(golang/go#61443): eliminate skipMethodSorting one way or the other.
-func (d *Decoder) Decode(skipMethodSorting bool, read func(pkgPath string) ([]byte, error)) (*Set, error) {
+func (d *Decoder) Decode(read func(pkgPath string) ([]byte, error)) (*Set, error) {
 	// Read facts from imported packages.
 	// Facts may describe indirectly imported packages, or their objects.
 	m := make(map[key]analysis.Fact) // one big bucket
@@ -247,7 +244,7 @@ func (d *Decoder) Decode(skipMethodSorting bool, read func(pkgPath string) ([]by
 			key := key{pkg: factPkg, t: reflect.TypeOf(f.Fact)}
 			if f.Object != "" {
 				// object fact
-				obj, err := typesinternal.ObjectpathObject(factPkg, string(f.Object), skipMethodSorting)
+				obj, err := objectpath.Object(factPkg, f.Object)
 				if err != nil {
 					// (most likely due to unexported object)
 					// TODO(adonovan): audit for other possibilities.
@@ -271,11 +268,8 @@ func (d *Decoder) Decode(skipMethodSorting bool, read func(pkgPath string) ([]by
 //
 // It may fail if one of the Facts could not be gob-encoded, but this is
 // a sign of a bug in an Analyzer.
-func (s *Set) Encode(skipMethodSorting bool) []byte {
+func (s *Set) Encode() []byte {
 	encoder := new(objectpath.Encoder)
-	if skipMethodSorting {
-		typesinternal.SkipEncoderMethodSorting(encoder)
-	}
 
 	// TODO(adonovan): opt: use a more efficient encoding
 	// that avoids repeating PkgPath for each fact.

internal/facts/facts_test.go

@@ -311,7 +311,7 @@ func testEncodeDecode(t *testing.T, files map[string]string, tests []pkgLookups)
 		}
 
 		// decode
-		facts, err := facts.NewDecoder(pkg).Decode(false, read)
+		facts, err := facts.NewDecoder(pkg).Decode(read)
 		if err != nil {
 			t.Fatalf("Decode failed: %v", err)
 		}
@@ -345,7 +345,7 @@ func testEncodeDecode(t *testing.T, files map[string]string, tests []pkgLookups)
 		}
 
 		// encode
-		factmap[pkg.Path()] = facts.Encode(false)
+		factmap[pkg.Path()] = facts.Encode()
 	}
 }
 
@@ -413,7 +413,7 @@ func TestFactFilter(t *testing.T) {
 	}
 
 	obj := pkg.Scope().Lookup("A")
-	s, err := facts.NewDecoder(pkg).Decode(false, func(pkgPath string) ([]byte, error) { return nil, nil })
+	s, err := facts.NewDecoder(pkg).Decode(func(pkgPath string) ([]byte, error) { return nil, nil })
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -528,7 +528,7 @@ func TestMalformed(t *testing.T) {
 				packages[pkg.Path()] = pkg
 
 				// decode facts
-				facts, err := facts.NewDecoder(pkg).Decode(false, read)
+				facts, err := facts.NewDecoder(pkg).Decode(read)
 				if err != nil {
 					t.Fatalf("Decode failed: %v", err)
 				}
@@ -555,7 +555,7 @@ func TestMalformed(t *testing.T) {
 				}
 
 				// encode facts
-				factmap[pkg.Path()] = facts.Encode(false)
+				factmap[pkg.Path()] = facts.Encode()
 			}
 		})
 	}

internal/typesinternal/objectpath.go

@@ -1,24 +0,0 @@
-// Copyright 2023 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package typesinternal
-
-import "go/types"
-
-// This file contains back doors that allow gopls to avoid method sorting when
-// using the objectpath package.
-//
-// This is performance-critical in certain repositories, but changing the
-// behavior of the objectpath package is still being discussed in
-// golang/go#61443. If we decide to remove the sorting in objectpath we can
-// simply delete these back doors. Otherwise, we should add a new API to
-// objectpath that allows controlling the sorting.
-
-// SkipEncoderMethodSorting marks enc (which must be an *objectpath.Encoder) as
-// not requiring sorted methods.
-var SkipEncoderMethodSorting func(enc interface{})
-
-// ObjectpathObject is like objectpath.Object, but allows suppressing method
-// sorting.
-var ObjectpathObject func(pkg *types.Package, p string, skipMethodSorting bool) (types.Object, error)