Improve handling of nonsense rename attempts #4111
Conversation
|
|
||
| renameProvider :: PluginMethodHandler IdeState Method_TextDocumentRename | ||
| renameProvider state pluginId (RenameParams _prog (TextDocumentIdentifier uri) pos newNameText) = do | ||
| nfp <- getNormalizedFilePathE uri |
There was a problem hiding this comment.
Most of this is indentation changes. I recommend reviewing with "Hide whitespace"
jhrcek
force-pushed
the
jhrcek/improve-nonsense-rename-handling
branch
from
237d7c7 to
f20dfa5
Compare
|
Actually it was relatively easy to implement PrepareRename handler and the behavior is now even better with it:
|
| @@ -657,6 +660,10 @@ instance PluginRequestMethod Method_CodeLensResolve where | |||
|
|
|||
| instance PluginRequestMethod Method_TextDocumentRename where | |||
|
|
|||
| instance PluginRequestMethod Method_TextDocumentPrepareRename where | |||
| -- TODO more intelligent combining? | |||
| combineResponses _ _ _ _ (x :| _) = x | |||
There was a problem hiding this comment.
I think it is fine we only pick up the newest
There was a problem hiding this comment.
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.
| -- In particular it allows some cases through (e.g. cross-module renames), | ||
| -- so that the full rename handler can give more informative error about them. | ||
| let renameValid = not $ null namesUnderCursor | ||
| pure $ InL $ PrepareRenameResult $ InR $ InR $ #defaultBehavior .== renameValid |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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.