Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/go/types/objectpath: remove sorting of Named type methods #61443

Closed
findleyr opened this issue Jul 19, 2023 · 8 comments
Closed

x/tools/go/types/objectpath: remove sorting of Named type methods #61443

findleyr opened this issue Jul 19, 2023 · 8 comments
Assignees
@adonovan @findleyr
Labels
Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@findleyr
Copy link
Contributor

findleyr commented Jul 19, 2023 •
edited

As we've seen in #58668 (comment), the sorting of Named type methods can dominate the cost of using the objectpath package. In the example there, both gopls and the staticcheck analysis driver are essentially unusable when Named type methods are sorted.

Proposal: We propose to remove this sorting, effectively reverting the fix for #44195. We should instead guarantee in our documentation that the order of methods is deterministic, both the compiler and go/types agree on this sorting, and this sorting is preserved by all importers/exporters.

(Aside: I'm going to remove the sorting (conditionally) in gopls now, because this has a large impact on gopls usability. However, whatever back-channels I use to do so should be short-lived, and we should agree on a path forward that works for everyone.)

CC @adonovan @dominikh @griesemer @mdempsky
@findleyr findleyr self-assigned this Jul 19, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 19, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jul 19, 2023
@gopherbot
Copy link

Change https://go.dev/cl/517737 mentions this issue: go/types/objectpath: remove use of linkname for gopls back doors

gopherbot pushed a commit to golang/tools that referenced this issue Aug 9, 2023
@findleyr
go/types/objectpath: remove use of linkname for gopls back doors
59fd05d 
Use internal variables as back doors for gopls into the objectpath
package, rather than linkname. Using linkname breaks x/tools vendoring.

See golang/go#61443 for background as to why this back door is
necessary.

Change-Id: Iabf6e825d169ac1c4080dc326eccc661eaae7ec6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/517737
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@findleyr findleyr changed the title x/tools/go/types/objectpath: remove sorting of Named type methods proposal: x/tools/go/types/objectpath: remove sorting of Named type methods Aug 10, 2023
@findleyr
Copy link
Contributor Author

Elevating this to a proposal, per discussion with @rsc. We need to decide whether it's ok to revert this behavior. Otherwise, we need to propose a new API that disables sorting.

@dominikh
Copy link
Member

The current behavior of objectpath is quite under-defined. It doesn't say anything about sorting, the stability of paths, what will cause them to change, etc.

@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 6, 2023

Have all concerns about this proposal been addressed?

@rsc
Copy link
Contributor

rsc commented Oct 3, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/534139 mentions this issue: go/types/objectpath: remove method sorting

@rsc
Copy link
Contributor

rsc commented Oct 11, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/tools/go/types/objectpath: remove sorting of Named type methods x/tools/go/types/objectpath: remove sorting of Named type methods Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

@findleyr findleyr

Labels
Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Proposals
Status: Accepted
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@dominikh @rsc @adonovan @gopherbot @findleyr