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

fendor · 2024-10-24T12:24:58Z

I think the linked code is wrong, the benchmark isn't executed on each PR, see https://github.com/haskell/haskell-language-server/actions/runs/11481138334/job/31951108378?pr=4433, but only nightly or on demand. So good catch :)

soulomoon · 2024-10-25T07:44:11Z

We need performance label to run the benchmark, see #4271.
Now it runs, the result seems good for the benchmark

soulomoon

Good work, just a few neatpicks and a question

hls-test-utils/src/Development/IDE/Test.hs

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

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

noughtmare · 2024-10-26T10:28:49Z

Interesting, it seems the stack build converts more warnings to errors than the cabal build.

wz1000 · 2024-10-30T10:54:51Z

I'm a bit confused, I see we now store the structured diagnostics for warnings from the typechecker in tmrWarnings, but where do other structured diagnostics from other places and typechecker errors go? It seems like we convert them to LSP diagnostics and don't have the structured value available?

noughtmare · 2024-10-30T14:17:45Z

@wz1000 I think I removed that in 0776c65 (in response to #4433 (comment)), because we don't use it anywhere else yet and it would be very simple to add back when we do actually need it.

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.

soulomoon · 2024-11-03T15:50:09Z

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

+      fdLspDiagnostic =
+        lspDiag
+          & attachReason (fmap (diagnosticReason . errMsgDiagnostic) mbOrigMsg)
+          & setGhcCode mbOrigMsg


So all the MsgEnvelope are converted to FileDiagnostic through this function?

I think so, yes.

I was actually looking at when this function is called with a Just and couldn't easily find anything, because there are so many callers that pass along a value for which you then have to transitively search.

OK, I found these now:

https://github.com/haskell/haskell-language-server/pull/4433/files/0776c65490d8840d03ec8740f0c8048ea1d55b35#diff-9ec5d8d4031c00a406a3c0ce65ca9e155f3a05d7014a97469f89b5fd3bb65bb9R71

https://github.com/haskell/haskell-language-server/pull/4433/files/0776c65490d8840d03ec8740f0c8048ea1d55b35#diff-d7140bbfca1b7aaafd8c82d1d6aa21a3f941eb27ba8f08e4d9fb13766b21b418R885

And those are the only two places. I've now checked every caller.

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 :)

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 requested review from jhrcek and soulomoon October 24, 2024 13:05

noughtmare added 3 commits October 24, 2024 19:16

Resolve fendor's feedback

16551d1

Apply stylish-haskell formatting

5a6216b

Apply more stylish-haskell formatting

4465f49

noughtmare mentioned this pull request Oct 24, 2024

Release 2.10.0.0 #4434

Open

soulomoon added the performance Issues about memory consumption, responsiveness, etc. label Oct 25, 2024

Merge branch 'master' into support-structured-diagnostics

79341ed

soulomoon reviewed Oct 25, 2024

View reviewed changes

hls-test-utils/src/Development/IDE/Test.hs Outdated Show resolved Hide resolved

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

soulomoon reviewed Oct 25, 2024

View reviewed changes

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

noughtmare added 2 commits October 25, 2024 23:10

Resolve some of soulomoon's feedback

82367ec

Fix small issues

ada484e

noughtmare added 2 commits October 26, 2024 12:30

Remove unused imports

6c05e23

Remove StructuredDiagnostic

0776c65

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

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

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

fendor commented Oct 24, 2024

soulomoon commented Oct 25, 2024 •

edited

Loading

soulomoon left a comment •

edited

Loading

noughtmare commented Oct 26, 2024 •

edited

Loading

wz1000 commented Oct 30, 2024

noughtmare commented Oct 30, 2024 •

edited

Loading

noughtmare commented Nov 2, 2024

fendor commented Nov 2, 2024 •

edited

Loading

soulomoon left a comment •

edited

Loading

soulomoon Nov 3, 2024

noughtmare Nov 7, 2024

noughtmare Nov 7, 2024

noughtmare Nov 7, 2024 •

edited

Loading

noughtmare commented Nov 19, 2024

fendor commented Nov 19, 2024 •

edited

Loading

noughtmare commented Nov 19, 2024

fendor commented Nov 19, 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

fendor commented Oct 24, 2024

soulomoon commented Oct 25, 2024 • edited Loading

soulomoon left a comment • edited Loading

Choose a reason for hiding this comment

noughtmare commented Oct 26, 2024 • edited Loading

wz1000 commented Oct 30, 2024

noughtmare commented Oct 30, 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

soulomoon Nov 3, 2024

Choose a reason for hiding this comment

noughtmare Nov 7, 2024

Choose a reason for hiding this comment

noughtmare Nov 7, 2024

Choose a reason for hiding this comment

noughtmare Nov 7, 2024 • 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

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

soulomoon commented Oct 25, 2024 •

edited

Loading

soulomoon left a comment •

edited

Loading

noughtmare commented Oct 26, 2024 •

edited

Loading

noughtmare commented Oct 30, 2024 •

edited

Loading

fendor commented Nov 2, 2024 •

edited

Loading

soulomoon left a comment •

edited

Loading

noughtmare Nov 7, 2024 •

edited

Loading

fendor commented Nov 19, 2024 •

edited

Loading