[WIP] Completion support #227

serras · 2019-12-09T23:08:21Z

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 · 2019-12-10T09:35:24Z

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

ndmitchell · 2019-12-10T09:40:33Z

@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 · 2019-12-10T16:26:06Z

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 · 2019-12-10T16:42:42Z

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

fendor · 2019-12-10T20:38:10Z

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

ndmitchell · 2019-12-10T20:39:50Z

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 · 2019-12-12T16:20:56Z

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 · 2019-12-12T22:37:27Z

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 · 2019-12-13T10:24:20Z

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 · 2019-12-13T11:55:54Z

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

This reverts commit 04ca13b.

cocreature · 2019-12-13T12:12:59Z

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 · 2019-12-13T12:27:15Z

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 · 2019-12-13T15:19:21Z

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 · 2019-12-14T19:49:22Z

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)

src/Development/IDE/LSP/Completions.hs

src/Development/IDE/Core/Completions.hs

cocreature · 2019-12-18T09:50:34Z

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 · 2019-12-18T20:19:17Z

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 · 2019-12-19T13:12:56Z

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 · 2019-12-19T13:59:59Z

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

wz1000 · 2019-12-19T18:29:41Z

src/Development/IDE/Core/Completions.hs

+parseNs (String "z") = pure tvName
+parseNs _ = mempty
+
+instance FromJSON NameDetails where


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.

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

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.

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.

wz1000 · 2020-09-15T20:49:51Z

src/Development/IDE/Core/Completions.hs

+      -- default to value context if no explicit context
+      context = fromMaybe ValueContext $ getCContext pos (tm_parsed_module tm)
+
+      {- correct the position by moving 'foo :: Int -> String ->    '


@serras sorry for digging up this old PR, but do you remember the rationale for this?

Most of this code was taken from haskell-ide-engine, and I think that's where this originated.

I know, I was one of the original implementors! Guess I'll have to do some more digging...

@cocreature

* Initial implementation of completion support * Add fuzzy to set of additional dependencies in 8.8 * Fix test * Work a bit more on completion * Attempt at getting completions from last good tckd module * Revert "Attempt at getting completions from last good tckd module" This reverts commit 04ca13b9d831eaaf013239cd8cbc49ea284b6de1. * "useWithStale" everywhere * Some suggestions by @cocreature * Adjust positions in the document * Start working on tests * Fix compilation problem * Fix tests * Better type tests

@cocreature

* Initial implementation of completion support * Add fuzzy to set of additional dependencies in 8.8 * Fix test * Work a bit more on completion * Attempt at getting completions from last good tckd module * Revert "Attempt at getting completions from last good tckd module" This reverts commit 04ca13b9d831eaaf013239cd8cbc49ea284b6de1. * "useWithStale" everywhere * Some suggestions by @cocreature * Adjust positions in the document * Start working on tests * Fix compilation problem * Fix tests * Better type tests

@cocreature

* Initial implementation of completion support * Add fuzzy to set of additional dependencies in 8.8 * Fix test * Work a bit more on completion * Attempt at getting completions from last good tckd module * Revert "Attempt at getting completions from last good tckd module" This reverts commit 04ca13b9d831eaaf013239cd8cbc49ea284b6de1. * "useWithStale" everywhere * Some suggestions by @cocreature * Adjust positions in the document * Start working on tests * Fix compilation problem * Fix tests * Better type tests

Initial implementation of completion support

c0e0665

serras force-pushed the completions branch from 952271c to c0e0665 Compare December 9, 2019 23:20

serras added 2 commits December 10, 2019 07:56

Add fuzzy to set of additional dependencies in 8.8

85806d1

Fix test

10e9e74

Merge branch 'da-master' into completions2

e978c65

Work a bit more on completion

2842ebb

Attempt at getting completions from last good tckd module

04ca13b

Revert "Attempt at getting completions from last good tckd module"

6c692aa

This reverts commit 04ca13b.

"useWithStale" everywhere

1a24f16

cocreature approved these changes Dec 16, 2019

View reviewed changes

src/Development/IDE/LSP/Completions.hs Outdated Show resolved Hide resolved

src/Development/IDE/LSP/Completions.hs Outdated Show resolved Hide resolved

src/Development/IDE/LSP/Completions.hs Outdated Show resolved Hide resolved

src/Development/IDE/Core/Completions.hs Show resolved Hide resolved

serras added 2 commits December 17, 2019 09:41

Some suggestions by @cocreature

ec781d9

Merge branch 'da-master' into completions2

60805c2

cocreature self-assigned this Dec 17, 2019

serras added 2 commits December 17, 2019 17:17

Merge branch 'da-master' into completions2

258b66c

Adjust positions in the document

f358a5c

serras added 3 commits December 18, 2019 21:15

Start working on tests

4210fdf

Merge branch 'da-master' into completions2

ab1056a

Fix compilation problem

6a93eba

cocreature added 2 commits December 19, 2019 14:07

Fix tests

6e5826d

Better type tests

dbc270e

cocreature merged commit b52ee60 into haskell:master Dec 19, 2019

wz1000 reviewed Dec 19, 2019

View reviewed changes

serras mentioned this pull request Dec 20, 2019

Better docs for completions #288

Merged

serras mentioned this pull request Jan 4, 2020

Remove JSON instances for unused completion code #305

Merged

wz1000 reviewed Sep 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Completion support #227

[WIP] Completion support #227

serras commented Dec 9, 2019

mpickering commented Dec 10, 2019

ndmitchell commented Dec 10, 2019

serras commented Dec 10, 2019

ocharles commented Dec 10, 2019

fendor commented Dec 10, 2019

ndmitchell commented Dec 10, 2019

cocreature commented Dec 12, 2019

serras commented Dec 12, 2019

ndmitchell commented Dec 13, 2019

serras commented Dec 13, 2019

cocreature commented Dec 13, 2019

ndmitchell commented Dec 13, 2019 •

edited

Loading

cocreature commented Dec 13, 2019

serras commented Dec 14, 2019

cocreature left a comment

cocreature commented Dec 18, 2019

serras commented Dec 18, 2019

cocreature commented Dec 19, 2019

serras commented Dec 19, 2019

wz1000 Dec 19, 2019

serras Dec 19, 2019

wz1000 Dec 19, 2019

wz1000 Dec 19, 2019

wz1000 Sep 15, 2020

serras Sep 15, 2020

wz1000 Sep 15, 2020

[WIP] Completion support #227

[WIP] Completion support #227

Conversation

serras commented Dec 9, 2019

mpickering commented Dec 10, 2019

ndmitchell commented Dec 10, 2019

serras commented Dec 10, 2019

ocharles commented Dec 10, 2019

fendor commented Dec 10, 2019

ndmitchell commented Dec 10, 2019

cocreature commented Dec 12, 2019

serras commented Dec 12, 2019

ndmitchell commented Dec 13, 2019

serras commented Dec 13, 2019

cocreature commented Dec 13, 2019

ndmitchell commented Dec 13, 2019 • edited Loading

cocreature commented Dec 13, 2019

serras commented Dec 14, 2019

cocreature left a comment

Choose a reason for hiding this comment

cocreature commented Dec 18, 2019

serras commented Dec 18, 2019

cocreature commented Dec 19, 2019

serras commented Dec 19, 2019

wz1000 Dec 19, 2019

Choose a reason for hiding this comment

serras Dec 19, 2019

Choose a reason for hiding this comment

wz1000 Dec 19, 2019

Choose a reason for hiding this comment

wz1000 Dec 19, 2019

Choose a reason for hiding this comment

wz1000 Sep 15, 2020

Choose a reason for hiding this comment

serras Sep 15, 2020

Choose a reason for hiding this comment

wz1000 Sep 15, 2020

Choose a reason for hiding this comment

ndmitchell commented Dec 13, 2019 •

edited

Loading