haskell-language-server icon indicating copy to clipboard operation
haskell-language-server copied to clipboard

Support structured diagnostics

Open dylan-thinnes opened this issue 1 year ago • 9 comments

Addresses https://github.com/haskell/haskell-language-server/issues/2014

Should hopefully enable https://github.com/haskell/haskell-language-server/issues/3246

TODOs:

  • [x] ~~Resolve TODOs in code~~
  • [x] ~~Add diagnostic codes to more tests~~
  • [x] ~~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
  • [x] 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

dylan-thinnes avatar Jun 10 '24 15:06 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 avatar Jun 16 '24 14:06 dylan-thinnes

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

dylan-thinnes avatar Jun 16 '24 14:06 dylan-thinnes

Re: deriving JSON instances, the messages often contain fragments of AST and other things which aren't terribly friendly to ToJSON/FromJSON.

This tickled my brain and I remembered that GHC has its own JSON conversion functions! https://gitlab.haskell.org/ghc/ghc/-/blob/master/compiler/GHC/Utils/Json.hs?ref_type=heads

And they definitely have instances for diagnostics, since they're used in the JSON output for GHC, see e.g. here: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11093

Having written this I realise that that only handles one direction, so it doesn't help us much, oh well.

michaelpj avatar Jun 17 '24 10:06 michaelpj

They might have another serialization format, though. There's nothing stopping us from serializing them to bytes and the smuggling a bytestring as a JSON string in data :joy:

michaelpj avatar Jun 17 '24 10:06 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.

wz1000 avatar Jun 20 '24 08:06 wz1000

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.

michaelpj avatar Jun 20 '24 08:06 michaelpj

One remaining point to address from review: https://github.com/haskell/haskell-language-server/pull/4311#discussion_r1651757991, have made a commit for it https://github.com/haskell/haskell-language-server/commit/414c845a74ef64ded31c7486607aa90004bab56e

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

dylan-thinnes avatar Jun 24 '24 23:06 dylan-thinnes

@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 avatar Sep 05 '24 12:09 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 (..),

noughtmare avatar Sep 05 '24 15:09 noughtmare

Closing in favour of #4433

fendor avatar Dec 23 '24 10:12 fendor