x/tools/gopls: rename on a method receiver should rename all receivers for the type

[bcmills]

What version of Go are you using (go version)?

example.com$ go version
go version devel +2be7788f8 Fri Oct 9 02:49:19 2020 +0000 linux/amd64

example.com$ go version -m $(which gopls)
/usr/local/google/home/bcmills/bin/gopls: devel +186f0220d0 Mon Oct 5 11:12:24 2020 -0400
        path    golang.org/x/tools/gopls
        mod     golang.org/x/tools/gopls        v0.5.1  h1:AF3Uh7HF08SZpKFfgJO6zfF3bbxyDXWqdkK4kMXiQ1o=
        dep     github.com/BurntSushi/toml      v0.3.1  h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
        dep     github.com/google/go-cmp        v0.5.1  h1:JFrFEBb2xKufg6XkJsJr+WbKb4FQlURi5RUcBveYu9k=
        dep     github.com/sergi/go-diff        v1.1.0  h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
        dep     golang.org/x/mod        v0.3.0  h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
        dep     golang.org/x/sync       v0.0.0-20200625203802-6e8e738ad208      h1:qwRHBd0NqMbJxfbotnDhm2ByMI1Shq4Y6oRJo21SGJA=
        dep     golang.org/x/tools      v0.0.0-20200930165741-f1523d29dbb9      h1:1R38tQp22dcHpTKJPjgVa16FhlDy/kHEaCM/ndi/FIc=
        dep     golang.org/x/xerrors    v0.0.0-20200804184101-5ec99f83aff1      h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
        dep     honnef.co/go/tools      v0.0.1-2020.1.5 h1:nI5egYTGJakVyOryqLs1cQO5dO0ksin5XXs2pspk75k=
        dep     mvdan.cc/gofumpt        v0.0.0-20200802201014-ab5a8192947d      h1:t8TAw9WgTLghti7RYkpPmqk4JtQ3+wcP5GgZqgWeWLQ=
        dep     mvdan.cc/xurls/v2       v2.2.0  h1:NSZPykBXJFCetGZykLAxaL6SIpvbVy/UFEniIfHAa8A=

example.com$ ls -d ~/.emacs.d/elpa/eglot-*
/usr/local/google/home/bcmills/.emacs.d/elpa/eglot-20200830.1254

example.com$ emacs --version
GNU Emacs 27.1

Does this issue reproduce with the latest release?

Yes.

What did you do?

Initial state:

-- go.mod --
module example.com

go 1.16
-- main.go --
package main

import "fmt"

type T struct{}

func (x *T) String() string {
	return fmt.Sprintf("%T{}", x)
}

func (x *T) Clone() *T {
	y := new(T)
	*y = *x
	return y
}

func main() {}

1. emacs -nw main.go
2. Move the point to the first method receiver (main.go:7:6).
3. M-x eglot-rename
4. Enter a new name (such as t) for the method receiver.

What did you expect to see?

Per http://golang.org/wiki/CodeReviewComments#receiver-names:

Be consistent, too: if you call the receiver "c" in one method, don't call it "cl" in another.

Since the file already satisfied that consistency rule before the rename, I expect it to satisfy that rule after the rename, too: all of the method receivers should have been renamed to the new name entered in step (4).

What did you see instead?

Only the receiver for the one method was renamed.

gopls log:

[Trace - 15:40:27.246 PM] Sending request 'initialize - (1)'.
Params: {"processId":712138,"rootPath":"/tmp/tmp.hs2uMVS9fG/example.com/","rootUri":"file:///tmp/tmp.hs2uMVS9fG/example.com/","initializationOptions":null,"capabilities":{"workspace":{"applyEdit":true,"executeCommand":{"dynamicRegistration":false},"workspaceEdit":{"documentChanges":false},"didChangeWatchedFiles":{"dynamicRegistration":true},"symbol":{"dynamicRegistration":false},"configuration":true},"textDocument":{"synchronization":{"dynamicRegistration":false,"willSave":true,"willSaveWaitUntil":true,"didSave":true},"completion":{"dynamicRegistration":false,"completionItem":{"snippetSupport":true},"contextSupport":true},"hover":{"dynamicRegistration":false,"contentFormat":["markdown","plaintext"]},"signatureHelp":{"dynamicRegistration":false,"signatureInformation":{"parameterInformation":{"labelOffsetSupport":true}}},"references":{"dynamicRegistration":false},"definition":{"dynamicRegistration":false},"declaration":{"dynamicRegistration":false},"implementation":{"dynamicRegistration":false},"typeDefinition":{"dynamicRegistration":false},"documentSymbol":{"dynamicRegistration":false,"hierarchicalDocumentSymbolSupport":true,"symbolKind":{"valueSet":[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26]}},"documentHighlight":{"dynamicRegistration":false},"codeAction":{"dynamicRegistration":false,"codeActionLiteralSupport":{"codeActionKind":{"valueSet":["quickfix","refactor","refactor.extract","refactor.inline","refactor.rewrite","source","source.organizeImports"]}}},"formatting":{"dynamicRegistration":false},"rangeFormatting":{"dynamicRegistration":false},"rename":{"dynamicRegistration":false},"publishDiagnostics":{"relatedInformation":false}},"experimental":null}}


[Trace - 15:40:27.246 PM] Received response 'initialize - (1)' in 0ms.
Result: {"capabilities":{"textDocumentSync":{"openClose":true,"change":2,"save":{}},"completionProvider":{"triggerCharacters":["."]},"hoverProvider":true,"signatureHelpProvider":{"triggerCharacters":["(",","]},"definitionProvider":true,"typeDefinitionProvider":true,"implementationProvider":true,"referencesProvider":true,"documentHighlightProvider":true,"documentSymbolProvider":true,"codeActionProvider":{"codeActionKinds":["quickfix","refactor.extract","refactor.rewrite","source.fixAll","source.organizeImports"]},"codeLensProvider":{},"documentLinkProvider":{},"workspaceSymbolProvider":true,"documentFormattingProvider":true,"documentOnTypeFormattingProvider":{"firstTriggerCharacter":""},"renameProvider":true,"foldingRangeProvider":true,"executeCommandProvider":{"commands":["generate","fill_struct","regenerate_cgo","test","tidy","undeclared_name","upgrade_dependency","vendor","extract_variable","extract_function","gc_details","generate_gopls_mod"]},"callHierarchyProvider":true,"workspace":{"workspaceFolders":{"supported":true,"changeNotifications":"workspace/didChangeWorkspaceFolders"}}},"serverInfo":{"name":"gopls","version":"Build info\n----------\ngolang.org/x/tools/gopls v0.5.1\n    golang.org/x/tools/[email protected] h1:AF3Uh7HF08SZpKFfgJO6zfF3bbxyDXWqdkK4kMXiQ1o=\n    github.com/BurntSushi/[email protected] h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=\n    github.com/google/[email protected] h1:JFrFEBb2xKufg6XkJsJr+WbKb4FQlURi5RUcBveYu9k=\n    github.com/sergi/[email protected] h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=\n    golang.org/x/[email protected] h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=\n    golang.org/x/[email protected] h1:qwRHBd0NqMbJxfbotnDhm2ByMI1Shq4Y6oRJo21SGJA=\n    golang.org/x/[email protected] h1:1R38tQp22dcHpTKJPjgVa16FhlDy/kHEaCM/ndi/FIc=\n    golang.org/x/[email protected] h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=\n    honnef.co/go/[email protected] h1:nI5egYTGJakVyOryqLs1cQO5dO0ksin5XXs2pspk75k=\n    mvdan.cc/[email protected] h1:t8TAw9WgTLghti7RYkpPmqk4JtQ3+wcP5GgZqgWeWLQ=\n    mvdan.cc/xurls/[email protected] h1:NSZPykBXJFCetGZykLAxaL6SIpvbVy/UFEniIfHAa8A=\n"}}


[Trace - 15:40:27.248 PM] Sending notification 'initialized'.
Params: {}


[Trace - 15:40:27.248 PM] Received notification 'window/showMessage'.
Params: {"type":4,"message":"Loading packages..."}


[Trace - 15:40:27.248 PM] Received request 'workspace/configuration - (1)'.
Params: {"items":[{"scopeUri":"file:///tmp/tmp.hs2uMVS9fG/example.com/","section":"gopls"},{"scopeUri":"file:///tmp/tmp.hs2uMVS9fG/example.com/","section":"gopls-example.com"}]}


[Trace - 15:40:27.249 PM] Sending notification 'textDocument/didOpen'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go","version":0,"languageId":"go","text":"package main\n\nimport \"fmt\"\n\ntype T struct{}\n\nfunc (x *T) String() string {\n\treturn fmt.Sprintf(\"%T{}\", x)\n}\n\nfunc (x *T) Clone() *T {\n\ty := new(T)\n\t*y = *x\n\treturn y\n}\n\nfunc main() {}\n"}}


[Trace - 15:40:27.249 PM] Sending notification 'workspace/didChangeConfiguration'.
Params: {"settings":null}


[Trace - 15:40:27.250 PM] Sending request 'textDocument/documentSymbol - (2)'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go"}}


[Trace - 15:40:27.254 PM] Sending response 'workspace/configuration - (1)' in 5ms.
Result: [null,null]


[Trace - 15:40:27.327 PM] Received request 'client/registerCapability - (2)'.
Params: {"registrations":[{"id":"workspace/didChangeWatchedFiles-0","method":"workspace/didChangeWatchedFiles","registerOptions":{"watchers":[{"globPattern":"**/*.{go,mod,sum}","kind":7},{"globPattern":"/tmp/tmp.hs2uMVS9fG/example.com/**/*.{go,mod,sum}","kind":7}]}}]}


[Trace - 15:40:27.327 PM] Received notification 'window/logMessage'.
Params: {"type":3,"message":"2020/10/09 15:40:27 go env for /tmp/tmp.hs2uMVS9fG/example.com/\n(root /tmp/tmp.hs2uMVS9fG/example.com)\n(go version go version devel +2be7788f8 Fri Oct 9 02:49:19 2020 +0000 linux/amd64\n)\n(valid build configuration = true)\n(build flags: [])\nGOINSECURE=\nGONOPROXY=\nGOMOD=/tmp/tmp.hs2uMVS9fG/example.com/go.mod\nGOPROXY=https://proxy.golang.org,direct\nGOPATH=/tmp/tmp.hs2uMVS9fG/_gopath\nGOPRIVATE=\nGOCACHE=/usr/local/google/home/bcmills/.cache/go-build\nGONOSUMDB=\nGO111MODULE=auto\nGOFLAGS=\nGOROOT=/usr/local/google/home/bcmills/sdk/gotip\nGOSUMDB=sum.golang.org\nGOMODCACHE=/tmp/tmp.hs2uMVS9fG/_gopath/pkg/mod\n\n"}


[Trace - 15:40:27.329 PM] Sending response 'client/registerCapability - (2)' in 1ms.
Result: 


[Trace - 15:40:27.588 PM] Received notification 'window/logMessage'.
Params: {"type":3,"message":"2020/10/09 15:40:27 go/packages.Load\n\tsnapshot=0\n\tdirectory=/tmp/tmp.hs2uMVS9fG/example.com\n\tquery=[./... builtin]\n\tpackages=2\n"}


[Trace - 15:40:27.628 PM] Received notification 'window/showMessage'.
Params: {"type":3,"message":"Finished loading packages."}


[Trace - 15:40:27.629 PM] Received request 'workspace/configuration - (3)'.
Params: {"items":[{"scopeUri":"file:///tmp/tmp.hs2uMVS9fG/example.com/","section":"gopls"},{"scopeUri":"file:///tmp/tmp.hs2uMVS9fG/example.com/","section":"gopls-example.com"}]}


[Trace - 15:40:27.671 PM] Sending response 'workspace/configuration - (3)' in 41ms.
Result: [null,null]


[Trace - 15:40:27.719 PM] Received response 'textDocument/documentSymbol - (2)' in 469ms.
Result: [{"name":"T","detail":"struct{...}","kind":23,"range":{"start":{"line":4,"character":5},"end":{"line":4,"character":15}},"selectionRange":{"start":{"line":4,"character":5},"end":{"line":4,"character":6}}},{"name":"(*T).String","detail":"()","kind":6,"range":{"start":{"line":6,"character":0},"end":{"line":8,"character":1}},"selectionRange":{"start":{"line":6,"character":12},"end":{"line":6,"character":18}}},{"name":"(*T).Clone","detail":"()","kind":6,"range":{"start":{"line":10,"character":0},"end":{"line":14,"character":1}},"selectionRange":{"start":{"line":10,"character":12},"end":{"line":10,"character":17}}},{"name":"main","detail":"()","kind":12,"range":{"start":{"line":16,"character":0},"end":{"line":16,"character":14}},"selectionRange":{"start":{"line":16,"character":5},"end":{"line":16,"character":9}}}]


[Trace - 15:40:30.981 PM] Sending request 'textDocument/signatureHelp - (3)'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go"},"position":{"line":6,"character":6}}


[Trace - 15:40:30.981 PM] Sending request 'textDocument/hover - (4)'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go"},"position":{"line":6,"character":6}}


[Trace - 15:40:30.981 PM] Received response 'textDocument/signatureHelp - (3)' in 0ms.
Result: null


[Trace - 15:40:30.982 PM] Sending request 'textDocument/documentHighlight - (5)'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go"},"position":{"line":6,"character":6}}


[Trace - 15:40:30.982 PM] Received notification 'window/logMessage'.
Params: {"type":1,"message":"2020/10/09 15:40:30 no signature help: cannot find an enclosing function\n\tposition={6 6}\n"}


[Trace - 15:40:30.982 PM] Received response 'textDocument/hover - (4)' in 0ms.
Result: {"contents":{"kind":"markdown","value":"```go\nvar x *T\n```"},"range":{"start":{"line":6,"character":6},"end":{"line":6,"character":7}}}


[Trace - 15:40:30.982 PM] Received response 'textDocument/documentHighlight - (5)' in 0ms.
Result: [{"range":{"start":{"line":6,"character":6},"end":{"line":6,"character":7}},"kind":1},{"range":{"start":{"line":7,"character":28},"end":{"line":7,"character":29}},"kind":1}]


[Trace - 15:40:35.647 PM] Sending request 'textDocument/rename - (6)'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go"},"position":{"line":6,"character":6},"newName":"t"}


[Trace - 15:40:35.648 PM] Received response 'textDocument/rename - (6)' in 0ms.
Result: {"documentChanges":[{"textDocument":{"version":0,"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go"},"edits":[{"range":{"start":{"line":6,"character":6},"end":{"line":6,"character":7}},"newText":"t"},{"range":{"start":{"line":7,"character":28},"end":{"line":7,"character":29}},"newText":"t"}]}]}


[Trace - 15:40:36.160 PM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"uri":"file:///tmp/tmp.hs2uMVS9fG/example.com/main.go","version":2},"contentChanges":[{"range":{"start":{"line":7,"character":28},"end":{"line":7,"character":29}},"rangeLength":1,"text":"t"},{"range":{"start":{"line":6,"character":6},"end":{"line":6,"character":7}},"rangeLength":1,"text":"t"}]}

[muirdm]

Some questions:

1. If the receiver names aren't currently consistent, should we rename them all to be consistent?
2. If we can't rename a receiver because the new name would conflict with another name in the method body, should we fail the entire rename operation or rename as many receivers as we can?

[mattheusv]

From a user perspective, I think it's better to rename even if the receiver names are not consistent at the moment, and rename as many receivers as possible.

[jmalloc]

Some preferences from someone who just created a dupe of this issue 🤦

2. If we can't rename a receiver because the new name would conflict with another name in the method body, should we fail the entire rename operation or rename as many receivers as we can?

Fail the entire rename, so you can evaluate the conflict and decide if you still think the new receiver name is appropriate.

1. If the receiver names aren't currently consistent, should we rename them all to be consistent?

Rename them all, only if the issue above did not occur.

[adonovan]

I think this would be a relatively easy fix. Because a type may have many methods, only some of whose receivers may be safely renamed, I think the tool should be best effort: that is, it should report an error only if it cannot rename the selected receiver, but it should attempt to rename as many of the other receivers as possible.

[adonovan]

One question that came up during the implementation: should renaming any reference to a receiver cause renaming of all methods' receivers? Or should we enable users to selectively rename just a single receiver, by requiring some extra indication of intent for the larger operation: by renaming all receivers only when the rename was initiated at a receiver declaration, not just a reference?

[gopherbot]

Change https://go.dev/cl/665935 mentions this issue: gopls/internal/golang: Rename all receiver variables

[gopherbot]

Change https://go.dev/cl/666455 mentions this issue: gopls/internal/golang: Rename all receivers only at declaration