Support extending constructors

[berberman]

Perhaps we no longer need to parse the extended binding, completely depending on the diagnostic message, since we now have ExportsMap. Closes haskell/haskell-language-server#609:

[jneira]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pepeiborra]

Thank you for this change, this was so annoying!
Do we need a better test?

Could you undo all the auto formatting? It will lead to unnecessary merge conflicts for other contributions, and its best done by maintainers when the time is right. Thank you!

[berberman]

Sorry for that, I've restored changes done by the formatter.
As for tests, hmmm, I don't have a good idea. Old tests can pass after the changes. Should I add the case in the screenshot?

[pepeiborra]

Yes, I think encoding the screenshot as a test would be good enough. Thanks!

[berberman]

I have a question about this test case,

,  (`xfail` "known broken") $ testSession "extend single line import with constructor" $ template
      (T.unlines
            [ "module ModuleA where"
            , "data A = Constructor"
            ])
      (T.unlines
            [ "module ModuleB where"
            , "import ModuleA (A)"
            , "b :: A"
            , "b = Constructor"
            ])
      (Range (Position 2 5) (Position 2 5))
      "Add Constructor to the import list of ModuleA"
      (T.unlines
            [ "module ModuleB where"
            , "import ModuleA (A(Constructor))"
            , "b :: A"
            , "b = Constructor"
            ])

which is marked as "known broken". Expected failure here means "user should not write code like that" or "currently we don't support this feature"?

[pepeiborra]

That test case is marked as known broken because it reproduces a known bug that needs to be fixed. Which your PR does

[berberman]

Now we can extend () to (A(Constructor)), but not (A) to (A(Constructor)). So this test is failing, breaking the CI, since the reult becomes (A(Constructor),A). The later one could be implemented, however, I don't think the user would add the constructor to the import list without the parent like this...

[berberman]

The test which was marked as "known broken" previously can pass now, and I added a new test to demonstrate extending constructors. However, current implementation breaks the "extend multi line import with value", because spaces are dropped before removing the trailing comma, causing multi line collapsed. This can be fixed by removing the comma in the end of the import list but keeping spaces.

[pepeiborra]

I find this code very hard to read. Could you try to golf it a bit to make it more readable? Some concrete suggestions:

• whitespace and layout
• both rest' and binding' perform branching on whether parent is null. Make this more apparent
• introduce more names or add more comments, there's a lot of character matching that is hard to follow
• consider replacing some of the character matching with regexes if that makes things simpler.

[berberman]

Thanks for your suggestions! I totally agree. It lacks of extensibility and is hard to read 😢

[pepeiborra]

@berberman are you planning to implement this suggestion?

[adamse]

Anything I can do to help move this forward? :)

[berberman]

Sorry for the delay; I‘ve been pretty busy recently...
I may be free next weekend, and helps are welcome :)
Current implementation works but is ugly, so my plan is something like handling two branches (whether the thing we are going to extend is a constructor of a data type) separately, and using regexs or parsers to improve the readability and robustness.

[pepeiborra]

I'll merge this as it is, please send a new PR for clean-ups.