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/cmd/gorename: un-embed field #61081

Closed
ghost opened this issue Jun 30, 2023 · 7 comments
Closed

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

ghost opened this issue Jun 30, 2023 · 7 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@ghost
Copy link

ghost commented Jun 30, 2023

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.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jun 30, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jun 30, 2023
@cherrymui
Copy link
Member

cc @golang/tools-team

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 11, 2023
@cherrymui cherrymui changed the title x/tools/cmd/gorename: un embed field x/tools/cmd/gorename: un-embed field Jul 11, 2023
@cherrymui
Copy link
Member

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
Copy link
Author

ghost commented Jul 11, 2023

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
Copy link
Member

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
Copy link
Author

ghost commented Jul 12, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants
@adonovan @gopherbot @cherrymui and others