Rename implementation - #1878
Conversation
…enameSymbol plugin
…ucture, created CPP flag
|
Have you thought about augmenting something like Here is another case for you to consider(the language is full of these, you have to use exactprint to have any hope of being correct): foo = do bar
baznaively substituting quux = do bar
baz
|
Yes, I agree, please consider hooking up retrie with hiedb. @xich has given me powers to maintain and release retrie, so I can help review and land the changes |
ok, I can see why it’s important that I work with retrie. I’ll look into doing this. Thank you both for the help getting started! |
|
An alternative if the user is using a formatting program, would be to modify the AST and have the formatter reprint it. I assume that retrie is doing something similar under the hood. |
|
@OliverMadine do how is this going? Do you mind sharing what you are working on? |
Hello, I've been working on using retrie to perform the rewrites. I've pushed a basic version that manages limiting the rewrites to references (using The main problem now is renaming declarations themselves. when renaming foo n = foo (n-1) + foo (n-2)we get foo n = bar (n-1) + bar (n-2)but we expect bar n = bar (n-1) + bar (n-2)I'm looking at modifying the ast directly to do this. As for hie-bios, I had a hacky approach that assumes the hie.yaml lists all components in a multi-cradle then Sorry for the late response. I'll also try to update you more frequently from now on. |
|
@OliverMadine Your hacky solution is still welcome for supporting older cabal and stack versions! |
Fantastic, this is good progress. I left a comment on how to deal with renaming the declaration. I think that it would be a good idea to land this early version of the feature as soon as possible. If you don't want people to use it yet, change the flag in |
|
The lack of updates here is getting worrying. @OliverMadine Can you share the latest status? |
I’ve had covid for the past week so it hasn’t been my most productive… I pushed a few small changes but my main issue is still renaming declarations (I've explained why I don't think your previously suggested solution will work above). |
Hope you are all right, and thanks for the update! I'll reply inline. While we look for a solution, it would be a good idea to rebase on top of master and fix conflicts so that we can get CI running and a green set of lights. |
pepeiborra
left a comment
There was a problem hiding this comment.
Looks good! I have left a couple of comments requesting minor changes.
In addition to those, I think you need to register the plugin test suite in the GitHub CI explicitly, see https://github.com/haskell/haskell-language-server/blob/master/.github/workflows/test.yml
| updateLhsDecls :: [Location] -> RdrName -> HsModule GhcPs -> HsModule GhcPs | ||
| updateLhsDecls refs newRdrName ps@HsModule{hsmodDecls} = | ||
| ps {hsmodDecls = map (fmap renameLhsDecl) hsmodDecls} | ||
| where | ||
| renameLhsDecl :: HsDecl GhcPs -> HsDecl GhcPs | ||
| renameLhsDecl (SigD xSig (TypeSig xTySig sigNames wc)) = | ||
| SigD xSig $ TypeSig xTySig (map renameRdrName' sigNames) wc | ||
| renameLhsDecl (ValD xVal funBind@FunBind{fun_id, fun_matches = fun_matches@MG{mg_alts}}) | ||
| = ValD xVal $ funBind { | ||
| fun_id = renameRdrName' fun_id, | ||
| fun_matches = fun_matches {mg_alts = fmap (map (fmap $ renameLhsMatch newRdrName)) mg_alts} | ||
| } | ||
| renameLhsDecl (TyClD xTy dataDecl@DataDecl{tcdLName, tcdDataDefn = hsDataDefn@HsDataDefn{dd_cons}}) | ||
| = TyClD xTy $ dataDecl { | ||
| tcdLName = renameRdrName' tcdLName | ||
| } | ||
| renameLhsDecl (TyClD xTy synDecl@SynDecl{tcdLName}) | ||
| = TyClD xTy $ synDecl { | ||
| tcdLName = renameRdrName' tcdLName | ||
| } | ||
| renameLhsDecl decl = decl |
There was a problem hiding this comment.
Did you consider SYB for doing this rewrite, just like wingman does?
That way, if newer GHC versions change the AST, this code will still work.
There was a problem hiding this comment.
I think this would be a little difficult since there are a lot of names in the decls that we do not want to rename (either because retrie is already renaming the given reference or we don't support renaming for the given type of name).
However, I could take this approach (i.e. modifying the entire source directly and not using retrie). See my comment below.
|
Any thoughts on modifying the entire parsed source directly (using SYB rather than using Retrie)? Here’s a branch that takes this approach. The advantages seem to be:
The disadvantage would be that it is slower. Possibly significantly? |
I like it. Retrie uses SYB as well under the hood, so I don't expect this solution to be slower. A rename is the trivial rewrite where both the lhs and the rhs are a simple name, so we don't really need the full power of retrie. |
|
I'm opening a new PR due to the change of approach (and to clean the mess of commits I made while learning how to make the test cases consistent). |
|
Fixed link^: #2108 |
A plugin to implement workspace-wide renaming of symbols as my GSoC 2021 project!
Todo List
Example:
