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.

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

x/tools/gopls: refactor: compute complete sets of methods related through interface satisfaction #58461

Open
adonovan opened this issue Feb 10, 2023 · 5 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

In our work to incrementalize type checking, we intentionally regressed on our support for method renaming in order to unblock progress. Specifically, when we rename an interface method and discover a concrete method that also needs renaming, we need to not just add the concrete method to the set of target objects but also include all of the reverse dependencies of that concrete method in the set of target packages. This process of dynamic programming the cross product of objects and packages must be iterated until it reaches a fixed point.

We need to fix this before v0.12.0.

@adonovan adonovan added this to the gopls/v0.12.0 milestone Feb 10, 2023
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Feb 10, 2023
@gopherbot
Copy link

Change https://go.dev/cl/464902 mentions this issue: gopls/internal/lsp/source/rename: incremental renaming

gopherbot pushed a commit to golang/tools that referenced this issue Feb 14, 2023
This change is a reimplementation of the renaming operation so
that it no longer mixes types.Objects from different packages.
The basic approach is as follows.

First, the referenced object is found and classified. If it is
nonexported (e.g. lowercase, or inherently local such as an
import or label, or within a function body), the operation is
local to that package and is carried out essentially as before.

However, if it is exported, then the scope of the global search
is deduced (direct for package-level var/func/const/type,
transitive for fields and methods). The object is converted to an
objectpath, and all the reverse dependencies are analyzed, using
the objectpath to identify the target in each package.

The renameObject function (the entry point for the fiddly renamer
algorithm) is now called once per package, and the results of all
calls are combined.

Because we process variants, duplicate edits are likely. We sort
and de-dup at the very end under the assumption that edits are
well behaved. The "seen edit" tracking in package renaming is no
longer needed.

Also:
- Package.importMap maps PackagePath to Package for all dependencies,
  so that we can resolve targets identified by (PackagePath,
  objectpath) to their objects in a different types.Importer "realm".
  It is materialized as a DAG of k/v pairs and exposed as
  Package.DependencyTypes.
- The rename_check algorithm (renamer) is now applied once to each
  package instead of once to all packages.
- The set of references to update is derived from types.Info, not the
  references operation.

Still to do in follow-ups:
- Method renaming needs to expand the set of renamed types (based on
  'satisfy') and recompute the dependencies of their declarations,
  until a fixed point is reached. (Not supporting this case is a
  functional regression since v0.11.0, but we plan to submit this
  change to unblock foundational progress and then fix it before the
  next release. See golang/go#58461)
- Lots of generics cases to consider (see golang/go#58462).
- Lots more tests required. For golang/go#57987.

Change-Id: I5fd8538ab35d61744d765d8bd101cd4efa41bd33
Reviewed-on: https://go-review.googlesource.com/c/tools/+/464902
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@adonovan
Copy link
Member Author

adonovan commented Apr 7, 2023

Related: #58506. I think the fix for that bug also fixed this one, but we need to confirm.

@adonovan adonovan added the Refactoring Issues related to refactoring tools label Apr 24, 2023
@adonovan
Copy link
Member Author

Still not fixed.

@findleyr
Copy link
Contributor

One note, while working on change signature refactoring: it is really a more general problem to discover the "equivalence set" of signatures for a given View. It's not specific to rename. We'd want to do this for change signature refactoring as well.

Modifying the issue title accordingly.

@findleyr findleyr changed the title x/tools/gopls: rename: handle methods properly in new implementation x/tools/gopls: refactor: compute complete sets of methods related through interface satisfaction Mar 18, 2024
@gopherbot
Copy link

Change https://go.dev/cl/572296 mentions this issue: gopls/internal/golang: support removing unused parameters from methods

gopherbot pushed a commit to golang/tools that referenced this issue Mar 18, 2024
Make a minimal change to the removeparam algorithm to support removing
unused parameters of methods.

This doesn't address the problem of expanding the set of signatures or
calls based on interface satisfaction constraints: that is really a
separate concern, which also affects method renaming (golang/go#58461).
Were we to have a more general solution to that problem, it would be
relatively straightforward to also expand the change signature
algorithm.

For golang/go#38028

Change-Id: I296cb1f828f07d841c83b1fd33593ccd2fee3539
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572296
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants