x/tools/gopls: rename: handle generic methods more precisely #58462
Labels
gopls/generics
Issues related to gopls' support for generics
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
gopherbot
added
Tools
|
Change https://go.dev/cl/464902 mentions this issue: |
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
added
Refactoring
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
gopls's renaming is tricky in the presence of generic methods, and our support for them is rudimentary. The challenge is in trying to decide which interfaces and methods correspond (and must be renamed together) when type parameters are involved. Recent changes to the renaming implementation for incrementalized type checking have not made things worse but have made us more aware of the problems and the paucity of testing generally. We should try to come up with a more comprehensive theory of how to do it. This isn't a regression so it needn't happen for v0.12.0.
The text was updated successfully, but these errors were encountered: