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

Closed
wtlangford opened this issue Sep 20, 2019 · 8 comments
Closed

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

wtlangford opened this issue Sep 20, 2019 · 8 comments
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Thinking Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@wtlangford
Copy link

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 Issues related to the Go language server, gopls. label Sep 20, 2019
@gopherbot
Copy link

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

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

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

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 This label describes issues relating to any tools in the x/tools repository. label Sep 25, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 4, 2019
@leonardochaia
Copy link

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
@jehiah
Copy link

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

@FloatingSunfish
Copy link

FloatingSunfish commented Jul 31, 2022

I suppose this is the price we pay for having structs not need to declare what interfaces they implement.
Do you think it's not too late to add an implements keyword to make such renames possible?

I personally prefer having the freedom to rename interface functions and their implementers as I please versus not needing to declare what interfaces my structs implement.
The cons weigh much more than the pros.

UPDATE: GoLand seems to support renaming interface functions and its implementers. See here.

@findleyr
Copy link
Contributor

This appears to work now, except when it doesn't (#65098). That you sometimes can't rename from the implementor is #62613.

Therefore, closing as I think this is mostly done, and the two areas where it is incomplete are covered by other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Thinking Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

9 participants