Simpler completion

[wz1000]

After @mpickering pointed this out yesterday, I noticed that completion was a lot more complicated than it needed to be. Instead of the convoluted wrangling we do with import declarations, all the information we need is in the GlobalRdrEnv.

In addition to this, I've separated out the step of getting the type information/snippets for the completion into the LSP CompletionItemResolve request. Type information for local variables which
is readily at hand in the TypecheckedModule itself is still reported in the original completion request.

A potential performance issue I've tried to fix is that looking up the types of all the possible completions would result in GHC loading a bunch of interface files it didn't need to.

For example, consider a module that imports Prelude, but doesn't use any functions from GHC.Enum(like enumFrom). Normally, GHC will not load the interface file for GHC.Enum. However, when we try to get the type of the enumFrom completion, this forces GHC to go and actually load the interface file for that module.

I've implemented the function we used to look up the type information to just return Nothing in this case and not try to load the GHC.Enum interface file. You can see this in action in the "does not pull in unnecessary modules until needed" test case I've added.

The above behaviour is up for discussion, and I will re-implement the old/default behaviour for getTypeForName depending on the consensus.

The interface file will now be pulled in when the client requests to "resolve" the completion.

The behavioural changes due to this PR are:

• Completion items need to be resolved to get their type information/snippets.
• The types/snippets for some functions from modules that haven't been used previously may not be available, as described above.

[lorenzo]

Completions related code is already a significant part of the HieExtras module, what do you think of splitting into another one for better readability?

[lorenzo]

Needs a mappend implementation for ghc 8.2

[lorenzo]

The solution is to simply add here
mappend = (<>)

[lorenzo]

Nice cleanup!

[lukel97]

This is great, was this previously cached for a reason? Helpful reading by the way https://stackoverflow.com/a/3951092/1320911

[lorenzo]

This basically achieve the same as caching it. When I added it to the cache I just wanted to avoid the text packing on each use.

[lukel97]

I agree with @lorenzo, I think the completion logic is significant enough to split out

[wz1000]

@bubba, @lorenzo any comments on the type lookup behaviour? If I restore the old behaviour, because looking up the type is anyway done in the resolveCompletion step, the interface file would only be loaded in when the user goes over the relevant completion (or whenever the client decides to resolve the completion).

[lorenzo]

@wz1000 I don’t think I fully understand the consequences of changing the behavior, superficially this looks ok to me. What do you think the consequences of not having the old behavior might be?

[wz1000]

This is how the new behaviour manifests itself in vscode:

Before GHC has loaded the GHC.Enum interface file, the completion item looks like this:

After I add a usage of enumFrom, GHC is forced to load the GHC.Enum interface file, we get the type for fromEnum also. When the completion is selected, we will also get a "snippet" for the argument.

If I revert to the old behaviour, just having the fromEnum entry selected will make GHC load the interface file for GHC.Enum, and we will get the type of fromEnum in the completion window even if we don't have a usage of enumFrom elsewhere in the file.

[alanz]

I think doing the expensive work in the resolve step is the right way to go. As I understand it, this is precisely why the resolve step exists.

Completion needs to be a fast/unintrusive operation, so reducing the work done is a good step.

[alanz]

I am happy with this approach, and agree we should move the stuff into its own file.

But for traceability it would be better to land this one as a change to HieExtras, so we can see the diff, and then add a second administrative change to move the code around.

[lukel97]

@wz1000 relying on Hoogle for type information felt a bit dodgey, so I am happy that this is the correct way, even if it is a bit slower to resolve, which happens "asynchronously" so I am not concerned.

But in your example posted, does this mean that completion won't show any types until that interface file is loaded by means of it being used inside the document? Or is a completion request alone able to force it to load the interface file?

[wz1000]

@bubba

But in your example posted, does this mean that completion won't show any types until that interface file is loaded by means of it being used inside the document?

Yes, this. But I guess it won't be too bad to load the interface file on a completion resolve request.

Btw, for a comparison of memory usage before and after this patch, see BEFORE and AFTER

The test is loading the file 20 times, then loading it 20 times asking for completions every time. The bump in the middle in BEFORE is when we start asking for completions.

[wz1000]

I've changed it so that the interface file is loaded in when the client asks to resolve the completion, and the type/snippet is available for every symbol in the completion list.
This should be ready to merge now.