x/tools/gopls: support module-local renaming

[inliquid]

VS Code. Latest gopls code from master. Trying quite simple rename operation:

[stamblerre]

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]

This is closely tied to issue #32869

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

[abitrolly]

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]

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]

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]

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

[cskr]

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]

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]

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]

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]

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]

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]

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]

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

[abitrolly]

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

[toejough]

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]

No idea. @ianthehat should know.

[stamblerre]

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]

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]

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]

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]

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

[toejough]

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]

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]

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.