Support structured diagnostics

[dylan-thinnes]

Addresses #2014

Should hopefully enable #3246

TODOs:

• Resolve TODOs in code
• Add diagnostic codes to more tests
• Make PR on GHC side so that we can eventually remove ghcide/src/Development/IDE/GHC/Compat/Driver.hs
  • Issue https://gitlab.haskell.org/ghc/ghc/-/issues/24996 and MR https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12891
• Deal with CPP to make this work properly on all versions
• Fix hls-plugin-api using _code field set by withWarnings to determine which warning to use.

Additional work (future PRs):

• Replacing regexes with structured diagnostic checks
• Improve typing so that it can be known that an error has a structured diagnostic, instead of defaulting to Maybe

[michaelpj]

Possibly insane suggestion: could we write some orphan JSON instances for GHC's message types and stuff the GhcMessage into the data field on the Diagnostic? Then we don't have to carry anything else around, and we won't lose things going via the DiagnosticStore?

[dylan-thinnes]

Re: deriving JSON instances, the messages often contain fragments of AST and other things which aren't terribly friendly to ToJSON/FromJSON. I tried deriving an instance and immediately came up against

No instance for ‘ToJSON (Language.Haskell.Syntax.Binds.HsBindLR GHC.Hs.Extension.GhcRn GHC.Hs.Extension.GhcRn)’ 

which makes me think this approach is not going to be easy.

[dylan-thinnes]

Gonna take a break for today, but two big TODOs remain, and a few open questions.

[michaelpj]

I don't think we should serialise the raw data over the wire, instead we should stash it in some sort of Map and send over an identifier/key which we can later use to look up the value from the Map. It requires care to ensure that the values are properly garbage collected when no longer valid though.

There's no viable serialization format anyway. I do think it would be nice if there was one, though.

That said, I don't think we should need this info in diagnostics that we e.g. get back from the client. So the main issue is when we e.g. ask for the diagnostics covering a range so we can then match on them to work out code actions. At that point we need the GHC diagnostics. So I don't think we need a separate store, at worst we might have to augment our server-side diagnostic store.

[dylan-thinnes]

One remaining point to address from review: #4311 (comment), have made a commit for it 414c845

I'm looking into the CI failures now - I assume some CPP shenanigans are at least part of the issue.

[jhrcek]

Seems like this section can now be nicely DRYied like this (also taking advantage of the fact that we recently dropped support for GHCs < 9.4, so MIN_VERSION_ghc(9,3,0) will always be true going forward):

 let closure_errs = maybeToList $ checkHomeUnitsClosed' (hsc_unit_env hscEnv') (hsc_all_home_unit_ids hscEnv') closure_err_to_multi_err diag = ideErrorWithSource (Just "cradle") (Just DiagnosticSeverity_Warning) _cfp (T.pack (Compat.printWithoutUniques (singleMessage diag))) #if MIN_VERSION_ghc(9,6,1) (Just (fmap GhcDriverMessage diag)) #else Nothing #endif multi_errs = map closure_err_to_multi_err closure_errs bad_units = OS.fromList $ concat $ do x <- map errMsgDiagnostic closure_errs DriverHomePackagesNotClosed us <- pure x pure us isBad ci = (homeUnitId_ (componentDynFlags ci)) `OS.member` bad_units

[noughtmare]

@dylan-thinnes are you still planning to work on this? The immediate CI issue seems very simple to solve. It seems there is just a redundant import. What else still needs to happen?

[noughtmare]

Removing these four redundant imports fixes all issues for me locally:

diff --git a/plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Diagnostics.hs b/plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Diagnostics.hs index 5425c419..a0cea4dc 100644 --- a/plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Diagnostics.hs +++ b/plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Diagnostics.hs @@ -13,8 +13,7 @@ where import Control.Lens ((&), (.~)) import qualified Data.Text as T -import Development.IDE (FileDiagnostic, - ShowDiagnostic (ShowDiag)) +import Development.IDE (FileDiagnostic) import Development.IDE.Types.Diagnostics (fdLspDiagnosticL, ideErrorWithSource) import Distribution.Fields (showPError, showPWarning) diff --git a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs index c9ce440f..35f41ea3 100644 --- a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs +++ b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs @@ -87,7 +87,6 @@ import Language.LSP.Protocol.Types (ApplyWorkspa CodeActionKind (CodeActionKind_QuickFix), CodeActionParams (CodeActionParams), Command, - Diagnostic (..), MessageType (..), Null (Null), ShowMessageParams (..), diff --git a/plugins/hls-refactor-plugin/test/Main.hs b/plugins/hls-refactor-plugin/test/Main.hs index 377a6758..434fe48b 100644 --- a/plugins/hls-refactor-plugin/test/Main.hs +++ b/plugins/hls-refactor-plugin/test/Main.hs @@ -21,7 +21,6 @@ import Data.Foldable import Data.List.Extra import Data.Maybe import qualified Data.Text as T -import Data.Tuple.Extra import Development.IDE.GHC.Util import Development.IDE.Plugin.Completions.Types (extendImportCommandId) import Development.IDE.Test diff --git a/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs b/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs index 1fc7fa42..51e00550 100644 --- a/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs +++ b/plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs @@ -14,7 +14,6 @@ import qualified Data.Text as T import Development.IDE import Development.IDE.Core.Rules (getHieFile) import qualified Development.IDE.Core.Shake as Shake -import Development.IDE.Types.Diagnostics import GHC.Generics (Generic) import Ide.Plugin.Config (PluginConfig (..)) import Ide.Types (PluginDescriptor (..),

[fendor]

Closing in favour of #4433