golang
x/tools/gopls: rename fails with "conflicts with var in same block" when the new identifier already exists
This is related to (but slightly different from) #41851.
What did you do?
Follow the same steps to reproduce as for #41851, but for step (1) use a declared identifier instead of an undeclared one. (https://play.golang.org/p/_L1Lkry0R0o)
What did you expect to see?
The identifier at the point, as well as all references to it, should be renamed, despite the fact that the new identifier conflicts with the existing declaration.
The resulting program should now have a redeclared in this block error (https://play.golang.org/p/IP2ZkF7qnIe), which the user may resolve by removing one or the other of the conflicting declarations.
What did you see instead?
[client-request] (id:41) Wed Oct 7 14:54:24 2020:
(:jsonrpc "2.0" :id 41 :method "textDocument/rename" :params
(:textDocument
(:uri "file:///tmp/tmp.Ih8P4GysfM/example.com/main.go")
:position
(:line 14 :character 24)
:newName "errno"))
[server-reply] (id:41) ERROR Wed Oct 7 14:54:24 2020:
(:jsonrpc "2.0" :error
(:code 0 :message "renaming this var \"errNo\" to \"errno\" conflicts with var in same block")
:id 41)
I'm not sure this would be safe behavior in general. Consider a case like:
func _(greatName int) int {
badName := 123
greatName = badName - greatName
// lots of other code
return badName
}
The user is looking at the bottom of the function and really doesn't like badName. They lsp-rename it to greatName without realizing there is already a greatName. Suddenly all the uses of badName and greatName would become indistinguishable.
What if the user doesn't notice their mistake to undo it?
I agree with @muirdm that we can't support this case by default. Maybe with a -force flag on the command-line or something like that.
Also remember that rename applies to the entire module including files not open in your editor. If the user is in the middle of a big change with many modified files then a botched rename might be impossible to undo automatically.
Looking through old issues, I came upon this.
I don't think we should do this. IMO, failing is the correct thing to do. One of the invariants we try to enforce is that refactoring operations do not break the build.
One of the invariants we try to enforce is that refactoring operations do not break the build.
I can't speak for everyone, but a build that is already deeply broken is my typical state when I'm working on a large change. I would much rather have a rename that I can use to actually rename things than one that is afraid to ever actually make the kinds of changes that I need to make. 😅
Since I've run into so many cases where gopls just refuses to make the change I need, I've mostly learned not to even bother with it, and I end up treating it as just a fancier version of goimports — and given how much effort I know has gone into making gopls work, that's quite disappointing.
(Today I usually just usequery-replace-regexp for small refactors or sed for large ones, but those are both strictly more dangerous than what gopls would do in terms of causing wide-scale breakage, and sed in particular is much harder to undo if it goes wrong.)
FWIW @adonovan remarked that gopls had lifted most of the guards against renaming inside a broken package, and was surprised that it still worked most of the time. I guess gorename was very strict about not doing anything in the presence of errors.
Sorry you can't use gopls renaming, and surprised. I use it ~all the time, and rarely have such annoyances. Perhaps we're using it differently.
I use M-x eglot-rename in Emacs all the time and rarely notice that it fails because of compilation errors. This is very useful but it does make me nervous that it has the potential to wreak havoc on my source (or simply crash) because it lacks the timidity (aka healthy respect for invariants) of gorename. Perhaps I'm unconsciously invoking eglot-rename only when I know the impact of the type errors is bounded and doesn't affect the declarations to be renamed. Happy to dig deeper into this with you.
You raise an interesting question about strictness and utility; I was mulling the same question while developing https://go.dev/cl/519715 for cmd/fix and gopls. Batch tools like cmd/fix want to be completely strict while interactive tools like gopls may need to be more aggressive even at the risk of making mistakes--so long as the mistakes are easily observed and reverted; subtle semantic changes are a bad idea for any tool. But it's very hard to know when ignoring type errors will lead to the first kind of mistake or the second.
On further thought: in theory — given complete control over the LSP protocol and some ideal UI — this could be a separate kind of refactoring from “rename”. We could call it, say, “merge variables”. 🤔
The downside of that approach is that it would complicate the UX: as a user I would have to learn yet another keybinding, or type out a much longer command by name, or figure out how to add a single keybinding that implements “rename or merge variables”.
What if we allow slaphappy renames but save off a patch of the changes to let the user "undo" if they want?
Shouldn't the IDE already be saving a patch of the changes?
Not if the files aren't open in the IDE. But that is another option - if the renames are contained to a single file, be more aggressive.
Yeah, usually I'm doing this for a variable local to a single function, so that would be viable for most of my use-cases.
IMO the ideal experience would be:
- User renames.
- gopls says "Are you sure?"
- User can abort or continue.
But in the short term having a way to do this via the gopls CLI with a -force flag or equivalent would be better than the current situation.
Also there are definitely some false positives. I was trying to rename an interface method and gopls refused, citing a different interface that it thought the new name would conflict with. Except I don't have any types that attempt to implement both interfaces, so the conflict was not real. I did a search-all for "would conflict with this method" in gopls and removed all the checks (and reinstalled and relaunched gopls) and it worked fine, except for a couple concrete implementations that didn't get renamed automatically.
I wish we could do this but the problem with inserting a prompt in the middle of a rename is that many clients will deadlock or timeout the rename call.
Is it feasible to add something like -force? It probably wouldn't be a solution for most people, but I'd be fine with running some gopls execute ... command manually.
[Rob] I wish we could do this but the problem with inserting a prompt in the middle of a rename is that many clients will deadlock or timeout the rename call. [Ethan] Is it feasible to add something like -force? It probably wouldn't be a solution for most people, but I'd be fine with running some gopls execute ... command manually.
We're (esp. @h9jiang) are working on a prototype for interactive LSP commands that can return a form to be answered before proceeding. The design is to stateless, like visiting the DMV, where you go to a random teller window (=new RPC) each time, and the teller either gives you what you asks for, or sends you away with another form to fill in. In this model, an operation could fail with a question such as "are you happy to apply force to this operation?". (Alternatively, it could be a list of locations where the user has approved force, but that would scale poorly.) The second request is not "rename x to y" but "rename x to y with force=true" (or useForceAt={a,b,c}).