haskell
Support structured diagnostics 2
This is my attempt at finishing the work of @dylan-thinnes on !4311.
Addresses https://github.com/haskell/haskell-language-server/issues/2014
Should hopefully enable https://github.com/haskell/haskell-language-server/issues/3246
All failures in the jobs of e651d41 seem to be spurious (except for the stylish-haskell formatting failure).
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.
Are these kinds of spurious failures common in HLS?
Yes, they unfortunately are
Some errors in ghc 9.4 seem to be genuine, though. Many tests are seemingly time outing for some reason.
You're right. Can anyone give me some tips on how to profile HLS to see where these timeouts are coming from?
@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)
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.
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.
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.
@fendor do you think we could try setting the -ld_classic flag? If so, where should I put it?
Oh, I see we already have that exact workaround in some places. I'll just copy that to the hlint plugin.
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 usually squash the PR when merging, so I don't think it is particularly necessary to squash it yourself, unless you want to add a custom message.
Awesome that the CI of the PR is green! Let's try to get some reviews in.
Great! If you just want to see the changes I made to Dylan's work, you can use this link https://github.com/haskell/haskell-language-server/pull/4433/files/6ca2a196caa89f2de3452f0afc5a41299a94e092..1f835dabecec0ed6cd2e9236fc766a0e01620e41
Thanks! However, you may be slightly unhappy to hear that I haven't reviewed the previous PR at all yet, so I need to look at everything.
I noticed https://github.com/haskell/haskell-language-server/pull/4433/commits/14d66975decfa01187aa1260ea5e3e8a17ef6d1b is still in this PR. In the discussion https://github.com/haskell/haskell-language-server/pull/4311#discussion_r1660206034 it was basically decided to not include this in this PR. Could you take this out?
One thing I don't understand is what the final codes actually are. When we construct them it seems to include the GHC- prefix:
https://github.com/haskell/haskell-language-server/pull/4433/files#diff-15ac916db5d1445c16a0cb4a644ecc44854ce21d0f648b07663212035766b1feR101-R121
But when we use requireDiagnostic we do not include the prefix:
https://github.com/haskell/haskell-language-server/pull/4433/files#diff-52be86ee0fb96c538fa0bf2d9872b8aa41a8dd1b0f434383ce6f9301d4c39572R269
Why does this pass the tests? ~~Edit: it seems we only use requireDiagnostic in the ghcide-bench executable, so perhaps it is just never tested (it seems we skip "benchmarks" in CI).~~ That can't be because this piece of code already caused CI failures on GHC 9.4.
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 :)
We need performance label to run the benchmark, see #4271. Now it runs, the result seems good for the benchmark
Interesting, it seems the stack build converts more warnings to errors than the cabal build.
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?
@wz1000 I think I removed that in 0776c65 (in response to https://github.com/haskell/haskell-language-server/pull/4433#discussion_r1816962330), because we don't use it anywhere else yet and it would be very simple to add back when we do actually need it.
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?
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 ).
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?
I'm fine with pushing directly to my branch if that is possible. Do I need to give you special permissions?
I don't know, I will just try to push at some point and ping you if I have any trouble :)
I personally just wanted access to the diagnostic codes. So I'm more than satisfied!
Since the release is stalled due to CI issues, I am inclined to just push the trigger and be done with it. Opinions, @soulomoon, @michaelpj, @wz1000 ?