Rename only if the current module compiles (#3799) #3848
Conversation
There was a problem hiding this comment.
The changes look good to me!
Can we add a test case for this scenario?
E.g. start with a well-typed module, wait for some diagnostics, modify it such that compilation fails, wait for diagnostics, fix the compile error and request to rename an identifier.
I think, the test case should be flaky (e.g. sometimes fail) without this patch and always succeed with this patch applied.
|
I've added a new test for |
| handleGetHieAst state nfp = | ||
| fmap (first removeGenerated) $ runActionE "Rename.GetHieAst" state $ useWithStaleE GetHieAst nfp | ||
| fmap removeGenerated $ runActionE "Rename.GetHieAst" state $ useE GetHieAst nfp |
There was a problem hiding this comment.
| fmap removeGenerated $ runActionE "Rename.GetHieAst" state $ useE GetHieAst nfp | |
| -- We explicitly do not want to allow a stale version here - we only want to rename if | |
| -- the module compiles, otherwise we can't guarantee that we'll rename everything, | |
| -- which is bad (see https://github.com/haskell/haskell-language-server/issues/3799) | |
| fmap removeGenerated $ runActionE "Rename.GetHieAst" state $ useE GetHieAst nfp |
|
@sgillespie Do you have the time capacity to push this over the finish line? It looks like, resolving merge conflicts and updating the comment should be enough :) |
Prefer `useE` over `useWithStaleE`
|
@fendor I'm sorry I missed everything way back in Nov-Jan. In any case, I've updated from master and added the comment. |
This PR resolves #3799 by using
useErather thanuseWithStaleE. I did not implement the suggestion to have two modi, for local renames only and for cross module renaming, rather I just fixed the issue raised in the issue.