x/tools/gopls: rename incorrectly renames multiple function definitions

[thanethomson]

gopls version

golang.org/x/tools/gopls v0.13.2
    golang.org/x/tools/gopls@v0.13.2 h1:Pyvx6MKvatbX3zzZmdGiFRfQZl0ohPlt2sFxO/5j6Ro=

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/thane/.cache/go-build'
GOENV='/home/thane/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/thane/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/thane/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/linuxbrew/.linuxbrew/Cellar/go/1.21.0/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/linuxbrew/.linuxbrew/Cellar/go/1.21.0/libexec/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/home/thane/work/informal/cometbft/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build508101450=/tmp/go-build -gno-record-gcc-switches'

What did you do?

With both the Neovim LSP and VS Code, both using gopls as my language server, I've attempted to rename this function from SetCompanionRetainHeight to SetCompanionBlockRetainHeight.

I cannot find a minimal way to reproduce this bug unfortunately.

What did you expect to see?

The function to be renamed, as well as all references to the function.

What did you see instead?

The function was renamed, but the function above it (checkHeightBound) was also renamed to SetCompanionBlockRetainHeight - incorrectly, of course. All prior references to checkHeightBound are now also renamed to SetCompanionBlockRetainHeight.

Not only this, but some prior references to SetCompanionRetainHeight still remain.

Editor and settings

Neovim config:

local nvim_lsp = require('lspconfig')
nvim_lsp.gopls.setup{
  settings = {
    gopls = {
      gofumpt = true
    }
  }
}

VS Code config is set to the defaults.

Logs

VS Code logs: https://gist.github.com/thanethomson/ad3fb91847a4cb84e281a4322e971667

[findleyr]

Woah. I've never seen a bug like this, but can confirm that it is reproducible in this repository. Bisecting / investigating.

[findleyr]

On a hunch, I grepped for method declarations in test packages: indeed export_test.go declares methods on Pruner. That's almost certainly the cause: a method index from one package graph is likely being incorrectly used in a different package graph. That would also explain why this failure mode is not frequently observed.

Of course, this is still a (bad) bug.

[findleyr]

Thanks very much for reporting this bug. I've minified the repro in https://go.dev/cl/521121, which contains a failing test.

[gopherbot]

Change https://go.dev/cl/521121 mentions this issue: goplls/internal/lsp/source: renaming methods in test variants

[findleyr]

Ok, I've narrowed down the problem, and it's not entirely trivial to solve. I will work on it later this week. Summary of investigation:

• the code finds the widest package containing the usage, in order to capture usages in test files
• for exported methods, the code computes the object path using this package <-- bug here
• the code uses this object path to apply an indexed renaming; but this is a bug as an object path computed from a test variant may not be correct for a non-test variant, as method indexes may change

To fix, we should always compute object paths using the narrowest package, and should treat renaming in test variants as a (separate) local task.

[findleyr]

CC @adonovan for awareness, since he has some experience with this tricky logic (but not to fix! I'll fix :) )