Support structured diagnostics 2 #4433

noughtmare · 2024-10-17T08:13:03Z

This is my attempt at finishing the work of @dylan-thinnes on !4311.

Addresses #2014

Should hopefully enable #3246

These are two things Dylan listed as work for 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

Some future work discovered in reviews of this PR:

requireDiagnostic could take a more structured error code query type as argument. See Support structured diagnostics 2 #4433 (comment)
The HTTP work should be un-reverted in a future PR: 1bc221c
Check that tests using requireDiagnostics and related functions use explicit error codes if possible

noughtmare · 2024-10-17T09:35:36Z

All failures in the jobs of e651d41 seem to be spurious (except for the stylish-haskell formatting failure).

noughtmare · 2024-10-17T15:39:08Z

I think the remaining failures are not caused by this PR. Are these kinds of spurious failures common in HLS?

Also, I would really like to get this merged before the next release, because I want to develop a VS Code extension which uses these diagnostic numbers.

fendor · 2024-10-18T07:05:18Z

Are these kinds of spurious failures common in HLS?

Yes, they unfortunately are

fendor · 2024-10-18T09:28:29Z

Some errors in ghc 9.4 seem to be genuine, though. Many tests are seemingly time outing for some reason.

noughtmare · 2024-10-18T12:01:57Z

You're right. Can anyone give me some tips on how to profile HLS to see where these timeouts are coming from?

fendor · 2024-10-18T12:27:26Z

@noughtmare Profiling is likely not necessary, I think the tests are waiting for an LSP message that never arrives, and then just wait for 60s and time out.

Use HLS_TEST_LOG_STDERR=1 cabal test ... and TASTY_PATTERN to run a specific test with all HLS logging enabled.
Printf debugging also works quite well to figure out where the tests gets stuck.
To get the raw LSP messages, you can use LSP_TEST_LOG_MESSAGES=1 or LSP_TEST_LOG_STDERR=1 (they do the same thign)

noughtmare · 2024-10-18T12:33:06Z

Ah, I was thinking there was some inefficiency somewhere that only GHC 9.6 and later could properly optimize. However, I think you're suggesting an alternative explanation: that one of the conditional CPP blocks contains an actual correctness issue. Perhaps I could just visually inspect those first; I don't think that would be that much work.

Edit: no obvious loops or anything, so perhaps some tracing would indeed be better.

ghcide/src/Development/IDE/GHC/Compat.hs

ghcide/src/Development/IDE/GHC/Compat/Driver.hs

noughtmare · 2024-10-22T21:10:29Z

I've decided to make the tests ignore all error codes on GHC 9.4. I think some errors already have codes in 9.4, so we could do a bit more, but I don't think it is worth the effort.

noughtmare · 2024-10-23T07:34:27Z

The remaining MacOS failure seems like a compiler bug. The linker is throwing an error.

Edit: https://gitlab.haskell.org/ghc/ghc/-/issues/24648

Adding the compiler flag -ld_classic might be a workaround.

noughtmare · 2024-10-23T11:40:55Z

@fendor do you think we could try setting the -ld_classic flag? If so, where should I put it?

noughtmare · 2024-10-23T11:45:02Z

Oh, I see we already have that exact workaround in some places. I'll just copy that to the hlint plugin.

noughtmare · 2024-10-23T13:34:29Z

Yay, it has finally passed CI! Should I squash the commits? I'm not sure what to write in the message as I don't really know the details of what exactly Dylan did.

We're leaving the TODOs for either later in this PR or in another PR

noughtmare · 2024-11-02T11:10:45Z

Thank you for the reviews! I hope I have addressed all feedback appropriately. Is there anything more I can do to speed up the process of merging this?

fendor · 2024-11-02T15:25:51Z

Probably not, except for regularly pinging on this thread to make sure we get enough reviews, soon :) We need two approvals, considering the importance of this change, we shouldn't rush merging it without proper review time.

However, I hope we can merge it within the next two weeks (hopefully a pessimistic timeframe, but I don't really want to give any time estimate at all :D ).

soulomoon

Just a question, other places look good to me.

ghcide/src/Development/IDE/Types/Diagnostics.hs

plugins/hls-refactor-plugin/test/Main.hs

noughtmare · 2024-11-19T14:27:11Z

regularly pinging

ping

fendor · 2024-11-19T15:04:26Z

In the last HLS meeting, we discussed this PR and what we want from it, for transparency, these are the notes https://docs.google.com/document/d/1zqRhA3uoiYgA_Q8eDpIEG0OiFcofGqkVP7N4ODDmikM/edit?tab=t.0

In short, I will review soon and try to make use of the structured diagnostics in 1 or 2 of the plugins, to demonstrate how we can use them in the future and to make sure this PR takes the right directions.
Are you fine with me pushing to this branch, or should I open PRs against your branch?

noughtmare · 2024-11-19T15:25:06Z

I'm fine with pushing directly to my branch if that is possible. Do I need to give you special permissions?

fendor · 2024-11-19T15:27:39Z

I don't know, I will just try to push at some point and ping you if I have any trouble :)

This reverts commit 0776c65.

Implement 'rangesOverlap' function which checks whether two 'Range's overlap in any way. Implement two new plugin utility functions which allow to conveniently get all currently displayed diagnostics for a given 'Range'.

Add compatibility module for GHC's structured error messages. Introduce 'Prism's and 'Lens's to easily access nested structures. Expand documentation for 'StructuredMessage'

fendor

It took me long enough, but I am happy with the PR now.

I still have to address my own comments, and likely I have to revert any changes to the plugins and split these commits off into a separate PR.

fendor · 2024-12-01T17:28:25Z

ghcide/src/Development/IDE/Core/Compile.hs

+#if MIN_VERSION_ghc(9,5,0)
                 throwE $ diagFromErrMsgs sourceParser dflags errors
+#else
+                 throwE $ diagFromSDocErrMsgs sourceParser dflags errors
+#endif


Note to myself, check whether we can hide this CPP in a function such as diagFromGhcErrorMessage with appropriate #ifs and typedefs to avoid the CPP statements here.

fendor · 2024-12-01T17:28:38Z

ghcide/src/Development/IDE/Core/RuleTypes.hs

@@ -157,6 +158,7 @@ data TcModuleResult = TcModuleResult
        -- ^ Which modules did we need at runtime while compiling this file?
        -- Used for recompilation checking in the presence of TH
        -- Stores the hash of their core file
+    , tmrWarnings        :: WarningMessages


Note to self, add docs for this.

fendor · 2024-12-01T17:31:15Z

ghcide/src/Development/IDE/GHC/Compat/Outputable.hs

+#if MIN_VERSION_ghc(9,5,0)
+type ErrMsg  = MsgEnvelope GhcMessage
+type WarnMsg  = MsgEnvelope GhcMessage
+#else
 type ErrMsg  = MsgEnvelope DecoratedSDoc
 type WarnMsg  = MsgEnvelope DecoratedSDoc
+#endif


Note to self, this feels like the wrong direction, we should have type defs for backwards compatibility, i.e.

#if !MIN_VERSION_ghc(9,5,0) type GhcMessage = DecoratedSDoc ```endif

which allows us to write code conforming to the latest GHC API.

I think it is more confusing and less maintainable in the long run, if we add type defs to maintain the previous API.

fendor · 2024-12-01T17:31:28Z

ghcide/src/Development/IDE/GHC/Warnings.hs

+{-
+ NOTE on withWarnings and its dangers


Note to self, improper GHC Note syntax

fendor · 2024-12-01T17:33:00Z

@noughtmare Is there anythign else you need or want from this PR?

noughtmare · 2024-12-01T17:54:41Z

I personally just wanted access to the diagnostic codes. So I'm more than satisfied!

soulomoon

LGTM, Just two questions

soulomoon · 2024-12-02T14:14:35Z

plugins/hls-class-plugin/src/Ide/Plugin/Class/CodeAction.hs

@@ -80,23 +86,25 @@ addMethodPlaceholders _ state _ param@AddMinimalMethodsParams{..} = do
 -- This implementation is ad-hoc in a sense that the diagnostic detection mechanism is
 -- sensitive to the format of diagnostic messages from GHC.
 codeAction :: Recorder (WithPriority Log) -> PluginMethodHandler IdeState Method_TextDocumentCodeAction
-codeAction recorder state plId (CodeActionParams _ _ docId _ context) = do
+codeAction recorder state plId (CodeActionParams _ _ docId caRange _) = do


If I understand correctly, now we are getting the diags from diags store in ShakeExtra instead of CodeActionContext.
Two questions:

Does it mean diags in ShakeExtra contains more information than the one in the CodeActionContext now ?

Do these two places' diags always in sync (e.g., is there additional filters for diags in the CodeActionContext, but those filters not in diags store in ShakeExtra)?

Indeed, that's exactly what we are doing :)

Re 1., I think ShakeExtra also contains Diagnostics which are not displayed to the user, see the docs for ShowDiagnostic:

-- | Defines whether a particular diagnostic should be reported -- back to the user. -- -- One important use case is "missing signature" code lenses, -- for which we need to enable the corresponding warning during -- type checking. However, we do not want to show the warning -- unless the programmer asks for it (#261). data ShowDiagnostic = ShowDiag -- ^ Report back to the user | HideDiag -- ^ Hide from user deriving (Eq, Ord, Show)

This means, we may have more diagnostics available.

Re 2., I confidently, but without evidence, claim that the diagnostics sent by the client and the diagnostics we store in StoreExtras may be out of sync, but it has no noticeable negative effect for the user experience.

However, why are we doing this? Primarily due to the docs of CodeActionContext:

/** * An array of diagnostics known on the client side overlapping the range * provided to the `textDocument/codeAction` request. They are provided so * that the server knows which errors are currently presented to the user * for the given range. There is no guarantee that these accurately reflect * the error state of the resource. The primary parameter * to compute code actions is the provided range. */ diagnostics: Diagnostic[];

Which tells us that we cannot rely on the diagnostics provided by the CodeActionContext.
I will add this crucial bit of information to the documentation of activeDiagnostics... function.

Thanks, adding it to the docs helps for the future reviewers.:)

soulomoon · 2024-12-02T14:16:13Z

plugins/hls-class-plugin/src/Ide/Plugin/Class/Types.hs

        <+> pretty (show (getOccString cls) <> ":") -- 'show' is used here to add quotes around the class name
-        <+> pretty methods
+        <+> pretty (showSDocUnsafe $ ppr methods)


Are we safe with showSDocUnsafe ?

I think that's fine for logging, unsafe doesn't mean it might crash, but rather we use default DynFlags which may be missing some information, such as import paths, packages in scope, etc... So, the displayed names may be inaccurate. However, I can double-check what we do in other locations

Maybe we should get the more exact DynFlags from rule like GetModSummary, but, I agree, for logging, it should be fine as long as it won't crash.

noughtmare requested review from 0rphee, santiweight, eddiemundo, fendor and wz1000 as code owners October 17, 2024 08:13

noughtmare commented Oct 18, 2024

View reviewed changes

ghcide/src/Development/IDE/GHC/Compat.hs Outdated Show resolved Hide resolved

noughtmare commented Oct 20, 2024

View reviewed changes

ghcide/src/Development/IDE/GHC/Compat/Driver.hs Outdated Show resolved Hide resolved

noughtmare requested a review from michaelpj as a code owner October 23, 2024 13:36

noughtmare force-pushed the support-structured-diagnostics branch from 7773572 to 469d8a7 Compare October 23, 2024 13:37

dylan-thinnes added 9 commits October 23, 2024 15:38

Change FileDiagnostic type synonym to a datatype

bd1404f

Make ideErrorWithSource produce FileDiagnostic by adding filepath arg

e684905

Supply structured error wherever we easily can - TODOs for hard parts

9d4cad1

We're leaving the TODOs for either later in this PR or in another PR

Fix UnitTests for new FileDiagnostic struct

e6a0b09

Remove explicit uses of FileDiagnostic, add codes to LSP diagnostics

d87c246

Add field for expected error codes in ghcide tests

cc2188c

Expect GHC-83865 for "type error" test - basic test

d2cbe55

Return structured warnings in TcModuleResult by copying from Driver

6e1eab7

Store FileDiagnostic instead of LSP Diagnostic in Shake store

dcf9397

fendor added the status: needs review This PR is ready for review label Nov 2, 2024

fendor requested review from soulomoon and fendor November 2, 2024 15:30

soulomoon reviewed Nov 4, 2024

View reviewed changes

ghcide/src/Development/IDE/Types/Diagnostics.hs Show resolved Hide resolved

plugins/hls-refactor-plugin/test/Main.hs Outdated Show resolved Hide resolved

soulomoon self-requested a review November 4, 2024 16:51

fendor added 5 commits November 21, 2024 16:18

Revert "Remove StructuredDiagnostic"

65300ef

This reverts commit 0776c65.

Remove the unused parameter from 'ideErrorText'

649e2ba

Add documentation to diagnostic helpers

4c9e7a3

Add action to query active diagnostics for a given Range

6eac56f

Implement 'rangesOverlap' function which checks whether two 'Range's overlap in any way. Implement two new plugin utility functions which allow to conveniently get all currently displayed diagnostics for a given 'Range'.

Use lens for updating Diagnostic

cbbf59f

fendor force-pushed the support-structured-diagnostics branch 2 times, most recently from 48e1288 to af550e3 Compare November 30, 2024 15:15

fendor added 4 commits November 30, 2024 16:16

Add GHC Structured Error compatibility module

4c335b2

Add compatibility module for GHC's structured error messages. Introduce 'Prism's and 'Lens's to easily access nested structures. Expand documentation for 'StructuredMessage'

Migrate hls-class-plugin to use StructuredMessage

1209674

Remove unused imports

75c6409

Make hls-class-plugin use the structured messages

f4d5c24

fendor force-pushed the support-structured-diagnostics branch from af550e3 to f4d5c24 Compare November 30, 2024 15:17

Merge branch 'master' into support-structured-diagnostics

af9cd22

fendor mentioned this pull request Dec 1, 2024

Update python read-the-docs dependencies to latest #4457

Merged

Fix compatibility with GHC 9.4 and rename function

6b1f66e

fendor approved these changes Dec 1, 2024

View reviewed changes

soulomoon reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support structured diagnostics 2 #4433

Support structured diagnostics 2 #4433

noughtmare commented Oct 17, 2024 •

edited

Loading

noughtmare commented Oct 17, 2024

noughtmare commented Oct 17, 2024

fendor commented Oct 18, 2024 •

edited

Loading

fendor commented Oct 18, 2024 •

edited

Loading

noughtmare commented Oct 18, 2024

fendor commented Oct 18, 2024

noughtmare commented Oct 18, 2024 •

edited

Loading

noughtmare commented Oct 22, 2024 •

edited

Loading

noughtmare commented Oct 23, 2024 •

edited

Loading

noughtmare commented Oct 23, 2024

noughtmare commented Oct 23, 2024 •

edited

Loading

noughtmare commented Oct 23, 2024 •

edited

Loading

noughtmare commented Nov 2, 2024

fendor commented Nov 2, 2024 •

edited

Loading

soulomoon left a comment •

edited

Loading

noughtmare commented Nov 19, 2024

fendor commented Nov 19, 2024 •

edited

Loading

noughtmare commented Nov 19, 2024

fendor commented Nov 19, 2024

fendor left a comment

fendor Dec 1, 2024

fendor Dec 1, 2024

fendor Dec 1, 2024

fendor Dec 1, 2024

fendor commented Dec 1, 2024

noughtmare commented Dec 1, 2024

soulomoon left a comment

soulomoon Dec 2, 2024 •

edited

Loading

fendor Dec 3, 2024 •

edited

Loading

soulomoon Dec 3, 2024

soulomoon Dec 2, 2024

fendor Dec 3, 2024

soulomoon Dec 3, 2024

Support structured diagnostics 2 #4433

Are you sure you want to change the base?

Support structured diagnostics 2 #4433

Conversation

noughtmare commented Oct 17, 2024 • edited Loading

noughtmare commented Oct 17, 2024

noughtmare commented Oct 17, 2024

fendor commented Oct 18, 2024 • edited Loading

fendor commented Oct 18, 2024 • edited Loading

noughtmare commented Oct 18, 2024

fendor commented Oct 18, 2024

noughtmare commented Oct 18, 2024 • edited Loading

noughtmare commented Oct 22, 2024 • edited Loading

noughtmare commented Oct 23, 2024 • edited Loading

noughtmare commented Oct 23, 2024

noughtmare commented Oct 23, 2024 • edited Loading

noughtmare commented Oct 23, 2024 • edited Loading

noughtmare commented Nov 2, 2024

fendor commented Nov 2, 2024 • edited Loading

soulomoon left a comment • edited Loading

Choose a reason for hiding this comment

noughtmare commented Nov 19, 2024

fendor commented Nov 19, 2024 • edited Loading

noughtmare commented Nov 19, 2024

fendor commented Nov 19, 2024

fendor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fendor commented Dec 1, 2024

noughtmare commented Dec 1, 2024

soulomoon left a comment

Choose a reason for hiding this comment

soulomoon Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

fendor Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noughtmare commented Oct 17, 2024 •

edited

Loading

fendor commented Oct 18, 2024 •

edited

Loading

fendor commented Oct 18, 2024 •

edited

Loading

noughtmare commented Oct 18, 2024 •

edited

Loading

noughtmare commented Oct 22, 2024 •

edited

Loading

noughtmare commented Oct 23, 2024 •

edited

Loading

noughtmare commented Oct 23, 2024 •

edited

Loading

noughtmare commented Oct 23, 2024 •

edited

Loading

fendor commented Nov 2, 2024 •

edited

Loading

soulomoon left a comment •

edited

Loading

fendor commented Nov 19, 2024 •

edited

Loading

soulomoon Dec 2, 2024 •

edited

Loading

fendor Dec 3, 2024 •

edited

Loading