-
-
Notifications
You must be signed in to change notification settings - Fork 353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support structured diagnostics #4311
base: master
Are you sure you want to change the base?
Support structured diagnostics #4311
Conversation
We're leaving the TODOs for either later in this PR or in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
12b364a
to
73dc069
Compare
73dc069
to
003c15c
Compare
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
which makes me think this approach is not going to be easy. |
Gonna take a break for today, but two big TODOs remain, and a few open questions. |
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. |
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 |
I don't think we should serialise the raw data over the wire, instead we should stash it in some sort of |
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. |
Had to move `attachReason` between modules to achieve this, which is fine because it was never exported from its own module.
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. |
stylish-haskell does not handle CPP pragmas very well, is this a regression?
@@ -884,13 +885,28 @@ newComponentCache recorder exts _cfp hsc_env old_cis new_cis = do | |||
hscEnv' <- -- Set up a multi component session with the other units on GHC 9.4 | |||
Compat.initUnits dfs hsc_env | |||
|
|||
let closure_errs = checkHomeUnitsClosed' (hsc_unit_env hscEnv') (hsc_all_home_unit_ids hscEnv') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Addresses #2014
Should hopefully enable #3246
TODOs:
Resolve TODOs in codeAdd diagnostic codes to more testsMake PR on GHC side so that we can eventually removeghcide/src/Development/IDE/GHC/Compat/Driver.hs
hls-plugin-api
using_code
field set bywithWarnings
to determine which warning to use.Additional work (future PRs):
Maybe