Add GetHieAsts rule, Replace SpanInfo, add support for DocumentHighlight and scope-aware completions for local variables #784

wz1000 · 2020-09-12T15:17:53Z

This is all the remaining changes from the hls-3 branch.

Drops support for GHC 8.4

Fixes #663

Changes:

Remove the GetSpanInfo rule, depend entirely on GetHieAst for hover, go to definition etc.

New features:

Document Highlight for highlighting references of a Name in the same file
Completion for local variables in scope via GetBindings

Missing features:

~~Go to definition for module names~~
~~Kind info on hover~~
Relevant constraints on hover

TODOs:

Add tests for new features
fix as many existing tests as possible
Re-add as many missing features as possible, using something like the DocMap mechanism
Benchmark performance differences

Related GHC MRs for .hie file changes:

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4037

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4068

One has already been merged into the ghc-9.0 branch, the other will be merged soon hopefully

Old description:

This is the slight modification of what already lives in the hls-3 and hiedb-3 branches, necessary for haskell/haskell-language-server#391

We make some modifications to be able to save the HieAst before compressing the types. We compress the types on demand when we actually need to write the file to disk. This should improve hls performance somewhat as we get rid of the overhead of type compression (which can be expensive) in most cases.

isovector · 2020-09-12T16:11:01Z

Marvelous. Thank you so much!

pepeiborra · 2020-09-13T05:58:46Z

Will other clients like Stan be able to use the hie files produced by ghcide? If not, maybe we should change the file extension

pepeiborra · 2020-09-13T05:59:09Z

Any changes worth upstreaming to ghc?

pepeiborra

This PR seems a bit incomplete. Is there any reason for that?

src/Development/IDE/Core/Rules.hs

wz1000 · 2020-09-13T06:11:35Z

Will other clients like Stan be able to use the hie files produced by ghcide? If not, maybe we should change the file extension

Yes, the hie files are generated in precisely the same way.

Any changes worth upstreaming to ghc?

Yeah, we need enrichHie exported. I will make a MR today. Hopefully it can get into 9.0

pepeiborra · 2020-09-19T15:20:12Z

I just updated the benchmarks to measure allocations and delayed actions

pepeiborra

There's a lot of changes I do not understand, but they've been in HLS since 0.2.0 so I am happy to merge them provided the tests pass and the new benchmarks don't show any nasty regressions.

src/Development/IDE/Core/RuleTypes.hs

src/Development/IDE/Spans/LocalBindings.hs

wz1000 · 2020-09-19T19:16:54Z

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

@@ -392,19 +396,7 @@ getCompletions ideOpts cc pm pmapping prefixInfo caps withSnippets = do
          to                             'foo :: Int -> String ->    '
                                                              ^
      -}
-      pos =


We don't need this stuff anymore because of fuzzy position mapping. I had to wade through multiple git commits across repositories to find @bubba as the source of this code. If I understand correctly, it existed so that we could detect type contexts even if the module didn't parse.

Suppose the cursor is at the |

foo :: Integer -> Integer -> |

So the idea was to strip out the trailing "-> ", and then look for the "context" of the position (whether it was in a value/type/import etc.) at the position (newPositionToOld would just return Nothing).

Now, fromDelta will return the original range that was inserted to/replaced, so we can use it instead.

wz1000 · 2020-09-21T14:29:10Z

I'm a bit busy this week, will hopefully finish this up on the weekend.

Till then, here's a gif demonstrating local completions(you can try it out by running haskell-language-server):

Implement Document Hightlight LSP request Add GetDocMap, GetHieFile rules.

wz1000 · 2020-09-26T10:16:30Z

I am leaning towards not adding back the "relevant constraints on hover" feature.

It is not complete, it misses many constraints (for example, constraints introduced by GADT pattern matching etc.)
It will require a generic SYB style traversal of the AST, which is not needed for anything else anymore thanks to this PR
GHC 9.0 will allow writing this feature properly with the changes from https://gitlab.haskell.org/ghc/ghc/-/merge_requests/1286
(the weakest reason) this feature hasn't existed in HLS for a long time and nobody has seemed to miss it.

/cc @pepeiborra @ndmitchell thoughts?

wz1000 · 2020-09-26T10:55:18Z

I have fixed as many tests as I plan to. The remaining failures are genuine behavior divergences that I don't think are worth fixing. Please review them once CI completes.

The only TODO remaining is to add tests for the new features.

pepeiborra · 2020-09-26T11:46:58Z

I am fine with dropping the "constraints on hover" feature, it was bit erratic for me and only seemed to work on declaration lhs's. Is there any chance of back porting #1286 to the ghcide fork of Hie files?

Ignoring the tests related to that feature, there are only 5 failing tests, all related to hover. Good job!

Please share some benchmarks if you have the chance too.

wz1000 · 2020-09-26T11:49:31Z

Here are the interesting benchmark results:

hover after edit seems to be down:

completions after edit is up, but that is to be expected since we are doing more work, calculating local completions:

There is some funkyness with the allocations for document symbols I can't explain:

The rest seem to be unchanged, except for the shape of the allocation graph: http://78.47.113.11/benchmarks/

pepeiborra · 2020-09-26T14:42:35Z

Look at the "Delayed work" column in the results.csv file:

"Hover after edit" is almost twice as efficient in delayed time. 50% more efficient in user time, 50% less allocations
"Document symbols after edit" is almost twice as efficient in delayed time
"Completions after edit" is almost twice as inefficient in delayed time and allocates 19% more

wz1000 · 2020-09-26T14:44:59Z

What exactly is delayed time?

pepeiborra · 2020-09-26T14:46:06Z

What exactly is delayed time?

The time spent waiting for all the delayed actions in the Shake queue to complete

wz1000 · 2020-09-26T14:52:54Z

The regression in completion performance is expected since we now depend on Typecheck, not just GetParsedModule. Stale information should hide most of the difference in actual use I think. I've taken care to still generate completions using only the ParsedModule if typechecking fails.

wz1000 · 2020-09-26T16:19:57Z

I have no idea why there is a change for DocumentSymbols.

wz1000 · 2020-09-26T19:36:43Z

Added tests! This is done except for that one Unexpected Pass on 8.10.

wz1000 · 2020-09-26T20:24:11Z

I don't have the energy to figure out why the test is passing on 8.10 but not on GHC versions before that. I diffed the HieAst generation code, but could find no obvious culprits. I'm just CPPing the error away. With any luck this is good to go now.

wz1000 · 2020-09-27T06:35:36Z

Any opinions on whether I should remove 8.4 specific CPP in this PR, or a different one?

pepeiborra · 2020-09-27T07:52:17Z

I would say merge this when ready and clean up 8.4 CPP in another PR to minimise conflicts

wz1000 · 2020-09-27T07:53:37Z

OK, I think this is ready, we can merge if you are happy.

…ght and scope-aware completions for local variables (haskell#784) * Add GetHieAsts rule * hlint * fix build for 8.4 * Reimplement Hover/GotoDefn in terms of HIE Files. Implement Document Hightlight LSP request Add GetDocMap, GetHieFile rules. * Fix gotodef for record fields * Completion for locals * Don't need to hack cursor position because of fuzzy ranges * hlint * fix bench and warning on 8.10 * disable 8.4 CI jobs * Don't collect module level bindings * tweaks * Show kinds * docs * Defs for ModuleNames * Fix some tests * hlint * Mark remaining tests as broken * Add completion tests * add highlight tests * Fix HieAst for 8.6 * CPP away the unexpected success * More CPP hacks for 8.10 tests

…ght and scope-aware completions for local variables (haskell/ghcide#784) * Add GetHieAsts rule * hlint * fix build for 8.4 * Reimplement Hover/GotoDefn in terms of HIE Files. Implement Document Hightlight LSP request Add GetDocMap, GetHieFile rules. * Fix gotodef for record fields * Completion for locals * Don't need to hack cursor position because of fuzzy ranges * hlint * fix bench and warning on 8.10 * disable 8.4 CI jobs * Don't collect module level bindings * tweaks * Show kinds * docs * Defs for ModuleNames * Fix some tests * hlint * Mark remaining tests as broken * Add completion tests * add highlight tests * Fix HieAst for 8.6 * CPP away the unexpected success * More CPP hacks for 8.10 tests

wz1000 requested a review from pepeiborra September 12, 2020 15:18

wz1000 force-pushed the hie-rule branch from 66c9107 to 6cb3bb1 Compare September 12, 2020 15:23

wz1000 mentioned this pull request Sep 12, 2020

bump ghcide submodule haskell/haskell-language-server#396

Merged

pepeiborra reviewed Sep 13, 2020

View reviewed changes

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

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

wz1000 force-pushed the hie-rule branch from 5dbb8e0 to fc189f0 Compare September 18, 2020 09:14

wz1000 changed the title ~~Add GetHieAsts rule~~ Add GetHieAsts rule, Replace SpanInfo, add support for DocumentHighlight and scope-aware completions for local variables Sep 18, 2020

wz1000 requested a review from pepeiborra September 18, 2020 09:27

wz1000 force-pushed the hie-rule branch from 36ec7b8 to 2a9b859 Compare September 18, 2020 12:38

pepeiborra approved these changes Sep 19, 2020

View reviewed changes

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

src/Development/IDE/Spans/LocalBindings.hs Show resolved Hide resolved

src/Development/IDE/Spans/LocalBindings.hs Outdated Show resolved Hide resolved

wz1000 commented Sep 19, 2020

View reviewed changes

This was referenced Sep 23, 2020

Use hie-implicit-cradle-0.2.0.1 #827

Merged

Changes from prepare for release 0.4.0 #832

Merged

wz1000 added 12 commits September 26, 2020 13:11

Add GetHieAsts rule

829fbbd

hlint

0294e3e

fix build for 8.4

40ee2a9

Reimplement Hover/GotoDefn in terms of HIE Files.

732cb91

Implement Document Hightlight LSP request Add GetDocMap, GetHieFile rules.

Fix gotodef for record fields

764a8c8

Completion for locals

5770346

Don't need to hack cursor position because of fuzzy ranges

cc77c66

hlint

f0cedaa

fix bench and warning on 8.10

17250c5

disable 8.4 CI jobs

8dad292

Don't collect module level bindings

71e4c76

tweaks

49c3051

wz1000 force-pushed the hie-rule branch from 2a9b859 to 170eede Compare September 26, 2020 09:09

wz1000 added 2 commits September 26, 2020 14:41

docs

15565ad

Defs for ModuleNames

879f1b7

wz1000 added 2 commits September 26, 2020 16:20

Fix some tests

5d86341

hlint

5e27c5f

Mark remaining tests as broken

22fe011

wz1000 added 2 commits September 27, 2020 00:33

Add completion tests

ef8a268

add highlight tests

9ca775b

wz1000 added 2 commits September 27, 2020 01:45

Fix HieAst for 8.6

6378b67

CPP away the unexpected success

9539494

More CPP hacks for 8.10 tests

3ff2441

pepeiborra merged commit 62f4d06 into haskell:master Sep 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetHieAsts rule, Replace SpanInfo, add support for DocumentHighlight and scope-aware completions for local variables #784

Add GetHieAsts rule, Replace SpanInfo, add support for DocumentHighlight and scope-aware completions for local variables #784

wz1000 commented Sep 12, 2020 •

edited

Loading

isovector commented Sep 12, 2020

pepeiborra commented Sep 13, 2020

pepeiborra commented Sep 13, 2020

pepeiborra left a comment

wz1000 commented Sep 13, 2020

pepeiborra commented Sep 19, 2020

pepeiborra left a comment

wz1000 Sep 19, 2020

wz1000 commented Sep 21, 2020

wz1000 commented Sep 26, 2020

wz1000 commented Sep 26, 2020

pepeiborra commented Sep 26, 2020

wz1000 commented Sep 26, 2020

pepeiborra commented Sep 26, 2020

wz1000 commented Sep 26, 2020

pepeiborra commented Sep 26, 2020

wz1000 commented Sep 26, 2020 •

edited

Loading

wz1000 commented Sep 26, 2020

wz1000 commented Sep 26, 2020

wz1000 commented Sep 26, 2020 •

edited

Loading

wz1000 commented Sep 27, 2020

pepeiborra commented Sep 27, 2020 •

edited

Loading

wz1000 commented Sep 27, 2020

Add GetHieAsts rule, Replace SpanInfo, add support for DocumentHighlight and scope-aware completions for local variables #784

Add GetHieAsts rule, Replace SpanInfo, add support for DocumentHighlight and scope-aware completions for local variables #784

Conversation

wz1000 commented Sep 12, 2020 • edited Loading

isovector commented Sep 12, 2020

pepeiborra commented Sep 13, 2020

pepeiborra commented Sep 13, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

wz1000 commented Sep 13, 2020

pepeiborra commented Sep 19, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

wz1000 Sep 19, 2020

Choose a reason for hiding this comment

wz1000 commented Sep 21, 2020

wz1000 commented Sep 26, 2020

wz1000 commented Sep 26, 2020

pepeiborra commented Sep 26, 2020

wz1000 commented Sep 26, 2020

pepeiborra commented Sep 26, 2020

wz1000 commented Sep 26, 2020

pepeiborra commented Sep 26, 2020

wz1000 commented Sep 26, 2020 • edited Loading

wz1000 commented Sep 26, 2020

wz1000 commented Sep 26, 2020

wz1000 commented Sep 26, 2020 • edited Loading

wz1000 commented Sep 27, 2020

pepeiborra commented Sep 27, 2020 • edited Loading

wz1000 commented Sep 27, 2020

wz1000 commented Sep 12, 2020 •

edited

Loading

wz1000 commented Sep 26, 2020 •

edited

Loading

wz1000 commented Sep 26, 2020 •

edited

Loading

pepeiborra commented Sep 27, 2020 •

edited

Loading