x/tools/refactor/rename: renaming struct should rename method receivers that follow naming advice

[myitcv]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.4 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/mgo/src/modelogiq.com/g/_vendor:/home/myitcv/mgo"
GORACE=""
GOROOT="/home/myitcv/gos"
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build108524765=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

Using commit fcfba28e23c7bbd8474b355ca7d6a9d88afcca00 of golang.org/x/tools/refactor/rename

What did you do?

Starting with code:

// main.go

package main

import (
        "fmt"
)

type banana struct{}

func (b *banana) doit() {}

func main() {
        fmt.Println("Hello, playground")
}

Invoke:

gorename -offset main.go:#51 -to apple

What did you expect to see?

rename should also rename the receiver of the method doit from b -> a

The heuristic here should be that if the receiver does follow the generally accepted naming advice then rename can safely proceed and rename receivers.

What did you see instead?

Renaming banana -> apple leaves the receiver of method doit as b

[myitcv]

@cznic just noticed you gave this a 👎

What's your thinking behind this not being the right thing to do?

[cznic]

If I command some tool to rename a particular identifier x to y I want it to do exactly and precisely that or tell me why it cannot do it. I never ever want any magic involved unless I specifically ask for magic, which I basically never do nor want. If I want to also rename the receiver then I want to explicitly ask for one more rename, and then again, I want to have full control over the new receiver name even though the magic one could be the same accidentally. But once in a while I may want a different name for the reveiver and if the magic is there unconditionally, the tool is broken and usable no more.

[cespare]

I agree that renaming a type shouldn't rename the method receivers (it does feel too "magical", to borrow @cznic's word) but it would be useful for rename (or some other tool) to have the ability to rename the receiver of all methods of a type from x to y. From reading the gorename docs, it seems like you'd currently have to do a separate rename on every method.

[myitcv]

@cznic thanks, not unsurprising feedback now that I think about it.

I agree that just because I happen to find myself, more often than not, in the situation where I do want receivers that follow the generally accepted naming advice to also be renamed, doesn't preclude either:

• there being situations when I don't want this behaviour
• this not being the predominant position of everyone

On reflection, making this (the renaming of receivers) the default behaviour would almost certainly be wrong.

Presumably putting it behind a flag would work?

@cespare yes I agree that at present this is only possible via require multiple renames... which need to be done in serial and hence take a not insignificant amount of time for types with many receivers (if we ignore for one second that I'm not aware of a wrapping program that coordinates the renames). My thinking in pushing this back to rename was therefore that we benefit from a single run of types analysis.

But maybe my assumption that this would regularly be of use to the wider community is invalid?

[Dr-Terrible]

But maybe my assumption that this would regularly be of use to the wider community is invalid?

I found myself in the same boat: lot of receivers to rename that I cannot easily automated with rename, thus consuming a not insignificant amount of my time.

Presumably putting it behind a flag would work?

A flag is good enough for me, I really don't mind, provided rename offers a faster solution for these king of scenarios :)

[anacrolix]

I agree with @cznic: The behaviour should not be automatic. However I do desire to rename a bunch of receivers automatically. How can I do that with the tool currently? I'm trying something like:

$ gorename -from '"github.com/libp2p/go-libp2p-kad-dht".*::dht' -to node
gorename: -from "\"github.com/libp2p/go-libp2p-kad-dht\".*": invalid expression

And also:

$ gorename -from '"github.com/libp2p/go-libp2p-kad-dht"::dht' -to node
gorename: ambiguous specifier dht matches var at handlers.go:293:7, var at dht_net.go:47:7, var at routing.go:647:7, var at routing.go:456:7, var at dht.go:256:7, var at records.go:98:7, var at routing.go:570:7, var at dht.go:367:7, var at handlers.go:237:7, var at dht_bootstrap.go:113:7, var at dht.go:307:7, var at dht_bootstrap.go:73:7, var at routing.go:38:7, var at dht.go:168:7, field at query.go:24:2, var at dht.go:372:7, var at dht_test.go:171:3, var at dht.go:275:7, var at handlers.go:209:7, var at dht.go:111:2, field at dht_net.go:196:2, var at dht_test.go:767:10, var at dht.go:362:7, var at handlers.go:89:7, var at routing.go:151:7, var at dht.go:73:2, var at dht_net.go:158:7, var at dht.go:376:7, var at routing.go:113:7, var at dht_test.go:819:10, var at notif.go:45:2, var at handlers.go:47:7, var at handlers.go:341:7, var at dht_test.go:603:9, var at dht_net.go:51:7, var at dht_test.go:700:9, var at dht.go:144:7, var at query.go:42:7, var at routing.go:398:7, var at dht.go:227:7, var at dht.go:246:7, var at routing.go:273:7, var at dht_net.go:113:7, var at routing.go:467:7, var at notif.go:17:2, var at dht_net.go:149:7, var at records_test.go:46:2, var at dht_bootstrap.go:99:7, var at dht_test.go:779:3, var at routing.go:474:7, var at handlers.go:28:7, var at dht_test.go:689:9, var at lookup.go:56:7, var at dht.go:332:7, var at notif.go:77:2, var at routing.go:438:7, var at dht.go:326:7, var at records_test.go:20:2, var at dht.go:269:7, var at dht_test.go:797:22, var at dht_test.go:712:9, var at routing.go:253:7, var at dht.go:99:2, var at handlers.go:242:7, var at dht_test.go:569:10, var at dht_net.go:136:7, var at dht_bootstrap.go:48:7, var at dht.go:203:7, var at records.go:77:7, var at dht_bootstrap.go:127:7, var at handlers.go:148:7, var at dht.go:285:7, var at dht_test.go:459:2, var at records.go:26:7

[lbordowitz]

What about a style guide? An optional receiver style map could be written or generated which maps given types to their respective, unique receiver name. Whether during a refactor-rename, or even during the ordinary go fmt command, it could ensure that all receivers are consistently named.