Rename refactoring capablities v1

[wclr]

This PR adds rename refactoring capabilities to LS. It's supposed to support renaming of all kinds of exported identifiers (potentially even type classes with members 🤪), though due to the purs ide usage API issues it is limited when used with the current compiler version. Anyway it is still capable of renaming value/function identifiers and type constructors, though types and operators can not be renamed.

I actually have ready updates for the compiler to fix those issues with the usage API.

Tests are in test\Rename.purs - all tests pass only with the updated compiler version with usages api fixed.

This doesn't currently support renaming of local/private identifiers.

This PR is based on #194 (tidy formatting PR). All the changes referred to refactoring are in the single latest commit (yet).

[nwolverson]

I believe the () check is related to operators or suchlike, why is this being changed? Is there a specific case that seems to be handled incorrectly?

Regarding the constructor import comment, that is something that will be addressed shortly when I revive the changes for recent purs versions, where mostly this code should be less required.

[wclr]

Yes, you are right! Mia culpa. Have fixed it. I changed it when I was dealing with this problem of finding Type vs Constructor with the same name and I forgot about operators here (that is why we need more dev comments in the code explaining such things and/or tests -).

What do you mean by "will be addressed shortly", is there a PR or something that is going to deal with it in the compiler?

[nwolverson]

The compiler change is to allow filtering by import lists, so (when looking for a specific ident) this code shouldn't be needed. And for not specific ident it doesn't matter so much anyway. That is landed already but the LSP changes are somewhere on machine and stale - will try dig that up this week

…

On Mon, 22 May 2023, 13:53 Alex, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/IdePurescript/Modules.purs <#196 (comment)> : > getUnqualActiveModules :: State -> Maybe String -> Array String getUnqualActiveModules state ident = getModules include state where include (Module { qualifier: Just _ }) = false - include (Module { importType: Explicit idents }) = maybe false - (\x -> x `elem` idents || ("(" <> x <> ")") `elem` idents) Yes, you are right! Mia culpa. Have fixed it. I changed it when was dealting with this problem of finding Type vs Constructor with the same name and didn't think about operators (that is why we need more dev comments in the code explaining such things -). What do you mean by "will be addressed shortly", is there a PR or something that is going to deal with it in the compiler? — Reply to this email directly, view it on GitHub <#196 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVEPSYGBOLPWFM653K5WZ3XHOR3FANCNFSM6AAAAAAYJFRNUU> . You are receiving this because you commented.Message ID: ***@***.*** com>

[wclr]

Right, I too was working on this problem in finding references. Currently, in the rename refactor logic, I do the following: get all definitions for a target X ident, if is X happens to be a type and constructor purs ide returns two definitions. Then, I find usages for both definitions and choose one that contains the target ident position.

Another solution (that I tried to implement, but not finished yet) would be to determine on LS side if the target X is type or constructor (by traversing parsed module) then pass eligible modules to filter. But, this also requires additional logic to correctly analyze the results returned by purs ide.

I wonder how you have dealt with the issue.

[wclr]

@nwolverson Have you checked this one?

[nwolverson]

This is still blocked on the changes I referenced before, it turns out there was some remaining work to be done there, which I started but have not finished.

[nwolverson]

Right, sorry for the delay, can you look at reconciling this with main now.

The main change I have is that getTypeInfo is no longer used directly: the dance with getUnqualActiveModules etc was required to try and get currently imported identifiers, and was not entirely accurate, whereas with the new imports filter purs ide does this work for us.

getTypeInfoWithImportFilter is doing this, but getTypeInfoMaybeNew is (with a terrible name that seems to have stuck) choosing to use the old or new approach based on purs version

[wclr]

You may check!

[nwolverson]

Thanks, checking it out now

[nwolverson]

I've not really looked into the API, what's the difference between The element can't be renamed and prepareRename failed with message: Declaration not found" - I got the former trying to rename a binding I added that hadn't successfully saved with yet, while the not found is a non-exported one.

Is it normal to rename invalid identifiers successfully and the user asking for a stupid thing gets a stupid result and can hit undo? Quick test with the JS language suggests so.

After a quick play this feels pretty great!

[wclr]

The element can't be renamed standard error message when prepareRename returns nothing.

Declaration not found is the actual error returned by purs ide (for non-exported identifiers), it is thrown, I decided not to obscure it.

Is it normal to rename invalid identifiers successfully and the user asking for a stupid thing gets a stupid result and can hit undo?

I don't get the particular scenario, can you elaborate? There where cases when may rename work strange, though I didn't encounter something that lead to really invalid code (at least I was able to undo it), it is probably due to that purs ide works only with valid compiled code, and I know that rename refactoring is valid and safe to do on compiled and saved modules.

I think particular bad/strange case should be gathered and addressed if possible (in future).