Tag unused warning as such

[serras]

This patch uses the new "unnecessary" or "deprecated" tags for diagnostics to show unused variables/functions/... dimmed in the editor, as discussed in https://github.com/haskell/ghcide/issues/165#issuecomment-692532054

[serras]

Mandatory screenshot (notice the grey color)

[ndmitchell]

Awesome! One question - I usually don't turn off unused variables, just unused top-level bindings. It would be great to still get the grey color. Could we always turn these warnings on, but suppress them from making it to the diagnostics if we turned them on rather than the user?

[serras]

That sounds like a good idea, it shouldn't be more difficult than the unDefer bussiness in the same file.

[serras]

It took a bit more than expected, mostly because some tests broke because of that. In fact, locally all the benchmarks fail for me, but I haven't been able to fix them.

[ndmitchell]

Looks awesome!

[ndmitchell]

I assume HideDiag means they don't appear at all? Could you just look at _tags ahd only Hide the ones with no tags? (I've no idea here, somewhat guessing)

[serras]

I think that's a good idea: make those with tags "less important" but not hide them.

Implemented in the latest commit.

[ndmitchell]

Could we get a comment on what this does and why? (From the code, its clear, but nice to be able to eyeball it without reading the code)

[serras]

Added comment to the file:

-- | Add a unnecessary/deprecated tag to the required diagnostics.

[ndmitchell]

Ususally foo_ would be the version of foo that does slightly less than foo, e.g. doesn't return some info. Could we call expectDiagnostics_ something like expectedDiagnosticsTags?

[serras]

I've renamed it to expectDiagnosticsWithTags.

[ndmitchell]

The really important thing is that these diagnostics have the right tag associated with them. While they don't all need to be expectDiagnostics_ (or whatever it ends up being called), a few more being that with the tag assertion would be good.

[serras]

That makes sense. I've extended the checks in the latest commit.

[ndmitchell]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ndmitchell]

I see some test failure:

FAIL (0.59s)
      test/src/Development/IDE/Test.hs:77:
      Got unexpected diagnostics for Uri {getUri = "file:///tmp/extra-dir-28584335661024/ModuleA.hs"} got List [Diagnostic {_range = Range {_start = Position {_line = 1, _character = 0}, _end = Position {_line = 1, _character = 29}}, _severity = Just DsInfo, _code = Nothing, _source = Just "typecheck", _message = "The import of \8216ModuleB\8217 is redundant\n  except perhaps to import instances from \8216ModuleB\8217\nTo import instances alone, use: import ModuleB()", _tags = Just (List [DtUnnecessary]), _relatedInformation = Nothing}]

I did have to fix a merge conflict to get it going again. So certainly possible that's my fault.

[serras]

I see some test failure:

FAIL (0.59s)
      test/src/Development/IDE/Test.hs:77:
      Got unexpected diagnostics for Uri {getUri = "file:///tmp/extra-dir-28584335661024/ModuleA.hs"} got List [Diagnostic {_range = Range {_start = Position {_line = 1, _character = 0}, _end = Position {_line = 1, _character = 29}}, _severity = Just DsInfo, _code = Nothing, _source = Just "typecheck", _message = "The import of \8216ModuleB\8217 is redundant\n  except perhaps to import instances from \8216ModuleB\8217\nTo import instances alone, use: import ModuleB()", _tags = Just (List [DtUnnecessary]), _relatedInformation = Nothing}]

I did have to fix a merge conflict to get it going again. So certainly possible that's my fault.

I've spent some time and fixed the test. Hopefully everything should be green in some minutes.

[ndmitchell]

Awesome work - this is a great feature. Thanks for all your hard to work to write it and adjust so much of the test suite.