x/tools/cmd/gorename: Renaming Interface Functions Does Not Affect Implementations #54217

FloatingSunfish · 2022-08-03T05:29:46Z

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

go version go1.19 windows/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Admin\AppData\Local\go-build
set GOENV=C:\Users\Admin\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\Admin\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Admin\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.19
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\Admin\AppData\Local\Temp\go-build3223575907=/tmp/go-build -gno-record-gcc-switches

What did you do?

I have the below sample code:

package main

import "fmt"

type animal interface {
    makeSound()
}

type dog struct {
    name string
}

func (d *dog) makeSound() {
    fmt.Printf("%s: bark! bark!", d.name)
    fmt.Println()
}

func main() {
    dog1 := &dog{
        name: "dog1",
    }

    patHead(dog1)
}

func patHead(a animal) {
    a.makeSound()
}

I then renamed animal.makeSound() to animal.makeNoise().

What did you expect to see?

I expected that all users of the interface as well as its implementations would be affected by the rename.

What did you see instead?

The users of the interface were affected by the rename, which is correct:

type animal interface {
    makeNoise()
}

func patHead(a animal) {
    a.makeNoise()
}

However, its implementations remained untouched which resulted in a compiler error:

type dog struct {
    name string
}

func (d *dog) makeSound() {
    fmt.Printf("%s: bark! bark!", d.name)
    fmt.Println()
}

func main() {
    dog1 := &dog{
        name: "dog1",
    }

    patHead(dog1)
}

Error:

cannot use dog1 (variable of type *dog) as animal value in argument to patHead: *dog does not implement animal (missing method makeNoise)

And because of the above compiler error, I can't perform any further refactorings (e.g. renaming dog.makeSound() to dog.makeNoise) to fix the error.
Currently, the only workaround is to manually fix all broken function calls across all interface implementations, which is very tedious.

More Details:

GoLand supports renaming interface functions and affecting their implementations as documented here.

If the expected behavior above is not currently supported, I think it would make sense for gorename to rename both users and implementations of a renamed interface function because the current behavior breaks existing code.
It would make more sense for the rename to affect everything it needs to so no code gets broken.

This way, from the programmer's perspective, renaming an interface function will be as simple as renaming a class method, where gorename doesn't need to be told what to affect and what to leave alone.
It renames everything it needs to prevent compiler errors.

I also posted this on Stack Overflow, and there are some discussions here.

gorename should probably check where the interface is used so the rename can affect all types passed as that interface.

Alternatively, if there's no easy solution to this problem, then perhaps a fully-optional implements keyword can be added to Go so renaming interface functions will be as straightforward as in other languages.

The text was updated successfully, but these errors were encountered:

dmitshur · 2022-08-03T23:39:25Z

Is this about cmd/gorename specifically, or more broadly one of the packages that gopls uses?

CC @golang/tools-team.

FloatingSunfish · 2022-08-13T08:39:23Z

More discussions here.

joerdav · 2022-08-26T08:49:49Z

I'm happy to build an implentation for this, however I think I have the shared worry with others that there could be uninteded renames in the unlikely event that a type implements the interface unintentionally.

I thing the solution of an implements keyword is also a large language change for the sake of a refactoring tool, a larger proposal would have to be made I think to justify removing Go's implicit interfaces.

FloatingSunfish · 2022-09-04T03:22:12Z

@joerdav

I'm happy to build an implentation for this, however I think I have the shared worry with others that there could be uninteded renames in the unlikely event that a type implements the interface unintentionally.

This is the biggest blocker in this issue. I think any refactoring should be smart enough to only affect structs that are currently being used for the interface.
Since Go's tools refuse to run when there are compiler errors, we are guaranteed that there are no compiler errors at the time of refactoring, and the goal is for that to stay the same after refactoring.

The refactoring tool should only affect structs that are currently being used for the interface.
Structs that happen to implement an interface but aren't being used for said interface should remain unchanged.

Currently, Go's compiler is smart enough to flag compiler errors when structs no longer implement an interface as a result of a rename, which gives a list of structs that should be affected by said refactoring.

adonovan · 2022-09-04T13:50:02Z

The refactoring tool should only affect structs that are currently being used for the interface.

That is the intended current behavior of gorename: it uses the satisfy package to discover assignments such as patHead(dog1) in which the *dog concrete type is assigned to an animal interface, and thereby builds a set of types whose methods must all be renamed together. If the renaming was initiated on an interface method, all the methods are renamed. If it is initiated on a concrete method, the tool reports an error that the renaming would break the interface satisfaction constraint.

And that's exactly the behavior I observe using your test case and the gorename at tip:

$ cat go.mod 
module golang.org/x/tools
...etc...

$ cat t/t.go
package main

import "fmt"

type animal interface {
    makeSound()
}
...etc...

$ go run ./cmd/gorename -from '"golang.org/x/tools/t"'.animal -to mammal
Renamed 2 occurrences in 1 file in 1 package.

$ go run ./cmd/gorename -from '"golang.org/x/tools/t"'.mammal.makeSound  -to bark
Renamed 3 occurrences in 1 file in 1 package.

$ go run ./cmd/gorename -from '"golang.org/x/tools/t"'.dog.bark  -to makeSound
t/t.go:13:15: renaming this method "bark" to "makeSound"
t/t.go:5:6: 	would make *golang.org/x/tools/t.dog no longer assignable to interface mammal
t/t.go:6:2: 	(rename golang.org/x/tools/t.mammal.bark if you intend to change both types)

$ go build ./t && echo OK
OK

FloatingSunfish · 2022-09-05T00:04:56Z

@adonovan
Hmm, could this be a vscode-go issue then instead of a gorename issue?

By your post, it seems that the gorename tool works as intended when you refactor from the command line.
This issue I reported happens when you do the same refactoring through VS Code, which I assumed uses Go's official tools behind the scenes.

Not sure who to tag from the vscode-go team for this.
If anyone knows, please tag them here so they can see this issue as well.

adonovan · 2022-09-08T16:51:51Z

Yes, it could be. Could you try the same renaming from the command line and see if it works? That would tell us for sure. Thanks.

FloatingSunfish · 2022-09-09T00:58:10Z

@adonovan
Unfortunately, I can't seem to get gorename to work on Windows' command line.
Not sure if this issue is the cause.

Anyone who can get a proper gorename command to run on Windows, please reply your commands here for reference.

adonovan · 2023-11-12T16:30:39Z

I just tested this renaming in gopls (not the obsolete cmd/gorename) from Emacs and VS Code and it works correctly in both. Closing as not reproducible.

gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 3, 2022

gopherbot added this to the Unreleased milestone Aug 3, 2022

dmitshur changed the title ~~x/tools/gorename: Renaming Interface Functions Does Not Affect Implementations~~ x/tools/cmd/gorename: Renaming Interface Functions Does Not Affect Implementations Aug 3, 2022

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 3, 2022

FloatingSunfish mentioned this issue Sep 5, 2022

Renaming Interface Functions Does Not Affect Implementations golang/vscode-go#2438

Closed

adonovan added the Refactoring Issues related to refactoring tools label Apr 24, 2023

adonovan closed this as completed Nov 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/tools/cmd/gorename: Renaming Interface Functions Does Not Affect Implementations #54217

x/tools/cmd/gorename: Renaming Interface Functions Does Not Affect Implementations #54217

FloatingSunfish commented Aug 3, 2022 •

edited

dmitshur commented Aug 3, 2022

FloatingSunfish commented Aug 13, 2022

joerdav commented Aug 26, 2022 •

edited

FloatingSunfish commented Sep 4, 2022 •

edited

adonovan commented Sep 4, 2022 •

edited

FloatingSunfish commented Sep 5, 2022 •

edited

adonovan commented Sep 8, 2022

FloatingSunfish commented Sep 9, 2022 •

edited

adonovan commented Nov 12, 2023

x/tools/cmd/gorename: Renaming Interface Functions Does Not Affect Implementations #54217

x/tools/cmd/gorename: Renaming Interface Functions Does Not Affect Implementations #54217

Comments

FloatingSunfish commented Aug 3, 2022 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

More Details:

dmitshur commented Aug 3, 2022

FloatingSunfish commented Aug 13, 2022

joerdav commented Aug 26, 2022 • edited

FloatingSunfish commented Sep 4, 2022 • edited

adonovan commented Sep 4, 2022 • edited

FloatingSunfish commented Sep 5, 2022 • edited

adonovan commented Sep 8, 2022

FloatingSunfish commented Sep 9, 2022 • edited

adonovan commented Nov 12, 2023

FloatingSunfish commented Aug 3, 2022 •

edited

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

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

joerdav commented Aug 26, 2022 •

edited

FloatingSunfish commented Sep 4, 2022 •

edited

adonovan commented Sep 4, 2022 •

edited

FloatingSunfish commented Sep 5, 2022 •

edited

FloatingSunfish commented Sep 9, 2022 •

edited