Implement completionItem/resolve #3204

wz1000 · 2022-09-21T04:52:33Z

Implement completionItem/resolve to lazily resolve types and documentation for completion items.
This results in very big memory usage improvments while also improving the responsiveness of completions.
Usually when GHC typechecks code, it reads in and deserializes interfaces lazily, so if the code imports
a bunch of modules, the interfaces will only be read in and deserialized if a particular name is actually
required for typechecking.

However, when we greedily generate completion lists, the act of querying for types and documentation results
in sucking in and deserializing interface sections for every single name in scope. This is very bad for
memory usage as it negates virtually all the benefits of lazy deserialization.

Instead, we can compute types and documentation on demand for individual completion items using the completionItem/resolve
LSP functionality.

A downside to this is that we can no longer generate snippets for functions
arguments with placeholders mentioning the types because we no longer have this
information available when we generate the original completion, and it is not
supported by LSP to resolve the actual completion snippet using completionItem/resolve.

However, I don't personally consider this to be a very big deal because this
feature was unreliable (eg for things from the export map which weren't in
scope) and I found it to be quite annoying when it did work.

michaelpj · 2022-09-21T10:57:11Z

Wow, this is a big deal!

michaelpj

Code looks good! I'm going to do my usual routine and question whether it's really the case that adding a complex feature like this requires 0 lines of explanatory comments :)

michaelpj · 2022-09-21T12:00:14Z

ghcide/src/Development/IDE/Plugin/Completions/Logic.hs

-        docs <- getDocumentationTryGhc packageState curMod n
+      toCompItem :: Parent -> Module -> T.Text -> Name -> Maybe (LImportDecl GhcPs) -> [CompItem]
+      toCompItem par m mn n imp' =
+        -- docs <- getDocumentationTryGhc packageState curMod n


commented out code

@wz1000 is this the line causing the eager deserialization of interfaces?

This along with lookupName (the lookupName is slightly worse since it forces typechecking the iface which is also done lazily, getDocumentation will still load in the interface if it wasn't needed before and deserialize the documentation part).

still commented out

ghcide/src/Development/IDE/Plugin/Completions/Logic.hs

ghcide/src/Development/IDE/Plugin/Completions/Types.hs

ghcide/src/Development/IDE/Plugin/Completions.hs

michaelpj · 2022-09-21T12:10:13Z

ghcide/src/Development/IDE/Plugin/Completions.hs

+    mdkm <- liftIO $ runAction "Completion resolve" ide $ use GetDocMap file
+    case mdkm of
+      Nothing -> pure (Right comp)
+      Just (DKMap dm km) -> liftIO $ runAction "Completion resolve" ide $ do


I think you can make this kind of pattern simpler using the pattern we use elsewhere in the codebase using pluginResponse at the top level and maybeToEither and friends.

ghcide/src/Development/IDE/Plugin/Completions.hs

michaelpj · 2022-09-21T12:13:43Z

Also: tests?

michaelpj · 2022-09-22T08:51:05Z

Oh one last thing: we definitely want a cautionary comment on the main completion handler warning people not to accidentally pull in information from interface files in future!

michaelpj · 2022-09-26T08:13:03Z

#3080 gets the HIE file during completions, is that acceptable?

pepeiborra · 2022-09-26T10:56:10Z

#3080 gets the HIE file during completions, is that acceptable?

I am pretty confident getting the HIE file is 👍

michaelpj · 2022-09-27T09:16:49Z

I merged the record dot completions PR, which has introduced some conflicts.

michaelpj

Previous comments still need addressing also.

michaelpj · 2022-09-29T13:40:16Z

ghcide/src/Development/IDE/GHC/Compat.hs

+-- the NameCache.
+-- Reason: this may the first occurrence of (say) Foo.bar we have encountered.
+-- If we need to explore its value we will load Foo.hi; but meanwhile all we
+-- need is a Name for it.


why is that all we need? surely it will be the wrong Name and then we could get into trouble later? 🤔

In particular it looks like you look it up in the module environment later, how can that work if they're fresh?

ghcide/src/Development/IDE/Plugin/Completions/Logic.hs

isovector

lgtm for wingman

michaelpj

Fine, although I do think there were a few more comments that could be addressed.

michaelpj · 2022-12-16T19:18:03Z

.github/workflows/test.yml

@@ -221,7 +221,7 @@ jobs:
        name: Test hls-explicit-imports-plugin test suite
        run: cabal test hls-explicit-imports-plugin --test-options="$TEST_OPTS" || LSP_TEST_LOG_COLOR=0 LSP_TEST_LOG_MESSAGES=true LSP_TEST_LOG_STDERR=true cabal test hls-explicit-imports-plugin --test-options="$TEST_OPTS"

-      - if: matrix.test
+      - if: matrix.test && matrix.os != 'windows-latest'


why? is there an issue?

call-hierarchy tests were very flaky, I've fixed the problem I think and removed this.

michaelpj · 2022-12-16T19:18:48Z

ghcide/src/Development/IDE/Plugin/Completions/Logic.hs

-        docs <- getDocumentationTryGhc packageState curMod n
+      toCompItem :: Parent -> Module -> T.Text -> Name -> Maybe (LImportDecl GhcPs) -> [CompItem]
+      toCompItem par m mn n imp' =
+        -- docs <- getDocumentationTryGhc packageState curMod n


still commented out

Wait for index instead of compliation before making requests as indexing happens in the background in parallel with everything else, so we have to synchronize on the database being ready before making any call hierarchy requests.

michaelpj · 2022-12-19T11:19:52Z

pre-commit check is also failing, I think you're missing formatting

wz1000 requested review from jneira, berberman and pepeiborra as code owners September 21, 2022 04:52

michaelpj reviewed Sep 21, 2022

View reviewed changes

michaelpj mentioned this pull request Sep 25, 2022

Feat: basic record dot completions #3080

Merged

wz1000 mentioned this pull request Sep 28, 2022

Improve memory characteristics of ExportsMap #3231

Merged

wz1000 force-pushed the wip/completion-resolve branch from 4fe528a to f91edea Compare September 28, 2022 15:13

michaelpj reviewed Sep 29, 2022

View reviewed changes

wz1000 force-pushed the wip/completion-resolve branch 2 times, most recently from 1e1ad22 to c983c6b Compare November 23, 2022 13:40

wz1000 force-pushed the wip/completion-resolve branch 8 times, most recently from dee48c1 to e7073d6 Compare December 14, 2022 09:59

wz1000 requested a review from isovector as a code owner December 14, 2022 09:59

isovector approved these changes Dec 14, 2022

View reviewed changes

wz1000 force-pushed the wip/completion-resolve branch 4 times, most recently from b367ca7 to cf0e3c3 Compare December 16, 2022 09:12

wz1000 enabled auto-merge (rebase) December 16, 2022 09:22

wz1000 force-pushed the wip/completion-resolve branch 9 times, most recently from cc06936 to 7cf643b Compare December 16, 2022 12:59

wz1000 requested a review from santiweight as a code owner December 16, 2022 12:59

wz1000 requested review from Anton-Latukha and Ailrun as code owners December 16, 2022 15:42

wz1000 added 3 commits December 16, 2022 23:28

Implement completionItem/resolve

4b4e21d

Fixes

ac2b80e

Fixes

12bff8a

wz1000 force-pushed the wip/completion-resolve branch from 7988b03 to fe7d852 Compare December 16, 2022 17:58

michaelpj approved these changes Dec 16, 2022

View reviewed changes

Fix flaky call-hierarchy tests

2d58fe1

Wait for index instead of compliation before making requests as indexing happens in the background in parallel with everything else, so we have to synchronize on the database being ready before making any call hierarchy requests.

wz1000 force-pushed the wip/completion-resolve branch from fe7d852 to 2d58fe1 Compare December 18, 2022 21:25

wz1000 requested a review from July541 as a code owner December 18, 2022 21:25

wz1000 disabled auto-merge December 18, 2022 23:26

Merge branch 'master' into wip/completion-resolve

2744439

wz1000 enabled auto-merge (rebase) December 18, 2022 23:26

wz1000 merged commit 53f61b4 into master Dec 19, 2022

michaelpj mentioned this pull request Mar 21, 2023

Implement completionItem/resolve for more completion types #3535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement completionItem/resolve #3204

Implement completionItem/resolve #3204

wz1000 commented Sep 21, 2022 •

edited by gitpod-io bot

Loading

michaelpj commented Sep 21, 2022

michaelpj left a comment

michaelpj Sep 21, 2022

pepeiborra Sep 22, 2022

wz1000 Sep 22, 2022

michaelpj Dec 16, 2022

michaelpj Sep 21, 2022

michaelpj commented Sep 21, 2022

michaelpj commented Sep 22, 2022

michaelpj commented Sep 26, 2022

pepeiborra commented Sep 26, 2022

michaelpj commented Sep 27, 2022

michaelpj left a comment

michaelpj Sep 29, 2022

michaelpj Sep 29, 2022

isovector left a comment

michaelpj left a comment

michaelpj Dec 16, 2022

wz1000 Dec 18, 2022

michaelpj Dec 16, 2022

michaelpj commented Dec 19, 2022

Implement completionItem/resolve #3204

Implement completionItem/resolve #3204

Conversation

wz1000 commented Sep 21, 2022 • edited by gitpod-io bot Loading

michaelpj commented Sep 21, 2022

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Sep 21, 2022

michaelpj commented Sep 22, 2022

michaelpj commented Sep 26, 2022

pepeiborra commented Sep 26, 2022

michaelpj commented Sep 27, 2022

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isovector left a comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Dec 19, 2022

wz1000 commented Sep 21, 2022 •

edited by gitpod-io bot

Loading