hls-notes-plugin: Initial implementation

[jvanbruegge]

A plugin to expand hls' jump-to-definition to work for GHC-style notes.

TODO:

• Support notes in the same module
• Support notes in other modules
• Search for notes at index time, not in the handler
• Be more liberal with the format of note references/definitions

[michaelpj]

Looks cool! Maybe we can even do completions, although that would be ambitious :)

[jvanbruegge]

Hi, I am trying to finish this up, but I have trouble making progress. The current approach does not work because it only takes into account the files that are open in the editor, which is really useless for jump-to-definition. So at ZuriHac after talking to Michael we decided to search for notes in all files on startup and save their locations.

I've been trying to do that now, but I can't get HLS to give me all the files in the project. I found GetKnownFiles and tried to use that from within a pluginRules, but that always returns an empty list if I try to run my unit test. Does anyone have an idea what I am doing wrong? I pushed my WIP code in case that helps

[wz1000]

Your unit test needs to list all the modules in the arguments section of the hie.yaml file for GetKnownFiles to work.

[jvanbruegge]

Thanks, I will try that later

[jvanbruegge]

Ok, I think this is done now. The nix build failures do not seem related. I tried this on the GHC codebase and it does work:
Demo video

[jvanbruegge]

@michaelpj is there anything left to do here?

[jvanbruegge]

I've rebased on the current master now, can this be merged now? @michaelpj

[jvanbruegge]

The test failures are unrelated, the nix job fails because of fourmoulu, and the other test failures are twice in ghcide and once in hls-splice-plugin

[fendor]

I restarted the testsuites, sorry for the delay in responses!

[fendor]

This looks really good to me! I only have requests for more documentation which it is currently lacking and some organisational stuff.

Please add this plugin to the CODEOWNERS file and add yourself as a maintainer.
Additionally, you should add the feature list to the documentation, https://github.com/haskell/haskell-language-server/blob/master/docs/features.md#jump-to-definition and add it is a new plugin to the plugin table https://github.com/haskell/haskell-language-server/blob/master/docs/support/plugin-support.md. If you plan to regularly maintain, add tier 2, otherwise tier 3, please.

[jvanbruegge]

@fendor I had time now to fix this up. I added all the comments you requested and added myself to the codeowners file.

[jhrcek]

I just tried the plugin on hls codebase and it works in some cases, but is broken in others.
Check for example the ghcide/src/Development/IDE/Core/Compile.hs module in hls codebase - even though there's a note "Note [Recompilation avoidance in the presence of TH]"

it gives error whenever I try to go to its definition within the same file (see gif illustrating the issue).
Could you please check what's going on?

EDIT: probably it's because this repo is not correctly using the "note definition" syntax (i.e. note title must be underscored with ~~~ and there should be no intermediate --s between title and the underscore.

So multiline comment like this works

{- Note [bla]
~~~~~~~~~~

But this doesn't

-- Note [bla]
-- ~~~~~~~~

You could potentially fix the notes across the codebase as part of this PR if you feel like it 😄

I'm just wondering if the error messages could be improved a bit to make the reason why note definition was not found more obvious (e.g. "Note definition (a comment of the form "{- Note [TITLE]\n~~~ ... -}") was not found").

[jvanbruegge]

I've addressed the review comments, also made the note format more liberal and improved the error message

[jvanbruegge]

Also added a commit now that should make all notes in the hls codebase compliant with the plugin. Given that I made the format more liberal this only boiled down to adding underscores to a bunch of notes.

[jhrcek]

Thank you!
Looks good to me 👍

[jvanbruegge]

I've also rebased onto the current master now

[jvanbruegge]

I've added a waitForBuildQueue in the tests now and run it with only 5% CPUQuota to simulate a weaker/overloaded machine. If the tests still fail I am out of ideas

[jvanbruegge]

Ok, I found the issue on windows: Line endings. The regex only allowed a single newline character, but on windows it's \r\n. So the regex now uses \r?\n. The one failing test is a flaky test in hs-eval-plugin could someone restart that job?

[jhrcek]

Restarted

[jvanbruegge]

Great, CI is passing now. @fendor I have done the requested changes earlier already.

[jvanbruegge]

I'll go ahead and close this. My motivation to rebase this again because it has been sitting so long is pretty much zero.