Improve prepareRename responses#4867
Conversation
The previous implementation mistakenly returns the location of the definition site for imported names. This commit ensures the returned range contains the current cursor position.
izuzu-izuzu
force-pushed
the
rename-improve-prepare-result
branch
from
27128be to
20b8e00
Compare
izuzu-izuzu
requested review from
fendor,
jvanbruegge,
michaelpj and
wz1000
as code owners
izuzu-izuzu
force-pushed
the
rename-improve-prepare-result
branch
from
6425d3d to
a3fcc23
Compare
|
Apologies for the force-pushing. I’ll stop doing that. I only squashed commits that modify |
When I made the previous commit, I only had `Rename.hs` open, and HLS did not tell me about redundant imports in `test/Main.hs`. This might be worth a new feature request.
fendor
left a comment
fendor
left a comment
There was a problem hiding this comment.
Thank you, this looks mostly good to me! One comment
| _ -> case pointCommand hieAst pos nodeSpan of | ||
| (srcSpan : _) -> InL $ PrepareRenameResult $ InL (realSrcSpanToRange srcSpan) | ||
| [] -> InR Null |
There was a problem hiding this comment.
I dont understand why we need a second pointCommand here if the only point is to get a SrcSpan, can't we get this in one step, combined with getNamesAtPoint'?
I might be missing the high level idea, is the workflow roughly:
- Is there something under the cursor
- Is there something we can rename, which it only is if the thing under the cursor has a
SrcSpan?
The SrcSpan of the Name is no good, I assume?
| prepareRenameTests = testGroup "PrepareRename" | ||
| [ testCase "Module name (not yet renameable)" $ runRenameSession "" $ do | ||
| doc <- openDoc "PrepareRename.hs" "haskell" | ||
| -- REVIEW: The wait is for consistency with 'goldenWithDoc'. Is it necessary? |
There was a problem hiding this comment.
Yes, it is necessary, as the file needs to be renamed before we can actually rename anything. Without this barrier, the request for rename might be received before we finished typechecking.
| prepareRename :: TextDocumentIdentifier -> Position -> Session (PrepareRenameResult |? Null) | ||
| prepareRename doc pos = do | ||
| let params = PrepareRenameParams doc pos Nothing | ||
| rsp <- request SMethod_TextDocumentPrepareRename params | ||
| case rsp ^. L.result of | ||
| Left rspError -> liftIO $ assertFailure $ "prepareRename failed: " <> show rspError | ||
| Right rspResult -> pure rspResult |
There was a problem hiding this comment.
Upstream to lsp-test? If yes, please open an issue in lsp, and link to it from here.
That's fine, you can (and are encouraged) to force push all you want on your PR. |
2 participants
Closes #4866
Description
prepareRenamenow returns an explicitRangeof the identifier to rename, ornullif there is no symbol at the given position.This follows the LSP 3.17 specification; it also eliminates reliance on client support for
defaultBehavior.My notes for this PR can be found in
NOTES.md.TODO
waitForBuildQueuein the new testsNOTES.mdafter this PR is reviewedhptSomeThingsBelowUswarnings