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.

Sign up for GitHub

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

Jump to bottom

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

Open
thanethomson opened this issue Aug 18, 2023 · 8 comments
Open

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

thanethomson opened this issue Aug 18, 2023 · 8 comments
Assignees
@findleyr
Labels
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
gopls/v0.16.0

Comments

@thanethomson
Copy link

thanethomson commented Aug 18, 2023 •
edited

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
@thanethomson thanethomson added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Aug 18, 2023
@gopherbot gopherbot added this to the Unreleased milestone Aug 18, 2023
@findleyr
Copy link
Contributor

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

@findleyr
Copy link
Contributor

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
Copy link
Contributor

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

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.14.0 Aug 21, 2023
@gopherbot
Copy link

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

@findleyr findleyr self-assigned this Aug 21, 2023
@findleyr
Copy link
Contributor

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
Copy link
Contributor

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

@adonovan adonovan added the Refactoring Issues related to refactoring tools label Aug 31, 2023
@findleyr findleyr modified the milestones: gopls/v0.14.0, gopls/v0.14.1 Oct 11, 2023
@soypat
Copy link

soypat commented Nov 12, 2023

I came across a similar bug. In the following codebase renaming ControlBlock.Snd to Send. it renamed validateIncomingSegment to Send as well. Restarting VSCode and the language server did not fix the issue. Reinstalling gopls with latest fixed it.

soypat/seqs@ace88a3

@findleyr findleyr modified the milestones: gopls/v0.14.2, gopls/v0.15.0 Nov 14, 2023
@findleyr findleyr modified the milestones: gopls/v0.15.0, gopls/v0.16.0 Jan 17, 2024
@findleyr
Copy link
Contributor

Hmm, the reported case was incidentally fixed by https://go.dev/cl/534139. Not convinced this is fixed in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@findleyr findleyr

Labels
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.
Projects
None yet
Milestone
gopls/v0.16.0
Development

No branches or pull requests
5 participants
@thanethomson @adonovan @gopherbot @soypat @findleyr