Add unused variable support

[ndmitchell]

Using VSCode you can tag a Diagnostic as DiagnosticTag.Unnecessary as per https://github.com/Microsoft/vscode/blob/83625fa9cbbdec3eab4a635c62f88aab0929aa80/src/vs/vscode.d.ts#L3945 to have them rendered in a faded out manner. I suggest that ghcide should always enable the unused import, unused definition and unused variable warnings, and then map them through as Hint diagnostics (which I guess don't show up in the Problems page?) with Unnecessary as their tag (or higher priority if the user had turned them on too). While I'm not a fan of some of these warnings (unused variables mostly), having the distinction visible in the IDE would be very useful as a feedback sign.

As a note of caution, this tagging is only available in the next version of the LSP spec, so because of how new it is, will require adding to the underlying LSP libraries too.

[cocreature]

There are a couple of somewhat orthogonal changes here:

1. Adding the Unnecessary tag to diagnostics for unused warnings if the client announces support for diagnostic tags. This seems uncontroversial to me and we should do it imho.
2. Turn on unused warnings by default. As mentioned in haskell/ghcide#62, I feel fairly strongly that my editor should not show different warnings than running my build tool so I want this to be configurable. But I’d be fine with this being on by default.
3. Change the severity of unused warnings to Hint. There are two options here: Either we always mark them as a hint or we only mark them as a hint if they are not turned on explicitly. The latter seems to make more sense to me since I would expect that if a user turns them on explicitly, they do see them as something to fix. They will still be tagged which iiuc will result in them having a different color in VSCode which is useful during development since ime even if you do care about those warnings they are usually the ones you fix just before commiting and not while you are writing code.

So to summarize, I’m proposing to

1. Always tag them as Unnecessary if the client supports this.
2. Add a CLI flag to turn them on by default.
3. Change the severity of unused warnings to Hints if the user has not enabled them explicitly.

[ndmitchell]

I was proposing 1 and 3, but not 2. To my mind, the ability to enable additional warnings for the IDE only should be controlled in .hie.yaml, and ghcide should just respect that.

[cocreature]

So you aren’t proposing to turn them on by default? As I mentioned, I really want to be able to turn this off and I don’t see how hie.yaml lets me do this.

[ndmitchell]

I think by default its reasonable to gray out the unused variables. I believe in VS Code you can achieve that by making them very low-level diagnostics, which are so low level they don't get onto the problems page. So if the user hasn't explicitly turned them on, we turn them on and reduce their severity until you are only left with the gray text (which I think everyone should want).

[cocreature]

I still really dislike the idea of turning on warnings by default with no way to disable them. That reminds me of static analysis tools with a ton of false positives that people just stop using at some point because the signal to noise ratio is too low. Given that I actually like this particular warnings, I’m fine with turning them on by default but I want to at least have a CLI flag to disable that which shouldn’t add much complexity in the implementation given that we already need to differentiate between the warning being enabled explicitly in which case we don’t want to demote it to a hint and the warning being part of the defaults.

[ndmitchell]

You mean you don't want to see the variables in gray? Because that's the only proposed effect I am suggesting from "turning on the warning". Note that in most other IDE's you get the gray by default, and in VS Code, you can set the "unused variable color" for a particular language, and setting it to no effect is equivalent to turning off this warning.

[cocreature]

My issue is with “turning on the warning” not making them gray. I don’t want to see warnings that I don’t care about even if they are only displayed in gray and if I do care about them then I want them to be shown when I run my build tool in a terminal as well.

[ndmitchell]

I propose that we:

• Don't display the diagnostic to the user at all in any form.
• When syntax coloring the text the user has written, in the editor, put the variable that is unused in gray.

This is how TypeScript and similar work in VS Code.

[cocreature]

We don’t have control over how the client displays hint diagnostics and I don’t want to rely on how VSCode handles this. From the server side all we can do is mark them as hints and add the unnecessary tag.

I think we are mostly in agreement here. All I’m saying is that I want to have something like a --vanilla-ghc flag (happy to hear suggestions for a better name) that makes ghcide not emit any diagnostics that the user hasn’t turned on explicitly and thereby makes it act like your regular build tool does.

[ndmitchell]

Sure, no objection to that - I would rather the way of emitting unused variables wasn't the same as a diagnostic, or we could have a Fake tag on the diagnostic so all clients knew it wasn't real.

[cocreature]

It isn’t clear to me from the spec that clients are supposed to handle an unknown tag gracefully so I’m a bit hesitant to use a fake tag. I think marking it as a hint should probably give us a reasonably sane behavior in most clients which makes it hard to justify trying to come up with something other than diagnostics which would require custom support for ghcide in every client.

[ndmitchell]

Sorry, I meant what we want is some predefined tag like Fake which has a semantic guarantee we are looking for. I wasn't suggesting we invent a new tag - agree that Hint is fine.

I wonder if vanilla flag should also disable the deferred type errors treatment too, as that differs from what normal tools give.

[cocreature]

The deferred type errors remap the warning to an error in ghcide so the only difference is that it continues processing other files which seems small enough that I’d be fine with not having that be part of the vanilla flag but if there’s a demand for it I’d definitely be open to changing that or to having a separate flag to turn that off.

[serras]

I think that once haskell/ghcide#232 lands, we can reuse the same infrastructure for "unused" warnings. In fact, I volunteer to implement this feature once the other PR is merged.

[serras]

Unfortunately, it seems like haskell-lsp-types version 0.19 does not expose that field :(

[jneira]

@serras just in case, haskell-lsp-0.22 already exposes the field: http://hackage.haskell.org/package/haskell-lsp-types-0.22.0.0/docs/Language-Haskell-LSP-Types.html#t:Diagnostic 😃

[serras]

@jneira thanks for the pointer! now I have homework for the weekend :)

[serras]

I could not wait until the weekend! haskell/ghcide#815 is in

[jneira]

Closed by haskell/ghcide#815, thanks @serras!