[WIP] Completion support

[serras]

This PR adds completion support (as per #211). Right now it's mostly stripped out parts of haskell-ide-engine, put back together into the rule-based system of ghcide. It seems to work for value level things, but not so well for type-level things, so further investigation is needed.

The information shown is also very basic. In the future the idea is to also return the documentation of the functions, if available.

[mpickering]

I thought the idea was to not implement completion support directly in ghcide but as some kind of plugin. Has that plan changed?

[ndmitchell]

@mpickering I think the plan was always to implement completions inside ghcide. It was things like Hoogle/HLint/LiquidHaskell we thought were best handled by plugins.

[serras]

I was not aware on any plans on this respect, I just found ghcide to be a nice project to contribute to! But if you prefer to do it in other way, I'm happy too :)

[ocharles]

Just wanted to say I tried this out and it works very nicely!

[fendor]

Just mentioning that you are probably inheriting haskell/haskell-ide-engine#1434 from me.

[ndmitchell]

Thanks for the heads up @fendor - would be great if we can eventually share these (either because hie and ghcide merge, or because there is a common underlying library everyone uses)

[cocreature]

Thanks for the PR, I’m hoping to get around to taking a look at this tomorrow but from an initial look, it looks great 👍

[serras]

There's still a problem I need to solve. The completions only work if the chnages make the problem still typecheck. For example, if I have

thing :: String -> I

and I want to obtain completions for I, then nothing happens. But if I have

thing :: String -> Int

this is type-correct code and Int32, Int64 and so on are shown.

The question here is: is there any way to "save" the latest type correct TypecheckedModule and use it if the new one gives problems? That would be the solution to these problems.

[ndmitchell]

The core of the IDE was designed to support getting stale values, e.g. useWithStale https://github.com/digital-asset/ghcide/blob/master/src/Development/IDE/Core/Shake.hs#L432. I'm not sure where that is poked out, or even if it still works (or in fact ever worked), but that was the original design, precisely for this purpose.

[serras]

So if I changed my usage of use TypeCheck with useWithStale TypeCheck, everything should in theory work? Thay would be great!

[cocreature]

useWithStale definitely works (at least in some circumstances), we use this in DAML. But for ghcide, I would have expected that the -fdefer-type-errors stuff takes care of making sure that you still get a typechecking result even if typechecking fails.

[ndmitchell]

defer type errors will defer type errors, but not parse or name errors, which tends to be when you are asking for completions, and typically I want the best completions that can be found in < 0.1s. I think this is the right place to use stale results.

[cocreature]

The code looks good to me, thanks a lot! I’ve also tested it on an internal codebase and it doesn’t seem to increase memory usage significantly which I was a bit afraid off. I am trying to figure out what we need to make sure that this doesn’t cause any licensing problems. I’ll get back to you once I know what steps need to be taken.

[serras]

It would be great if you could test the code. It seems that everything works fine except for qualified imports, but I cannot find the reason why. Also, the new version with useWithStale works quite well, but not in a completely reliable way.

Some other issues before considering merging:

• The code from HIE uses something called withSnippets, but I cannot completely understand why. I am leaning towards removing that part.
• The code which obtains the type for elements doesn't work for imported functions (for example, the completion of head only shows Prelude, but not the type). My gut feeling is that it's related to getComplsForOne (around line 300 of IDE/Core/Completions), but any help would be great.

[cocreature]

This looks great with some minor comments and seems to work very nicely in some manual tests, thanks a lot! I would that we add at least one test that makes sure we don’t break this completely before merging.

I’ve left an inline comment on the withSnippets stuff. I think it makes sense to keep it for now.

As for the other bug, I would suggest to address this in a separate PR. For now, a test marked as an expected failure would be great.

(I’ve got a response on the licensing question and that’s all good)

[cocreature]

Thanks for fixing the positions, I would still like to see some tests before merging this. Doesn’t need to be comprehensive but at least one basic integration test and one that exercises the useWithStale code path and checks that the positions are correctly remapped.

[serras]

I've written some preliminary tests. However, I cannot get my head around how to check the stale file; I've tried several ways but all of them lead to a stuck test.

[cocreature]

I’ve pushed a fix for the tests. The two expected failures where caused by the fact that those did not typecheck nor did you have a stale result. I changed them to first produce something that typechecks so you have a stale result. Given that this already exercises the stale codepath I’ve removed the separate test for that.

Do you have anything else that you want to address? Otherwise I think this should be ready to land (I’ll be on vacation so it might take me longer to respond).

[serras]

No, the PR looks perfect to me. I guess we now should wait for bug reports and other problems that may arise.

[wz1000]

All this JSON stuff is not needed here because it is only used for the resolveCompletion LSP request, where the types of non local names and documentation is looked up. It doesn't seem like you added support for the resolve completion request.

[serras]

Nope, I didn't implement the resolver completion request, since all information is already sent in the original response. Is this wrong?

[wz1000]

I don't think you are going to get types for things not defined in the active module, since they are looked up in the local type env.

[wz1000]

If you don't plan to implement the resolve completion request, you can remove everything to do with JSON - which is everything from line 42 to 77.