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: renaming method of an in-use interface fails #34438

Open
wtlangford opened this issue Sep 20, 2019 · 6 comments
Open

x/tools/gopls: renaming method of an in-use interface fails #34438

wtlangford opened this issue Sep 20, 2019 · 6 comments

Comments

@wtlangford
Copy link

@wtlangford wtlangford commented Sep 20, 2019

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

$ go version
go version go1.13 darwin/amd64

$ gopls version
golang.org/x/tools/gopls v0.1.6
    golang.org/x/tools/gopls@v0.1.6 h1:jXi39uPnkIKQKqLUsZqznHkHyLqTXqjMId0kTrakGMI=

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="off"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/wlangford/Library/Caches/go-build"
GOENV="/Users/wlangford/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/wlangford/src/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wc/0nx11dxn5qqb_2tjyr7pyk900000gp/T/go-build613974333=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Minimal example:
https://play.golang.org/p/sDBH3VQY5TF

Attempt to rename the X.A() method using gopls.

What did you expect to see?

X.A, and impl.A should be renamed

What did you see instead?

gopls reports an error:

Request textDocument/rename failed.
Message: internal error: during renaming of abstract method func (goplsexample.X).A() int	changedMethods=false, coupled method=func (goplsexample.impl).A() int	Please file a bug report
Code: 0 
@gopherbot gopherbot added the gopls label Sep 20, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 20, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@ianthehat
Copy link

@ianthehat ianthehat commented Sep 20, 2019

In general I am not sure that is actually viable.
There is nothing that tells us that impl.A exists solely to implement X.A, it seems kind of obvious in a small example but it is not in the larger context.
Consider for example https://play.golang.org/p/YPivBBZLhZ8
Now if you rename X.A, and it renames impl.A, impl now no longer implements Y, which maybe it is supposed to do.
Also, should it rename impl2.A when there is no evidence at all that impl2 was supposed to implement X rather than Y?

@wtlangford
Copy link
Author

@wtlangford wtlangford commented Sep 20, 2019

I don't disagree. The only reason I tried it on the interface at all was in response to a message from gopls when I tried it on the implementation (in retrospect, I probably shouldn't have expected that to work either...).

The message from gopls was

Message: renaming this method "A" to "B"	would make goplsexample.impl no longer assignable to interface X	(rename goplsexample.X.A if you intend to change both types)

Maybe that message is the actual issue, directing users to try a thing that probably isn't reasonable.

@ianthehat
Copy link

@ianthehat ianthehat commented Sep 20, 2019

I guess we should think what the ideal experience is.
Maybe when you try to rename the interface method, it should warn you about the places it breaks the code, but allow you to apply the rename anyway.
It could then look at the build breaks, and see if it could stack up some more renames that you can choose to apply to fix them again.

@stamblerre stamblerre changed the title gopls: Renaming method of an in-use interface fails x/tools/gopls: renaming method of an in-use interface fails Sep 25, 2019
@gopherbot gopherbot added this to the Unreleased milestone Sep 25, 2019
@gopherbot gopherbot added the Tools label Sep 25, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 4, 2019
@leonardochaia
Copy link

@leonardochaia leonardochaia commented Dec 22, 2019

Is there a workaround for doing this kind of rename? assuming a simple example like the one provided: just an interface and an implementation.
Acknowledging that it may break other interface implementations?

@stamblerre stamblerre modified the milestones: gopls unplanned, gopls/v1.0.0 Jan 29, 2020
@stamblerre stamblerre added the Thinking label Mar 12, 2020
@jehiah
Copy link

@jehiah jehiah commented May 12, 2020

Using the example from https://play.golang.org/p/YPivBBZLhZ8 from earlier it's not possible to rename X.A or impl.A.

$ gopls version
golang.org/x/tools/gopls 0.4.1
    golang.org/x/tools/gopls@v0.4.1-pre2 h1:X5vb657+UV0LeFGyLzTdYti9nkyllZoLaKsebuW8p1M=
$ gopls rename -w main.go:11:13 B
gopls: renaming this method "A" to "B"	would make github.com/golang/go/issues/34438.impl no longer assignable to interface X	(rename github.com/golang/go/issues/34438.X.A if you intend to change both types)
$ gopls rename -w main.go:6:2 B
gopls: internal error: during renaming of abstract method func (github.com/golang/go/issues/34438.X).A() int	changedMethods=false, coupled method=func (github.com/golang/go/issues/34438.impl).A() int	Please file a bug report

One path forward would be to allow multiple renames in the same invocation, i.e. to allow (position, newname) to be repeated. While this doesn't necesarily make it easy to do incremental rename it does make it possible to do a complete change when the interface and implementation are completely contained to the module being renamed.

$ gopls rename -w main.go:6:2 B main.go:11:13 B
@stamblerre stamblerre removed this from the gopls/v1.0.0 milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.