x/tools/go/types/objectpath: TestOrdering asserts that method order is source-independent; should be that go/types agrees with cmd/compile

[adonovan]

TestOrdering has always contained a typo, now flagged by staticcheck in gopls, that causes its assertion to trivially pass. Fixing it reveals that the test was broken by yours truly in b9b97d9 (go.dev/cl/534139) when we removed Id-based sorting (because expensive). That means we are now using Pos-based sorting.

This seems like a bad outcome for gopls, among others:

• Reordering method decls may change the object paths for them and their elements (params etc).
• analysis fact caching may be broken.
• the objectpaths obtained when parsing source or loading from export data may differ.

The task of this issue is to figure out how bad this is and either reintroduce sorting (perhaps optimized somehow) or some other fix.

[gabyhelp]

Related Issues

• x/tools/go/types/objectpath: remove sorting of Named type methods #61443 (closed)
• go/types, x/tools/go/types, x/tools/analysis: encoding used by objectpath is inconsistent for use by compiler & tools/analysis #44195 (closed)
• go/types: guarantee sorting of named type methods #61298 (closed)
• go/types: change in embedded type ordering on dev.typeparams branch #47352 (closed)
• x/tools/go/types/objectpath: frequent use of scope.Names burns significant cycles #51017 (closed)

Related Code Changes

• go/types: use a stable ordering for methods

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

That means we are now using Pos-based sorting.

On closer inspection, the methods appear in the order returned by the type checker, which is source order, not Pos order. The two may differ in applications such as gopls that parse files out of order due to caching; so this means the format is robust to such optimizations.

This distinction is only a matter of degree: either way, method indices are not invariant across movement of method declarations within the same file. But this is not actually a problem: objectpaths are supposed to be names for symbols derived from different type-checking operations on the same package sources; they are not meaningful across different versions of the package as it evolves. So I think there is no latent problem.

TestOrdered was added to prove that issue #44195 (go 1.15?) was fixed by the sorting introduced in https://go.dev/cl/354433. The bug in that case was a discrepancy between the order of methods emitted by the compiler and by go/types. Since the same algorithm is is now used in both tools (since generics was added in go 1.18), that problem will have gone away. But the test case it left behind is too strict: instead of testing that objectpath is declaration-order independent, it should have tested that the compiler and go/types emit methods in the same order, whatever that is.