Rename implementation #1878

OliverMadine · 2021-05-30T20:23:04Z

A plugin to implement workspace-wide renaming of symbols as my GSoC 2021 project!

Todo List

Example:

…enameSymbol plugin

…ucture, created CPP flag

plugins/hls-rename-plugin/test/Main.hs

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

wz1000 · 2021-05-31T21:26:23Z

Have you thought about augmenting something like retrie with the reference data to get semantics preserving transformations using exactprint?

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
         baz

naively substituting quxx for foo breaks the parse:

quux = do bar
         baz

pepeiborra · 2021-05-31T22:26:07Z

Have you thought about augmenting something like retrie with the reference data to get semantics preserving transformations using exactprint?

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
         baz
naively substituting quxx for foo breaks the parse:
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

OliverMadine · 2021-06-01T12:31:39Z

Have you thought about augmenting something like retrie with the reference data to get semantics preserving transformations using exactprint?

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
         baz
naively substituting quxx for foo breaks the parse:
quux = do bar
         baz

Yes, I agree, please consider hooking up retrie with hiedb. @xich has given 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!

expipiplus1 · 2021-06-15T03:59:57Z

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.

pepeiborra · 2021-07-07T20:05:39Z

@OliverMadine do how is this going? Do you mind sharing what you are working on?

OliverMadine · 2021-07-09T12:11:09Z

@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 MatchResultTransformer) and allows rewrites of recursive calls (since this is not an issue with rename rewrites)

The main problem now is renaming declarations themselves.
For example:

when renaming foo to bar in

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 getCompilerOptions can return all components to be loaded simultaneously, however, it looks like @fendor has been working on a much nicer solution.

Sorry for the late response. I'll also try to update you more frequently from now on.

fendor · 2021-07-09T12:42:01Z

@OliverMadine Your hacky solution is still welcome for supporting older cabal and stack versions!

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

pepeiborra · 2021-07-09T16:18:04Z

@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 MatchResultTransformer) and allows rewrites of recursive calls (since this is not an issue with rename rewrites)

The main problem now is renaming declarations themselves.
For example:

when renaming foo to bar in
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 getCompilerOptions can return all components to be loaded simultaneously, however, it looks like @fendor has been working on a much nicer solution.

Sorry for the late response. I'll also try to update you more frequently from now on.

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 haskell-language-server.cabalto be disabled by default.

pepeiborra · 2021-07-18T20:31:23Z

The lack of updates here is getting worrying. @OliverMadine Can you share the latest status?

OliverMadine · 2021-07-19T12:23:24Z

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).

pepeiborra · 2021-07-19T17:29:16Z

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

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

plugins/hls-rename-plugin/hls-rename-plugin.cabal

haskell-language-server.cabal

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

plugins/hls-retrie-plugin/src/Ide/Plugin/Retrie.hs

stack-8.6.4.yaml

stack-8.6.5.yaml

stack-9.0.1.yaml

pepeiborra · 2021-08-16T16:51:43Z

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

+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


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.

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.

OliverMadine · 2021-08-17T05:56:10Z

@pepeiborra

Any thoughts on modifying the entire parsed source directly (using SYB rather than using Retrie)?

Here’s a branch that takes this approach.
It is passing all of the tests locally.

The advantages seem to be:

Compatibility with earlier ghc versions
No issues with retrie compatibility on windows
We can rename non top-level names
Easier solution to maintain

The disadvantage would be that it is slower. Possibly significantly?
Maybe I’m missing something else?

pepeiborra · 2021-08-17T07:43:03Z

@pepeiborra

Any thoughts on modifying the entire parsed source directly (using SYB rather than using Retrie)?

Here’s a branch that takes this approach.

It is passing all of the tests locally.

The advantages seem to be:

Compatibility with earlier ghc versions

No issues with retrie compatibility on windows

We can rename non top-level names

Easier solution to maintain

The disadvantage would be that it is slower. Possibly significantly?

Maybe I’m missing something else?

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.

OliverMadine · 2021-08-17T16:21:35Z

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).

samhh · 2021-08-18T08:12:13Z

Fixed link^: #2108

Oliver Madine and others added 5 commits April 13, 2021 16:16

Added rename symbol implementation (does not work for types yet) in R…

e21bb6b

…enameSymbol plugin

renamed RenameSymbol.hs plugin to Rename.hs, cleaned initial file str…

3c57f8c

…ucture, created CPP flag

Initial rename package structure and test stucture

2c71be1

Merge branch 'haskell:master' into rename-symbol

ba0934f

removed default cabal main file, declared rename capability

966ead0

OliverMadine marked this pull request as draft May 30, 2021 20:23

initial test cases

6634bc5

Ailrun reviewed May 30, 2021

View reviewed changes

plugins/hls-rename-plugin/test/Main.hs Show resolved Hide resolved

pepeiborra reviewed May 31, 2021

View reviewed changes

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs Outdated Show resolved Hide resolved

wz1000 reviewed May 31, 2021

View reviewed changes

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs Outdated Show resolved Hide resolved

OliverMadine added 3 commits May 31, 2021 21:33

Renaming of type constructors, Renaming of qualified names

c5b6bbf

GADT test case

5058801

Merge branch 'haskell:master' into rename-symbol

a80bf10

OliverMadine added 2 commits July 8, 2021 11:46

Initial integration with retrie

fc65cbf

updated session to use getCompilerOptions

90ff9ba

use custom context updater

67a7bc5

pepeiborra reviewed Jul 9, 2021

View reviewed changes

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs Outdated Show resolved Hide resolved

Use direct reference locations

57f8c08

ignore rename tests, rebase master

db9d9cc

OliverMadine added 11 commits August 14, 2021 12:31

fix: use isQualifiedImport to test for qualified

93d41fe

bump retrie version in stack for ghc 8.6.*

545e1aa

rebase master

c247a55

code style

e9e2920

disable for ghc 8.6.5/8.6.4/9.0.1

6827544

add rename to stack 8.6.* package list

57f4b45

update nix config with plugin

373ac3d

extra test cases, test consistency, code style

abe3e2d

return response error fro ghc < 8.8

03693f2

remove response error for ghc <8.8.0

e0206ae

fix: remove second renameProvider

e1f2454

pepeiborra requested changes Aug 16, 2021

View reviewed changes

OliverMadine added 6 commits August 16, 2021 12:52

error handling, ghc version management

4197543

ghc version compatibility management

b8dd9e2

fix: missing if in .cabal

bd8692a

fix: typo in test.yaml

9c7f46b

enable in ghc 9.0.1

45b991f

remove GhcPlugins import

21b8b25

pepeiborra reviewed Aug 16, 2021

View reviewed changes

OliverMadine added 6 commits August 16, 2021 18:26

use HasSrcSpan from Ghc.Compact

1c842bd

add CPP to use new HsModule for ghc >= 9.0.1

8a247cb

fix: use CPP for all occurence of HsModule

203f78c

use OldRealSrcSpan

8f95603

code style

33756cc

update missing declaration error, disable data constructor test

5952677

OliverMadine closed this Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename implementation #1878

Rename implementation #1878

OliverMadine commented May 30, 2021 •

edited

Loading

wz1000 commented May 31, 2021 •

edited

Loading

pepeiborra commented May 31, 2021 •

edited

Loading

OliverMadine commented Jun 1, 2021

expipiplus1 commented Jun 15, 2021

pepeiborra commented Jul 7, 2021

OliverMadine commented Jul 9, 2021 •

edited

Loading

fendor commented Jul 9, 2021

pepeiborra commented Jul 9, 2021

pepeiborra commented Jul 18, 2021

OliverMadine commented Jul 19, 2021 •

edited

Loading

pepeiborra commented Jul 19, 2021

pepeiborra left a comment

pepeiborra Aug 16, 2021

OliverMadine Aug 17, 2021

OliverMadine commented Aug 17, 2021

pepeiborra commented Aug 17, 2021

OliverMadine commented Aug 17, 2021 •

edited

Loading

samhh commented Aug 18, 2021

Rename implementation #1878

Rename implementation #1878

Conversation

OliverMadine commented May 30, 2021 • edited Loading

Todo List

wz1000 commented May 31, 2021 • edited Loading

pepeiborra commented May 31, 2021 • edited Loading

OliverMadine commented Jun 1, 2021

expipiplus1 commented Jun 15, 2021

pepeiborra commented Jul 7, 2021

OliverMadine commented Jul 9, 2021 • edited Loading

fendor commented Jul 9, 2021

pepeiborra commented Jul 9, 2021

pepeiborra commented Jul 18, 2021

OliverMadine commented Jul 19, 2021 • edited Loading

pepeiborra commented Jul 19, 2021

pepeiborra left a comment

Choose a reason for hiding this comment

pepeiborra Aug 16, 2021

Choose a reason for hiding this comment

OliverMadine Aug 17, 2021

Choose a reason for hiding this comment

OliverMadine commented Aug 17, 2021

pepeiborra commented Aug 17, 2021

OliverMadine commented Aug 17, 2021 • edited Loading

samhh commented Aug 18, 2021

OliverMadine commented May 30, 2021 •

edited

Loading

wz1000 commented May 31, 2021 •

edited

Loading

pepeiborra commented May 31, 2021 •

edited

Loading

OliverMadine commented Jul 9, 2021 •

edited

Loading

OliverMadine commented Jul 19, 2021 •

edited

Loading

OliverMadine commented Aug 17, 2021 •

edited

Loading