Add code actions for disabling a warning in the current file #897

georgefst · 2020-11-07T16:14:25Z

This is a little ugly - we need some way to keep track of the WarningFlag, so we store it as a string in the (previously unused by Ghcide) _code field of Diagnostic. I'm open to a better way.

Closes haskell/haskell-language-server#571.

georgefst · 2020-11-07T16:15:34Z

src/Development/IDE/Plugin/CodeAction.hs

+suggestDisableWarning Diagnostic{..}
+    | Just (StringValue (showFlag -> Just w)) <- _code =
+        pure
+            ( "Disable \"" <> w <> "\" warnings"


Exact wording of message is bikeshed-able

src/Development/IDE/Plugin/CodeAction.hs

pepeiborra

A test would be great

src/Development/IDE/Plugin/CodeAction.hs

wz1000 · 2020-11-07T22:12:14Z

src/Development/IDE/GHC/Warnings.hs

        modifyVar_ warnings $ return . (wr_d:)
  res <- action $ \x -> x{ms_hspp_opts = (ms_hspp_opts x){log_action = newAction}}
  warns <- readVar warnings
  return (reverse $ concat warns, res)
+
+attachReason :: WarnReason -> Diagnostic -> Diagnostic
+attachReason wr d = d{_code = StringValue <$> showReason wr}


This seems pretty weird, especially since the specification says this field may be displayed to the user. Perhaps the reason can be recorded in the ide state, and the code action can be generated based off of that?

This seems pretty weird, especially since the specification says this field may be displayed to the user.

Hmm, yeah, that is unfortunate. I was originally intending to use the more human-readable form here, analogously to how GHC shows e.g. -Wunused-imports in the CLI output. But this way it was easier to be sure that a particular diagnostic comes from a GHC warning (using T.stripPrefix "Opt_Warn").

Perhaps the reason can be recorded in the ide state, and the code action can be generated based off of that?

That sounds promising. There's a diagnostics field in ShakeExtras but the DiagnosticStore type doesn't seem to give us anywhere to store extra information, so I guess it would need replacing - I haven't yet looked at how much of a knock-on effect that might have.

I'm struggling to see a nice way to put this information in ShakeExtras.

Besides, based on other LSP servers, I think this actually is more or less the intended use case for the code field. I'm thinking we put the full flag there e.g. -Wunused-imports, and trigger the action for diagnostics whose code begins with "-W" Still a little bit ugly, but the very nature of LSP means we're bound to be resigned to the occasional but of string-ly typed code.

The way VScode at least shows this is actually quite nice: typecheck(-Wunused-imports). Regardless of the original purpose of this PR, it's nice to get this extra information about where a warning is coming from. In fact, it's the one thing in GHC's output that we're currently missing.

I'm thinking we put the full flag there e.g. -Wunused-imports

This is what c2cd237 does.

The code field is intended to refer to the "handbook code" for a diagnostic, as provided by some compilers. Version 3.16 of the spec has a data field for this, prompted from microsoft/language-server-protocol#887. Perhaps use that instead.

I think seeing as the flag is the closest thing to a "handbook code" GHC is likely to have any time soon, this is still the right place to put it. Especially given that the result is visually appealing in VScode (haven't tried other editors, but I'd hope they show something relatively similar).

Obviously I'll change this if you or any of the other core maintainers feel strongly.

pepeiborra

This looks good, but let's fix the location of the inserted pragmas to make sure the new flags are not overridden by later pragmas

src/Development/IDE/Plugin/CodeAction.hs

pepeiborra · 2020-11-22T16:15:18Z

What's left to merge this?

georgefst · 2020-11-26T22:56:26Z

Assuming @wz1000 is now happy with the _code field being used, after my comments, then the remaining issue is what to do about showFlag. I think what I'll have to do is just spell out all the cases (effectively duplicating flagSpecOf). Or spell out the 14 broken cases, then fall back to the current implementation - this is similar to what we do for passing flags to Ormolu/Fourmolu in HLS, where -XCPP needs to be special cases, but it's a little brittle.

Meanwhile, shall I open a PR on GHC's GitLab about exporting flagSpecOf? I don't know enough about that part of the code to know whether it would be sensible.

ndmitchell · 2020-11-27T09:27:41Z

Meanwhile, shall I open a PR on GHC's GitLab about exporting flagSpecOf? I don't know enough about that part of the code to know whether it would be sensible.

Yes. As a general approximation, if its useful, it should be exported from GHC. GHC isn't particularly careful about presenting a nice carefully crafted API. We've asked for things exported in the past - if it will be exported, adding it to the Compat layer for now and then introducing an ifdef to make it available later is a good way to go.

georgefst · 2020-11-27T18:14:09Z

Ok, thanks @ndmitchell, I'll do that.

Just to note, I'm very busy at the moment - in the middle of changing jobs and moving between cities. I'll probably get to this next weekend.

georgefst · 2020-12-18T22:45:10Z

Done.

georgefst · 2020-12-27T16:30:50Z

Ok, now this really is finished. Thanks @mpickering for pointing out wWarningFlags.

georgefst · 2020-12-27T16:35:10Z

What 5a2ffa4 does is essentially inline the definition of flagSpecOf.

It is a bit inefficient as it effectively uses a list as a map. But we don't have Ord for WarningFlag. IntMap could be an option...

Then again, it's a list with bounded length (number of warnings - somewhere under 100), so it's not disastrous.

pepeiborra · 2020-12-27T18:00:16Z

Thank you @georgefst ! Please take a look at the merge conflicts so that we can merge this ASAP

georgefst · 2020-12-27T19:03:13Z

@pepeiborra Rebased and resolved.

pepeiborra · 2020-12-28T09:58:01Z

Looks like there are still some test failures

georgefst · 2020-12-28T21:48:25Z

Looks like there are still some test failures

Ah, whoops. I'd only been running the ones I added. Turns out three of the old ones are broken by the presence of an additional code action.

64a0aa5 does the bare minimum to get the tests fixed but in some cases they should perhaps actually be made more robust instead...

georgefst · 2020-12-28T21:49:37Z

test/exe/Main.hs

@@ -1042,7 +1044,7 @@ removeImportTests = testGroup "remove import actions"
            ]
      doc <- createDoc "ModuleC.hs" "haskell" content
      _ <- waitForDiagnostics
-      [_, _, _, _, CACodeAction action@CodeAction { _title = actionTitle }]
+      [_, _, _, _, _, _, _, _, CACodeAction action@CodeAction { _title = actionTitle }]


This is particularly nasty.

pepeiborra · 2020-12-29T10:59:39Z

The ghcide Github project is becoming archived and merged into https://github.com/haskell/haskell-language-server

This PR will need to be reopened in the HLS repo. ~~To do that, create a new branch from HLS HEAD in your HLS repo and do~~:

git remote add ghcide https://github.com/georgefst/ghcide.git
git fetch ghcide
git merge ghcide disable-warnings

Thanks!

EDIT: we rewrote the ghcide commit messages in the HLS repo to fix the links to GitHub Issues/PRs which has the side-effect of breaking the merge instructions above. Instead, you should use cherry-pick as below:

git remote add ghcide https://github.com/georgefst/ghcide.git
git fetch ghcide
git cherry-pick --ff 53bbfe1d 85128c96 d007049b 1437c496 b31bf505 95aa3b39 64a0aa55

georgefst commented Nov 7, 2020

View reviewed changes

src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved

pepeiborra reviewed Nov 7, 2020

View reviewed changes

src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved

georgefst commented Nov 7, 2020

View reviewed changes

src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved

wz1000 reviewed Nov 7, 2020

View reviewed changes

georgefst marked this pull request as ready for review November 14, 2020 16:24

Anrock mentioned this pull request Nov 15, 2020

Add code action to suppress specific hlint warnings haskell/haskell-language-server#600

Open

pepeiborra suggested changes Nov 15, 2020

View reviewed changes

src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved

pepeiborra approved these changes Nov 15, 2020

View reviewed changes

georgefst added 6 commits December 27, 2020 18:54

Add code action for disabling a warning

53bbfe1

Use flag as code field

85128c9

Add tests for disabling warnings

d007049

Insert pragma below existing pragmas

1437c49

Implement 'showFlag' explicitly

b31bf50

Use 'wWarningFlags' to avoid hardcoding 'showFlag'

95aa3b3

georgefst force-pushed the disable-warnings branch from 5a2ffa4 to 95aa3b3 Compare December 27, 2020 19:02

Fix tests

64a0aa5

georgefst commented Dec 28, 2020

View reviewed changes

pepeiborra closed this Dec 29, 2020

wz1000 mentioned this pull request Dec 29, 2020

Bring over unmerged PRs from ghcide haskell/haskell-language-server#705

Closed

7 tasks

georgefst mentioned this pull request Jan 19, 2021

Add code actions for disabling a warning in the current file haskell/haskell-language-server#1235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add code actions for disabling a warning in the current file #897

Add code actions for disabling a warning in the current file #897

georgefst commented Nov 7, 2020

georgefst Nov 7, 2020

pepeiborra left a comment

wz1000 Nov 7, 2020

georgefst Nov 8, 2020

georgefst Nov 10, 2020

georgefst Nov 14, 2020

alanz Nov 29, 2020

georgefst Dec 18, 2020

pepeiborra left a comment

pepeiborra commented Nov 22, 2020

georgefst commented Nov 26, 2020

ndmitchell commented Nov 27, 2020

georgefst commented Nov 27, 2020

georgefst commented Dec 18, 2020

georgefst commented Dec 27, 2020

georgefst commented Dec 27, 2020

pepeiborra commented Dec 27, 2020

georgefst commented Dec 27, 2020

pepeiborra commented Dec 28, 2020

georgefst commented Dec 28, 2020

georgefst Dec 28, 2020

pepeiborra commented Dec 29, 2020 •

edited

Add code actions for disabling a warning in the current file #897

Add code actions for disabling a warning in the current file #897

Conversation

georgefst commented Nov 7, 2020

Choose a reason for hiding this comment

pepeiborra 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

Choose a reason for hiding this comment

pepeiborra left a comment

Choose a reason for hiding this comment

pepeiborra commented Nov 22, 2020

georgefst commented Nov 26, 2020

ndmitchell commented Nov 27, 2020

georgefst commented Nov 27, 2020

georgefst commented Dec 18, 2020

georgefst commented Dec 27, 2020

georgefst commented Dec 27, 2020

pepeiborra commented Dec 27, 2020

georgefst commented Dec 27, 2020

pepeiborra commented Dec 28, 2020

georgefst commented Dec 28, 2020

Choose a reason for hiding this comment

pepeiborra commented Dec 29, 2020 • edited

pepeiborra commented Dec 29, 2020 •

edited