Cross Module renaming

[vidit-od]

Fixes #4816

This PR aims at implementing Cross Module Renaming. Recently Cross Module Renaming was made True by default in HLS, but current implementation shows Renaming of an exported name is unsupported and Renaming of an imported name is unsupported when attempting to do so.

The main reason for this was the failWhenImportOrExport function blocking rename whenever any imported or exported variables are renamed.

The current implementation carefully checks and distinguish bw local , exported, imported variables. If cross Module is disabled. Blocks renaming imported and exported variables renaming. The older constrain of having an Explicit export list is still maintained.

The below comment ( link ) also talks about a scenario where relying completely on HieDb can lead to incorrect results when local references and imported/ exported references have same name. Hence Ast is also referred for renaming of local variables.

[vidit-od]

This PR aims at implementing cross module renaming.

It is ready for review and most of the functionality has been check by me manually. Raising this PR to see how the tests perform on various environments.

Will add testcases for this Feature once we can verify old ones work properly.

[fendor]

Could you briefly explain the strategy of the solution? I was under the impression, that the rename feature could entirely rely on hiedb to find references to a Name and replacing them, but it seems like you are doing quite a lot more with the GHC AST directly?

[vidit-od]

Reason for using AST

Implementation with HieDb :

Here changing the local variable leads to changing of all other variables of same names (Local and Global). The reason for this is HieDB uses OccName for finding references. AST presents absolute truth and should be used to rely on for preventing inconsistencies.

This similar break can be observed in any and all examples where a local variable has same name as imported variable. Although this is a very specific example of : Importing a variable -> having a same name variable as a local variable -> user triggers rename on that variable.

current implementation enables :

IMP : HieDB does cover all of the simpler cases. considering that AST traversal takes comparatively longer time we have to choose b/w faster but little inconsistent implementation vs slower but always correct one ; both version of codes are available with me :)

Hopefully this makes everything clear ! @fendor

[wz1000]

get this from the ModSummary, will allow you to get rid of the Nothing case.

[vidit-od]

i am not sure if this is logical / why this is happening but i tried doing what the review recommends by replacing the highlighted code with this code

currentModule = ms_mod $ pm_mod_summary pm
                isLocallyDefined name =
                    case nameModule_maybe name of
                        Nothing -> True
                        Just nameModule -> nameModule == currentModule             

doing this made a lot of testcases flaky. i have tried running almost all of the testcases numerous times and find no logic to why they fail. below is the list of testcases that showed flaky nature;

• "Data constructor with fields"
• "Function name"
• "Realigns do block indentation"

i dont know why this would happen but for all of the above cases we randomly get the below error
"Cannot rename symbol: module has no explicit export list and the symbol is referenced from other modules."

reverting this block to the older version made all of the cases work fine ( Confirmed this multiple times as well )
any guesses why this would happen ?

[wz1000]

Can you offer a overview of what the bug was, and your approach towards fixing it please? It would be good to add this to the commit message as well.

[wz1000]

It would also be good to add the example from #4816 as a regression test.

[vidit-od]

These are the conditions that define when to show the mentioned error.

CurrentModule -> isLocallyDefined -> renamingLocalDeclaration -> causing the error ?

could this be a possible reason ?

[vidit-od]

• i have updated the PR description with an overview explanation,
• also new regression tests including the ones listed in original issue as well as a few more cases have been added.
• The one review regarding usage of Modsummary has been reverted because of the reasons listed in above conversation.

On a side note: the tests for rename plugin gave me a very hard time. Tho , I dont know if we can do anything about it or not :)

requesting a re-review @wz1000 @fendor