Auto complete definitions within imports

[alexnaspo]

AutoComplete functions inside a module import

import Control.Applicative (A<cursor-position>) Will auto complete to Alternative, Applicative... etc

[jneira]

Thanks for the fix, great to see tests to exercise it

[jneira]

I am not sure if this fixes #2082. Afaics tests are demonstrating that completions works inside imports lists but the issue is about completions over qualified imports in the body of the module:

I have 2 modules

module MyModule (myid) where

myid :: a -> a
myid a = a

module MyModuleB (myid') where

import qualified MyModule

myid' :: a -> a
myid' = MyModule.myid

I delete myid in second module and invoke autocompletion when cursor placed after MyModule.
Expected behaviour

In autocompletion list i see only functions from MyModule
Actual behaviour

In autocompletion list i see all function from all modules

[alexnaspo]

Sorry, I misunderstood the ticket. Will remove from the description.

[pepeiborra]

I am not sure if this fixes #2082. Afaics tests are demonstrating that completions works inside imports lists but the issue is about completions over qualified imports in the body of the module:

I have 2 modules

module MyModule (myid) where

myid :: a -> a

myid a = a

module MyModuleB (myid') where

import qualified MyModule

myid' :: a -> a

myid' = MyModule.myid

I delete myid in second module and invoke autocompletion when cursor placed after MyModule.

Expected behaviour

In autocompletion list i see only functions from MyModule

Actual behaviour

In autocompletion list i see all function from all modules

To clarify the expected behaviour is functions from MyModule followed by functions from all modules.

[pepeiborra]

What does this function do?

[alexnaspo]

I had a typo in the name, however, it builds a map of "module.name" -> ["functions" .. ] which I use to look up the available functions for that module

[pepeiborra]

And why is it needed? Can't you use the ExportsMap as it is?

[alexnaspo]

I believe I need it because I have to group on "module.name" and use that as the key to look up all functions within that module. ExportsMap as it is seems to be a map of "function-name" -> [{moduleNameText: "module-name", name: "function-name" ...} ]

[pepeiborra]

Ah yes, you are right.

But why build a map to do only one lookup and then throw it away?

• Cost of building the map: O(NlogN)
• Cost of doing the lookup by scanning the original exports map: O(N)

Neither is good enough, you want O(logN). How? Probably by changing the representation of the exports map to add a second index, the module name.

[alexnaspoleap]

Do you think the first suggestion would be a good first pass? With potential follow up commits to pull out a plugin?

[pepeiborra]

The ExportsMap only indexes package modules. You will need to fetch the ModIface for local modules. So both changes are needed

[alexnaspo]

I took a first pass at this. There is still more work to do. As of this comment, I am just generating the map based on the already existing map. I also see that this does not work for local modules, as you suggested.

[alexnaspo]

Second pass - I generate the map from the modIface rather than the existing map.. This still does not account for local modules

[alexnaspo]

OK, I now generate the map for local modules as well. I will take a look at your other comments.

[jneira]

Sorry, I misunderstood the ticket. Will remove from the description.

dont worry, the pr looks great anyways, lets see if we had a opened issue about

[jneira]

Afaics this fixes #797 (cc @ndmitchell)

[jneira]

stack builds are failing:

ghcide                     > /root/build/ghcide/src/Development/IDE/Plugin/Completions/Logic.hs:636:19: error:
ghcide                     >     Not in scope: ‘HM.findWithDefault’
ghcide                     >     Perhaps you meant ‘Map.findWithDefault’ (imported from Data.Map)
ghcide                     >     Module ‘Data.HashMap.Strict’ does not export ‘findWithDefault’.
ghcide                     >     |
ghcide                     > 636 |           funcs = HM.findWithDefault [] (T.pack moduleName) exportsMap
ghcide                     >     |                   ^^^^^^^^^^^^^^^^^^
ghcide                     > 

[alexnaspoleap]

stack builds are failing:

ghcide                     > /root/build/ghcide/src/Development/IDE/Plugin/Completions/Logic.hs:636:19: error:
ghcide                     >     Not in scope: ‘HM.findWithDefault’
ghcide                     >     Perhaps you meant ‘Map.findWithDefault’ (imported from Data.Map)
ghcide                     >     Module ‘Data.HashMap.Strict’ does not export ‘findWithDefault’.
ghcide                     >     |
ghcide                     > 636 |           funcs = HM.findWithDefault [] (T.pack moduleName) exportsMap
ghcide                     >     |                   ^^^^^^^^^^^^^^^^^^
ghcide                     > 

thank you - this is fixed

[pepeiborra]

Almost there! Just some minor performance improvements requested

[pepeiborra]

This looks all good, but let's take a look at the Completion benchmark experiments before landing.

[alexnaspo]

This looks all good, but let's take a look at the Completion benchmark experiments before landing.

@pepeiborra - What is involved with this step? Is this part of CI or is it a manual step that I should run and report on? Happy to work through it

[pepeiborra]

This looks all good, but let's take a look at the Completion benchmark experiments before landing.

@pepeiborra - What is involved with this step? Is this part of CI or is it a manual step that I should run and report on? Happy to work through it

There's a CI job that runs the benchmarks, renders the results summary, and produces downloadable artifacts. See https://github.com/haskell/haskell-language-server/actions/runs/1211084983

However the CI results are not entirely reliable, so you can run locally with cabal configure --enable-benchmarks && cabal bench ghcide.

[pepeiborra]

Running locally in gitpod, I get the following results :

gitpod /workspace/haskell-language-server $ column  -t -s, -c name ghcide/bench-results/unprofiled/cabal/results.csv 

version    name                             success   samples   startup              setup                  userTime             delayedTime             totalTime            maxResidency   allocatedBytes
upstream   edit                             True      50        15.106430794000001   0.0                    126.02090627599999   3.000104200000001e-2    126.06007647000001   209MB          42291MB
HEAD       edit                             True      50        16.701233245         0.0                    148.14813984300008   3.8794936999999995e-2   148.19721942         656MB          42067MB
upstream   hover                            True      50        16.690553153         0.0                    46.163053074000004   0.7427802169999999      47.006025115         189MB          7171MB
HEAD       hover                            True      50        15.991552218         0.0                    50.04384343000001    0.7385649630000003      50.801488767         188MB          7107MB
upstream   hover after edit                 True      50        14.994895467000001   0.0                    45.68902047200001    79.83457485499997       125.53222702500001   208MB          41356MB
HEAD       hover after edit                 True      50        15.504793956         0.0                    45.34694745099999    86.838486113            132.231911924        208MB          41399MB
upstream   getDefinition                    True      50        21.100101736000003   0.0                    48.847873189999994   0.759664964             49.705360482         187MB          7101MB
HEAD       getDefinition                    True      50        15.803872888         0.0                    43.204030774999985   0.49277879500000005     43.704813536         187MB          7025MB
upstream   getDefinition after edit         True      50        15.703868614000001   0.0                    47.07017039400001    61.172846302            108.25133311900001   210MB          39006MB
HEAD       getDefinition after edit         True      50        16.295290536         0.0                    48.24514458200001    78.08464355300003       126.338950823        208MB          38942MB
upstream   completions                      True      50        16.100244966         0.0                    67.26779727300001    0.8191487659999999      68.09844481200001    218MB          12642MB
HEAD       completions                      True      50        28.394669321000002   0.0                    77.16584082200002    1.2095481110000001      78.396245182         220MB          12963MB
upstream   completions after edit           True      50        15.904602636000002   0.0                    63.16818985          77.64433259400002       140.82452278         246MB          45151MB
HEAD       completions after edit           True      50        18.298170006         0.0                    161.2938064010001    1.4900570119999998      162.796853899        252MB          44954MB
...

There is a substantial regression in userTime for the "completions after edit" experiment, which takes almost 3 times longer. This is really bad considering that the experiment doesn't actually try to complete within imports - it's just completing on a random identifier inside the module body. Bummer!

1. Can we avoid doing all the work unless we are completing within an import? Yes, but it will require some major code changes.
2. Can we parallelise the work? Yes, and the most effective way to do so is by extracting a new plugin.

Therefore the way forward is to extract import completion out to a new plugin - we don't need a new package, it can stay within ghcide the same way that we split code actions into multiple plugins recently.

[pepeiborra]

Ah, the other thing worth trying is switching from usesWithStale to useWithStaleFast which will have a major impact.
Sadly there isn't a plural version of it - you could investigate writing one (probably in a separate PR)

[alexnaspoleap]

Ah, the other thing worth trying is switching from usesWithStale to useWithStaleFast which will have a major impact.
Sadly there isn't a plural version of it - you could investigate writing one (probably in a separate PR)

Ahh ok, glad this was caught by the experiments! My original implementation made many calls to useWithStaleFast, we still don't want to use that approach, right?

One other thought I had would be to pass in moduleExports as a lazily evaluated closure that only gets invoked when it is actually needed.

I am also happy to separate it out into a separate plugin

[pepeiborra]

Ah, the other thing worth trying is switching from usesWithStale to useWithStaleFast which will have a major impact.

Sadly there isn't a plural version of it - you could investigate writing one (probably in a separate PR)

Ahh ok, glad this was caught by the experiments! My original implementation made many calls to useWithStaleFast, we still don't want to use that approach, right?

It's not ideal, but would be good to benchmark. Overhead will be seen in the delayedTime column

[alexnaspoleap]

Ah, the other thing worth trying is switching from usesWithStale to useWithStaleFast which will have a major impact.

Sadly there isn't a plural version of it - you could investigate writing one (probably in a separate PR)

Ahh ok, glad this was caught by the experiments! My original implementation made many calls to useWithStaleFast, we still don't want to use that approach, right?

It's not ideal, but would be good to benchmark. Overhead will be seen in the delayedTime column

Just pushed up a commit with this. I am still figuring out how to run the benchmark

[alexnaspo]

The benchmarks look much better

[pepeiborra]

And that's why benchmarks are important.

Marking this as ready to merge now, follow ups are optional and can be done in future PRs.

Thanks @alexnaspo for your dedication, this was an awesome effort.

[jneira]

Amazing tour the force from you both @pepeiborra and @alexnaspo, congrats!

[jneira]

This fixed (hopefully) #314