x/tools/cmd/gorename: un-embed field

[ghost]

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

go version go1.20 windows/amd64

What did you do?

if I have this code:

package alfa

type Bravo int

type Charlie struct {
   Bravo
}

and I run this command:

gorename -from '\".\".Charlie.Bravo' -to _Bravo

What did you expect to see?

package alfa

type Bravo int

type Charlie struct {
   _Bravo Bravo
}

What did you see instead?

package alfa

type _Bravo int

type Charlie struct {
    _Bravo
}

I get that both use cases are valid, but I would say the one I am expecting is the "more correct", given the context. If someone wanted to rename the embedded type, they could just call it directly.

[cherrymui]

cc @golang/tools-team

[cherrymui]

Maybe a workaround is to first edit the file to manually un-embed, to

type Charlie struct {
   Bravo Bravo
}

Then rename the field?

Un-embedding isn't a simple rename, as methods and (sub)fields will no longer be forwarded, and a simple renaming would not fix them. Maybe this is beyond the scope of gorename?

[ghost]

the purpose of gorename I would argue is refactoring, which this is. yes I understand everything has a scope, but I feel this is in scope for the tool. we have three possible wants for this situation:

1. change embedded type only
2. change embedded field name only
3. change both

currently only 3 is possible, but not 1 or 2. if my suggestion was implemented, then all three would be possible. I think telling the user "just edit it yourself" kind of goes against the whole idea of a refactor tool. if the code change is extremely onerous, sure lets leave it to some third party. but it doesn't seem that you've done the research to make that determination.

[adonovan]

The behavior is intentional; anything less would be a bug. Gorename (and similar logic in gopls) goes to some effort to discover which declarations are coupled, meaning that if one is renamed, then the other must be renamed too, or else the behavior of the program may change.

If you want the type and the field to have separate names, then you need to un-embed the field first, which may or may not itself be a breaking change.

we have three possible wants for this situation:

1. change embedded type only
2. change embedded field name only
3. change both currently only 3 is possible, but not 1 or 2.

Only option 3 preserves the behavior of the program. Options 1 and 2 have the potential to introduce compilation errors.

I think telling the user "just edit it yourself" kind of goes against the whole idea of a refactor tool. if the code change is extremely onerous, sure lets leave it to some third party. but it doesn't seem that you've done the research to make that determination.

On the contrary, the tool does the only sensible thing: a renaming should not break your build. If you don't like its effects, you always have the option undo the renaming (by running gorename again), then make some preparatory changes (such as un-embedding the field) and running the renaming again. A renaming that broke your build would deny you this option because you couldn't even undo it, as renaming requires a well-typed input.

[ghost]

On the contrary, the tool does the only sensible thing: a renaming should not break your build.

its too bad that you closed the issue without even giving a chance for me to post a reply, so it gives the appearance that you aren't interested in one. At any rate, the entire premise of the previous comment is flawed, as is the comment by the other poster. Yes, a naive implementation might result in an error. but a smart renamer could take code like this:

package alfa

import "fmt"

type Bravo struct {
   Charlie int
}

type Delta struct {
   Bravo
}

func (d Delta) Echo() {
   fmt.Println(d.Charlie)
}

and produce this, resulting in no errors:

package alfa

import "fmt"

type Bravo struct {
   Charlie int
}

type Delta struct {
   _Bravo Bravo
}

func (d Delta) Echo() {
   fmt.Println(d._Bravo.Charlie)
}

[adonovan]

In your example, be aware that un-embedding Delta.Bravo causes Delta to lose any methods that it obtained from Bravo. This could change the set of interfaces that it implements, breaking the build, so the tool would need to detect that. Worse, it could cause Delta to implement the same interfaces as before but with different implementations (via some other field), causing a subtle change in behavior; so a tool would need to detect that too. Let's assume we are able to solve those problems.

Fundamentally, the gorename command and the LSP textDocument/rename RPC are batch operations. They simply can't interrogate the user for every embedded struct field and offer the user the choice between renaming and un-embedding the field. So, they do the simplest thing: preserve the existing type and resolution structure of the program, merely with different names.

So what is a smart batch renamer to do?

its too bad that you closed the issue without even giving a chance for me to post a reply, so it gives the appearance that you aren't interested in one.

I'm sorry if I gave that appearance. I'm quite happy to discuss this with you, but I'm just trying to be realistic that I don't think this is a bug, nor can it be fixed.