Skip to content

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

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.

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

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
Labels
FrozenDueToAge Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Member

findleyr commented Jul 19, 2023

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
Contributor

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
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 <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
@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
Member 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.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Aug 10, 2023
@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 rsc moved this from Incoming to Active in Proposals Aug 30, 2023
@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

@rsc rsc moved this from Active to Likely Accept in Proposals Oct 3, 2023
@gopherbot
Copy link
Contributor

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 moved this from Likely Accept to Accepted in Proposals Oct 11, 2023
@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
@golang golang locked and limited conversation to collaborators Oct 10, 2024
@aclements aclements removed this from Proposals Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants