Improve handling of nonsense rename attempts

[jhrcek]

First step towards improving behavior when renaming unsupported things (e.g. keywords or words within comments).
Instead of throwing plugin error (reporte here #3915)
we detect that there is no Name to rename under cursor and throw less scary looking Invalid Param warning.

Behavior after this PR:

EDIT: Actually in the end I included @michaelpj's suggestion to implement Prepare Rename handler that "prefilters" invalid requests on the client and doesn't even allow creating rename request for them.

[jhrcek]

Most of this is indentation changes. I recommend reviewing with "Hide whitespace"

[jhrcek]

Actually it was relatively easy to implement PrepareRename handler and the behavior is now even better with it:

[soulomoon]

I think it is fine we only pick up the newest

[jhrcek]

I think this is used for combining of responses in case multiple plugins happen to handle same type of request.
I'd say it's ok to leave it like this for now, because this PR introduces this new request type and for now rename plugin is the only one that supports it.

[fendor]

Nice job!

[michaelpj]

I'm unsure if this is the correct behaviour. It's very hard to tell what the intention of the various options for results of prepareRename are. Worth a comment anyway. We could consider returning the range corresponding to the name, although it's unclear if we have that...

[jhrcek]

I would say that lsp spec is quite clear in this regard:

If { defaultBehavior: boolean } is returned (since 3.16) the rename position is valid and the client should use its default behavior to compute the rename range.

I looked at what the vscode-languageclient is doing with that response (see here) and when you return the boolean:
if true it triggers the default rename behavior (finds the range of the symbol under cursor itself, opens input box where you can specify the new name, it uses the current name of the symbol as placeholder).
if false it gives you a small error popup saying "The element can't be renamed".

Since I find the default behavior unproblematic (by pressing F2 on bunch of things you can see that vscode's default behavior correctly finds the range of symbol under cursor), I went with this Bool return type.

I'll let you read this / react and I can open a PR with some additional clarifying comment later today.

[michaelpj]

I guess I'm just wondering if we can rely on the "default behaviour" or whether it would be better for us to be returning the actual range of the name? Maybe it's fine?

[jhrcek]

I played with it for a bit and it seems it would be hard to get this right if we're to provide the Range under cursor.

The thing I had trouble to make work was qualified names.
e.g. if you rename IO.putStrLn the HieAST contains the range of the whole thing (including IO.) so if you trigger rename on it, you get the whole thing prefilled into the rename input which is confusing.

I'd be more comfortable keeping the current implementation which seems to work robustly on all cases we have in tests.

[michaelpj]

Huh, and vscode does the right thing and just changes putStrLn? IDK, seems potentially fragile but maybe it's fine.

Anyway, this all seems like super useful information, so worth writing down!