Navigation Menu

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: support module-local renaming #32877

Closed
inliquid opened this issue Jul 1, 2019 · 25 comments
Closed

x/tools/gopls: support module-local renaming #32877

inliquid opened this issue Jul 1, 2019 · 25 comments
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@inliquid
Copy link

inliquid commented Jul 1, 2019

VS Code. Latest gopls code from master. Trying quite simple rename operation:
изображение

@gopherbot gopherbot added this to the Unreleased milestone Jul 1, 2019
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Jul 1, 2019
@stamblerre
Copy link
Contributor

Right now, rename only works within a package and its tests. We are still working on a strategy for renaming packages in a module.

/cc @suzmue

@suzmue
Copy link
Contributor

suzmue commented Jul 2, 2019

This is closely tied to issue #32869

Once find all references in the module level is supported, we should be able to global renaming.

@stamblerre stamblerre changed the title x/tools/cmd/gopls: rename doesn't work for all occurences x/tools/gopls: rename doesn't work for all occurences Jul 2, 2019
@stamblerre stamblerre changed the title x/tools/gopls: rename doesn't work for all occurences x/tools/gopls: support module-local renaming Aug 5, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Aug 26, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@abitrolly
Copy link

Why modules are so important here? Is it possible to detect project root and scan all symbols the same way go build and go test ./... do?

@stamblerre
Copy link
Contributor

The go command is quite different from gopls, but we will be able to scan the module root in a similar way. The reason that modules are relevant here is that the idea of renaming a symbol is different in module mode. You only want to rename a symbol within the module where it is defined, since clients depend on specific versions of your module, which will be found in the module cache. In $GOPATH mode, we would rename the symbol throughout the other projects in your $GOPATH.

@abitrolly
Copy link

abitrolly commented Oct 15, 2019

When I did git clone for the tflint project I was editing and get project checkout, I didn't have to place the project somewhere on $GOPATH. Therefore it seems that it should be quite easy to cut all paths that lie outside of the project directory. If gopls can not detect which packages are native to project and which are external.

@gopherbot
Copy link

Change https://golang.org/cl/207264 mentions this issue: internal/lsp/testadata/rename: add a test case for renames across packages

@cskr
Copy link

cskr commented Dec 28, 2019

Is this really fixed? Renames across packages in the same module still fail with the latest commit in master. The error displayed within vim-go is:

failed to rename because "Foo" is declared in package "bar"

@stamblerre
Copy link
Contributor

Yes. That error sounds like you are trying to do a rename outside of the declaring package. I believe you have to be in the package where the variable is declared to rename it.

@cskr
Copy link

cskr commented Dec 31, 2019

Renaming from the file containing the declaration works. It does not rename references outside the package declaring the identifier, but in the same module. Is this a known issue? Is there an existing issue I can follow to know when tthishat is fixed?

@stamblerre
Copy link
Contributor

This is the issue tracking renaming identifiers across packages in the same module. Renaming across multiple modules is not supported. This functionality is available at master, not v0.2.2. You can confirm that you are using gopls at master by running gopls version.

@toejough
Copy link

toejough commented Jan 4, 2020

I had the same experience just now.

❯ gopls version
golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@(devel)
❯ git log --oneline | head -1
774c71fc internal/lsp/cache: fix type error reporting in cgo

FWIW, I'm using the latest LanguageClient-neovim on the latest neovim, not going through vscode.

Within the same module:

  • package A defines function Foo
  • package B uses function Foo
  • from A, rename Foo to Bar
  • A.Foo is correctly renamed to Bar
  • B's use of Foo remains, unrenamed.

@toejough
Copy link

toejough commented Jan 4, 2020

Honestly, though, I don't know how to distinguish between a server problem and a client problem, so I don't know if this is on gopls. Happy to try out any other recommended debugging steps. I'll poke around for some & try another client, at least.

@toejough
Copy link

toejough commented Jan 5, 2020

Tried out ALE, which failed to rename anything, and coc.nvim, which did the same local-file-only rename.

I was able to run gopls with logging, and captured the following for a rename:

[Trace - 21:05:33.918 PM] Sending request 'textDocument/prepareRename - (1)'.
Params: {"textDocument":{"uri":"file:///foo/bar/baz.go"},"position":{"line":77,"character":17}}


[Trace - 21:05:33.919 PM] Received response 'textDocument/prepareRename - (1)' in 0ms.
Result: {"start":{"line":77,"character":17},"end":{"line":77,"character":29}}


[Trace - 21:05:37.183 PM] Sending request 'textDocument/rename - (2)'.
Params: {"textDocument":{"uri":"file:///foo/bar/baz.go"},"position":{"line":77,"character":17},"newName":"ResetResultsNewSuffix"}


[Trace - 21:05:37.235 PM] Received response 'textDocument/rename - (2)' in 51ms.
Result: {"documentChanges":[{"textDocument":{"version":1,"uri":"file:///foo/bar/baz.go"},"edits":[{"range":{"start":{"line":76,"character":3},"end":{"line":76,"character":15}},"newText":"ResetResultsNewSuffix"},{"range":{"start":{"line":77,"character":17},"end":{"line":77,"character":29}},"newText":"ResetResultsNewSuffix"}]}]}


[Trace - 21:05:37.257 PM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":2,"uri":"file:///foo/bar/baz.go"},"contentChanges":[{"range":{"start":{"line":76,"character":15},"end":{"line":77,"character":29}},"rangeLength":70,"text":"NewSuffix resets the results fields on the thing\nfunc (t *Thing) ResetResultsNewSuffix"}]}

... and the following for a references search on the original name:

[Trace - 21:11:13.801 PM] Sending request 'textDocument/references - (3)'.
Params: {"textDocument":{"uri":"file:///foo/bar/baz.go"},"position":{"line":77,"character":17},"context":{"includeDeclaration":false}}


[Trace - 21:11:15.000 PM] Received response 'textDocument/references - (3)' in 1198ms.
Result: [{"uri":"file:///foo/bar/other.go","range":{"start":{"line":680,"character":3},"end":{"line":680,"character":15}}}]

Notice the rename only applies to the original file, when a call to find references on the original name returns a different file. I changed paths/names, so you'll have to trust me that it's all in the same module.

@abitrolly
Copy link

@toejough is it possible to express in test code like golang/tools@4191b8c ?

@abitrolly
Copy link

One of https://github.com/golang/tools/tree/master/internal/lsp/testdata/rename should fail if there is a bug.

@toejough
Copy link

toejough commented Jan 6, 2020

do I have to do something special to run those tests?

go/src/golang.org/x/tools/gopls> go test ./test -v
...
            --- SKIP: TestCommandLine/GOPATH/Renames/crosspkg_Dolphin (0.03s)
                format.go:57: multi-file patch tests not supported yet
            --- SKIP: TestCommandLine/GOPATH/Renames/crosspkg_Tomato (0.02s)
                format.go:57: multi-file patch tests not supported yet
...

@abitrolly
Copy link

No idea. @ianthehat should know.

@stamblerre
Copy link
Contributor

You can run the tests inside of the golang.org/x/tools/internal/lsp directory by running go test ./internal/lsp/.... As @abitrolly said, we already have tests for these cases, so they should work correctly.

@toejough: I am unable to reproduce your example. In fact, I am able to rename from the function's use, not just the declaration. What is your workspace root when you open your editor?

@toejough
Copy link

toejough commented Jan 6, 2020

thanks - calling go test that way results in passing tests, not skipped.

what editor/plugin/client are you using to do the rename? I'm back to using vim-go, and I still get the failed to rename because "foo" is delcared in package "bar" error when trying to rename from the function's use.

I'm opening vim from my module's root directory, and :pwd in vim shows that dir still. if I open it from the directory of the file using the function, I get the same results, though.

If I send debug output to a file at launch, I get a message like:

[Trace - 18:03:10.549 PM] Sending request 'initialize - (1)'.
Params: {"rootUri": "file:///Users/you/go/src/company", "capabilities": {"workspace": {"workspaceFolders": true, "configuration": true, "didChangeConfiguration": {"dynamicRegistration": true}}, "textDocument": {"completion": {"completionItem": {"snippetSupport": true}}, "hover": {"contentFormat": ["plaintext"]}}}, "processId": 14403, "workspaceFolders": [{"uri": "file:///Users/you/go/src/company", "name": "/Users/you/go/src/company"}]}

@stamblerre
Copy link
Contributor

I believe that vim-go currently uses gopls rename through the command line, which may be why you are seeing these errors. I use VS Code with the VS Code Go extension to run gopls.

/cc @bhcleek for vim-go expertise

@toejough
Copy link

toejough commented Jan 7, 2020

I deleted my last reply - I made it via email from the bus at the end of my work day - having gotten dinner in me now I realize it didn't really add anything.

I appreciate the help on this (and gopls and vim-go in general - they are amazing tools!). I'll try vscode tomorrow with my codebase to see how that responds.

@bhcleek
Copy link
Contributor

bhcleek commented Jan 7, 2020

@stamblerre you're right that vim-go currently uses gopls rename.

@toejough
Copy link

toejough commented Jan 9, 2020

I still get the error about the function being defined in another package, but using vscode I'm able to rename across packages successfully. I'll give vscode another try for a bit, and move over to vim-go's project for more plugin/client discussion.

Thank you for the help and responsiveness!

@toejough
Copy link

toejough commented Jan 9, 2020

Is there an issue for making gopls rename do the same thing as whatever the vscode plugin is doing? I assume it's talking to gopls with the language server protocol, but I went to go file an issue with vim-go and I realized I still don't understand the problem well enough. Shouldn't gopls rename do module-local renaming?

@stamblerre
Copy link
Contributor

gopls rename works within the scope of the directory that you are running it from. #32394 tracks automatically detecting the correct scope, but for now, gopls requires a scope.

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Jul 22, 2020
@golang golang locked and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

8 participants