ghcide: Implements a CodeAction to disambiguate ambiguous symbols

[konn]

This PR aims at automating the disambiguation of import lists with the following options:

• Hide all the imports except the chosen module
• Make ambiguous symbol qualified
  • Currently, if two colliding modules have the same qualified name, the resulting code can still have ambiguous occurrence, though.

This PR fixes #1274: it calls setEntryDPT (DP (0,0)) at the end of rewriteToEdits.

DEMO

[konn]

Hmm, it seems annotation transfer doesn't go well and error is occurring at the call of tail in rewriteToEdit, making HLS clash when colliding symbol is detected...

[pepeiborra]

I think naïve implementation is done; now I want to write functional/golden tests. Where should I place the ghcide-related functional tests?

Thank you! The ghcide test suite is all one big module: ghcide/test/exe/Main.hs

[konn]

OK, before implementing tests, it somehow became working!

But it seems that combining multiple Rewrites into one adds unnecessary lines over the import list.
Although it doesn't pose a functional issue, it is more desirable to retain lines...

[berberman]

I think extendHiding closely resembles extendImport. They both process Located [LIE pass] similarly, and the only difference might be the Bool value, the first item of ideclHiding.

[konn]

I think extendHiding closely resembles extendImport. They both process Located [LIE pass] similarly, and the only difference might be the Bool value, the first item of ideclHiding.

Yeah, indeed extendHiding is derived from extendImportToplevel. Major difference is that we have to insert AnnHiding annotation before open parens when there is no hiding clause before.
Another difference is that extendImports accepts new imported symbol with parens when they are infix operato, while extendHiding without parens. This difference comes from the slight difference of how problematic identifier is reported in undefined identifier and ambiguous imports error.

Other difference is just a matter of preference: 'extendHiding` adds new hidden symbol at the start while extendImportToplevel at end.

Yes, these differences can be abstracted away and we can unify two functions. But I didn't do that because their usage and purpose differ slightly and can cause some confusion. Still, there can be some refactoring to factor out common operations as separate functions, as you pointed out.

[berberman]

You are right, I've ignored AnnHiding XD

For parens, I think it was #1212 that made identifiers parenthesized before stored into ExportsMap, so I didn't treate them specially in extending imports. A little bit off-topic: IMHO It would be better to handle parens right here, because on the one hand, functions to extend hiding and imports could behave consistently; on the other hand, afaik diagnostics reported for extending imports don't parenthesize operators, and we don't insert rendered text but operate AST to make sure that they are parenthesized correctly, so there is no need to parenthesize them in exports map. In this case unIEWrappedName can be removed, and as you said, further refactor might be smoother because of the unification.

[konn]

It turns out that multiple import list isn't a direct cause of an additional newline at the top.
A new line will be added when some identifier is removed from any modification is made to the import lists in the topmost import declaration.
I couldn't figure out why this phenomenon occurs, though...

[konn]

OK, as the resolution of the conflict with master, I decided to address #1274 in this PR, following the workaround suggested by @pepeiborra and @berberman.
One difference from @pepeiborra's solution is that we invoke setEntryDPT in rewriteToEdit, not in rewrite; this is intended to keep implementation details and necessary procedure in a single module, say Development.IDE.Plugin.CodeAction.ExactPrint. This is to avoid a similar procedure scattered over several modules.

[konn]

Hmm, it seems addConstraint tests start to fail... I will look into.

[konn]

OK, now it passes tests for constraint manipulation.

[konn]

I think this PR reached a stable state after merging current master's change.

[berberman]

LGTM, thank you for this awesome work!

[konn]

Not so important, but I've just added DEMO GIF to the summary.