x/tools/gopls: rename fails with "conflicts with var in same block" when the new identifier already exists

[bcmills]

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)

[muirdm]

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.

[bcmills]

... that's what undo is for‽

[stamblerre]

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.

[muirdm]

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.

[findleyr]

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.

[bcmills]

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.)

[findleyr]

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.

[adonovan]

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.

[bcmills]

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”.

[muirdm]

What if we allow slaphappy renames but save off a patch of the changes to let the user "undo" if they want?

[bcmills]

Shouldn't the IDE already be saving a patch of the changes?

[muirdm]

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.

[bcmills]

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.

[findleyr]

Rename edits should result in the files being opened by the client.