hls-notes-plugin: Initial implementation

[jvanbruegge]

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

TODO:

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

[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!

[jvanbruegge]

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

[jvanbruegge]

No idea why this one windows CI job is hanging, but since the last commit the tests of the plugin are also run in CI

[fendor]

I restarted CI, just carry on, I doubt this is your fault, but I will ping you again if I think it is :) Just consider the PR ready for review.

[jvanbruegge]

I've rebased this now again on master which was quite painful due to #3976

[jvanbruegge]

I am confused on why the circleCI is failing. Is says that the import of lift and liftIO are unused, but they are used?

[jhrcek]

hls tells me that both lift and liftIO are actually re-exported by Control.Monad.Except.

Either make export list of this module explicit or remove the unused imports. The reason why it's only failing in stack job is that in stack.yaml we have pedantic: true

[jvanbruegge]

Ahh, yes. Fixed that now

[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 :smile:

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.

[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.