-
Notifications
You must be signed in to change notification settings - Fork 202
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
[feature] Remove flymake-mode check from publishDiagnostics handler #596
Comments
I wonder if a good option here might be to explicitly support an |
We provide a hook that is called whenever diagnostics are received, and each hook function will be passed the diagnostics. This allows clean extension to support flycheck or other mechanisms for diagnostics display, while maintaining the existing flymake support intact. See joaotavora#596
Hi @purcell , I see you have a new pull request. I haven't looked at it, because I'm so very busy at the moment, sorry. But have you read the linked issue by @gagbo (doomemacs/doomemacs#4471)? We analysed the problem in depth there. As far as I remember, the problem hinges on this line:
If one removes The problem is that removing that symbol causes other bugs. But those bugs are Flymake's Eglot's and should really be taken care of. This is why I marked this issue "BUG". So I will look into your PR, but it feels like we're iterating something for which a simpler solution has already been found, a solution that doesn't require opening any new interfaces to the Eglot system. That solution just needs to be implemented and tested. It starts with removing the |
@joaotavora Thanks, I hadn't seen all of the discussion there, and don't have permissions to comment on it. The approach I've taken in #658 essentially leaves the flymake code as-is, including its handling of the unreported diagnostics and Part of the current issue is that we (Doom, and my hacky code, which I think eventually turned into the Doom code after I published it in a gist and it got passed around) end up abusing I don't know all the background of the flymake-related bugs here, and obviously there are reasons to want to keep eglot's external API minimal, but I'm not sure that the best route is to fiddle around to keep the current "sneakily hackable" code working: it seems fairly inevitable that it would break again. |
That's the thing, though. I don't actually think you're abusing it.
why do you suspect this? |
You could be right. I can certainly imagine that it should be possible to write a general flycheck checker which would use any flymake backend, but at the same time any one of those flymake backends would be perfectly entitled to assume it was being run by flymake and not flycheck.
Just because contributors using flymake shouldn't need to think about whether changes to the There are a few trade-offs here, so I'm just sharing some thoughts, and I'm more than happy to defer to your judgement! 😄 |
I'm experimenting with removing the
Sure. I haven't made up my mind yet. And certainly, I haven't fixed the bug, so it's good to have a PR that offers another, different way out. We may end up doing both and offering two ways to accomplish the same goal (though that is sometimes a footgun). So let's see what I can come up with half an hour of end-of-day semi-wasted brain power. |
(wasted but not 🍺 -wasted :-D, just tired) |
I should also try sketching out what the flycheck code would look like with my PR and paste it there, since that's probably quite relevant too. |
As you prefer. Pasting it in the other PR issue is also fine. |
Yeah, that was my plan. 👍 |
Boy am I tired, I really read "paste it here" :-/ |
Maybe just put your feet up for some well-deserved rest instead! |
Loosen coupling between eglot-flymake-backend and flymake-mode. The flymake-mode check in 'eglot-handle-notification publishDiagnostics' was a hack (and it wasn't even functioning correctly on M-x eglot-shutdown/eglot-reconnect). This should also allow eglot-flymake-backend to be driven by diagnostic-annotating frontends other than Flymake, such as the popular Flycheck package. * eglot.el (eglot--managed-mode): Use eglot--report-to-flymake. (eglot-handle-notification textDocument/publishDiagnostics): Use eglot--report-to-flymake.
Can you give this commit a quick spin? |
This commit seems to help, but the end result of the flycheck hackery seems erratic to me: flycheck apparently ends up in a "waiting for callback" state some of the time, and sometimes only a subset of the errors in a buffer show up. Here's what I think the overall issue is: there are two paths to calling So it seems to me that there's a fundamental mismatch between the callback lifecycles of flymake and flycheck here, which makes integrating via With my PR, that mismatch can be handled as follows: we translate and save the diagnostics into a "current errors" buffer-local var when they are received. Whenever flycheck decides to run a check, those errors are immediately returned. But of course at some unexpected time, the LSP server might returns some fresh diagnostics, so what we also need to do then is trigger a fresh check, effectively making LSP trigger the flycheck cycle: (defvar-local flycheck-eglot--current-errors nil)
(defun flycheck-eglot--on-diagnostics (diags)
(setq flycheck-eglot--current-errors
(mapcar (lambda (diag)
(with-current-buffer (flymake--diag-buffer diag)
(flycheck-error-new-at-pos
(flymake--diag-beg diag)
(pcase (flymake--diag-type diag)
('eglot-error 'error)
('eglot-warning 'warning)
('eglot-note 'info)
(_ (error "Unknown diag type, %S" diag)))
(flymake--diag-text diag)
:checker 'eglot
:end-pos (flymake--diag-end diag))))
diags))
;; The LSP sent us new diagnostics, so we trigger flycheck such that it
;; will pick them up
(run-at-time nil nil 'flycheck-buffer))
(defun flycheck-eglot--start (checker callback)
;; Ensure we'll receive diagnostics. This only needs doing once, but
;; it's a no-op if we do it multiple times, and here is a handy place to put it.
(add-hook 'eglot-diagnostics-hook 'flycheck-eglot--on-diagnostics nil t)
;; Don't actually start a new check, because we don't drive that process,
;; but instead immediately report the currently-known errors.
(funcall callback 'finished flycheck-eglot--current-errors))
(defun flycheck-eglot--available-p ()
(bound-and-true-p eglot--managed-mode))
(flycheck-define-generic-checker 'eglot
"Report `eglot' diagnostics using `flycheck'."
:start #'flycheck-eglot--start
:predicate #'flycheck-eglot--available-p
:modes '(prog-mode text-mode))
(add-to-list 'flycheck-checkers 'eglot) This works pretty nicely for me, as far as I can tell. |
I didn't know that @purcell, sorry for not finding the correct source. I initially took the snippet from a flycheck issue conversation flycheck/flycheck#1592 (enjoy the github back-reference spam from when I was rebasing the PR for Doom) but I didn't know if the initial version wasn't "original". The old version (without flymake-mode check)I always assumed that the LSP server sent all diagnostics on (lambda (flymake-diags &rest _)
(funcall callback
'finished
(mapcar #'flymake-diag->flycheck-err flymake-diags))) lambda should only be called once, and therefore flycheck diags should be up to date. At least I don't remember having complaints in Doom issue tracker with this version. I guess for servers that send incremental diagnostics that would be terrible, but I don't know the protocol and the ecosystem enough to know how often it happens. NowI'll try to find time to test f9388ff as well, and I'll reply when I have time. But for what it's worth, I also think that having a hook for diagnostics in the API is going to be more idiomatic and simpler to maintain. Simpler because with a hook we don't have to assume anything about how other checkers/packages work, just about keeping a consistent format for the diagnostics objects in the Cheers, PS: edited to change title levels so they’re less "in your face !" |
LOL, don't apologise, I just wouldn't want someone else to be blamed for the initial monstrosity. 😁
As far as I can tell, here a lambda is passed to |
Really? Interesting. In LSP, unlike the vast majority of syntax-checking scenarios, diagnostics don't come in just because you request them. They come in whenever they feel like it. Thus, Flymake, who normally contacts the backends after the buffer state has stabilized for a little while (like 0.5), has to be prepared to receive multiple responses from the callback. If Flycheck isn't doing this as well (are you 100% sure?) then I would say that's a flaw in Flycheck, or rather a fundamental limitation of Flycheck in the face of LSP. Maybe it works if it's "lucky". Indeed many LSP servers do send only one diagnostic notification per change, but the LSP server doesn't require them to. I'm pretty sure I'm seen LSP servers that send diagnostics immediately after launch, and some others that take their sweet time. I'm vaguely remember a LSP server sending them in multiple batches after startup. This ties together with your PR. I've seen your PR now and all it does is wrap the "middle" diagnostics calls. There are other diagnostic-reporting situations that the hook doesn't catch, and that's a flaw in that proposal that would have to be addressed. For example, Flycheck will not be notified of diagnostics that were already in the buffer before pressed But I'm not here to try to make life hard for any of you, so what do you say to this. In that commit, I created a unary helper function called In the long term, though, I think the fix should be having Flycheck relax/enhance its backend-communication skills. On maybe you could try to bring your favorite Flycheck features to Flymake? What are they? What are you missing? Would love to hear. |
It's perhaps more simple to maintain on your end, not mine :-) A hook is another interface (and @purcell 's hook does have some problems).
You don't have have to "assume" anything about how Flymake works. It is all be impeccably specified in the docstring of @purcell mentioned a problem regarding |
Yes:
I think that's indeed the case, and that it's a limitation of flycheck.
Yes and no. Such caching is possible, but I think there is also the inevitable issue of who is driving the process: flycheck wants to initiate a check itself and then receive updates, so if we know that there are updates (because the LSP callback told us so), then we'll need to tell flycheck to ask us about them. That's what my code snippet above does, but that principle might be applicable even with the current eglot code, ie. without my PR, unsure.
Yes, that could get sticky. How would you know whether previously-received diagnostics are still current in the batched case?
Correct, and I agree, I'm not convinced the PR is complete. My goal was to sketch out a more decoupled integration mechanism so that the flycheck quirks could be handled independently.
Yes, I think that could make sense and would be more or less equivalent. This is probably the best way to move forward now.
Yes, potentially, but I expect this would be a significant technical challenge.
Mainly all the display integration options: having errors either display on focus, or appear in a separate full-window tabulated list, plus the ability to show/merge errors from multiple chained backends. There are quite a few reasons it became much more popular than flymake, not least the large number of default and third-party checkers available. The major part for me is that I use flycheck for everything else, so using flymake for the subset of my work which involves LSP is a jarring inconsistency: when error display and navigation is different it can get confusing. Perhaps an alternate way to go is writing a package that gives flymake the ability to run flycheck checkers... I haven't really thought that through. 😄 |
Anything that hasn't been reporter yet for a given value of
Right, I think I understand. This caching idea is useless because you don't know when to "stop". Scratch that idea. So what happens if you try your backend adapter and one of these imperfect approaches?
This is probably not perfect, and you will miss some diagnostics in some edge cases, but I can't see it performing any worse than a solution built around
Are you aware of
showing erros from multiple backends is possible. Merging them is not. Where are merging rules specified?
I think the main reason is that Flymake was a different beast when Flycheck appeared. I rewrote it completely.
Yes, yes, very much so. Basically doing the reverse of what you did in that infamous gist. Write a function that takes in a Flycheck checker, whatever it is (you probably know what it looks like), and spits out a Flymake backend. Since Flymake backend-communication seems to be a superset of Flycheck's, it looks more viable. |
Yes, this one is my current strategy.
I think a call with
Yes, I've seen that, and on the face of it that's a similar facility. I can't remember what bugged me about it but I'll try to remember and provide more useful feedback.
Sorry, "merging" was a bad choice of word on my part. I meant simply showing all errors from all chained checkers in a single list buffer.
Yes, this is probably true, and I am grateful for your work!
I agree. There are a few forms this could take, including wrapping individual checkers, or allowing flycheck to run its entire checker chain. I haven't looked into the viability of either but it could be an interesting approach. |
Re: backend-adapter "invariant"I've always been in the camp of
Therefore a call with a Re: why not Flymake ?
Main reason for me is just the sheer quantity of checkers available. It probably stems from flycheck being the de facto checker for a while, but whenever I want to start using a linter, I know there's a very high probability that a flycheck checker is already written and packaged. Not really the case for Flymake. For example EDIT: another example is tide, the js package which is currently married to flycheck. It’s probably on them to help users use flymake over flycheck if they want; but meanwhile there are situations where you kind of have to use a flycheck checker, and "having all diags in a single list" requirement makes me lean to flycheck. Having a generic adapter to make the flycheck->flymake conversion would be good, but I guess it's a lot of work as well. |
I'm just using the lambda from before, calling this eglot-flymake-backend when emacs starts editing a managed buffer : (eglot-flymake-backend
(lambda (flymake-diags &rest _)
(funcall callback
'finished
(mapcar #'flymake-diag->flycheck-err flymake-diags)))) |
ah, correct. Then what happens when you try this? (eglot-flymake-backend
(lambda (flymake-diags &rest _)
(when flymake-diags
(funcall callback
'finished
(mapcar #'flymake-diag->flycheck-err flymake-diags))))) |
I'm expecting this to work, but also the diagnostics never to be cleared out after the user fixed the issue. That's definitely something to fix on my end though. How does flymake handles diagnostics that should be removed on a specific region ? |
Here's an important bit. make sure you don't accidentally have flymake and flycheck turned on at the same time! At least make sure to add |
After the user fixes the issue, the buffer has been modified, a new batch of diagnostics is requested that should replace the previous one.
I could tell you, but that doesn't matter immensely now, I think. What's important is how Flycheck handles this. What is the protocol for calling a callback with |
The problem indeed with the So maybe I would remove the |
To do this, simply inspect |
The callback implementation is an internal implementation detail for flycheck. As a checker writer, I have to call the callback with the new state ( There's a hook that deactivates flymake with this code (through |
I'm not asking you about the implementation, I'm asking you what does Flycheck promise to you, the checker-writer as you well put it, when you call the callback like that. Does it make fairies and pizza appear, or what? What is the protocol? |
I actually don't know, never dug too deep there, I'll have to soon enough I guess, but for now I can't really say. |
Would expect it to be documented in https://www.flycheck.org/en/latest/developer/developing.html, but really isn't. Anyway, I think there's at least one brute-force (but not that horrible either) way to fix this (defvar some-timer nil)
(let ((callback the-magical-flycheck-callback-youre-using))
(when some-timer (cancel-timer some-timer))
;; if after two seconds the LSP hasn't done its job, assume there are no diagnostics
(setq some-timer (run-with-timer 2 nil callback 'finished (list))
(eglot-flymake-backend
(lambda (flymake-diags &rest _)
(when flymake-diags
(cancel-timer some-timer)
(funcall callback
'finished
(mapcar #'flymake-diag->flycheck-err flymake-diags)))))) |
with the Without there's probably a nil message that gets sent and everything is wiped: I'll try looking into flycheck internals to see what can be done, and maybe digging the issue tracker |
Diagnostics accumulate? Strange. You really have to understand what |
I'm using |
I digged a little more, in flycheck, flymake, eglot and lsp-mode code, and it seems that it's a lot harder to have a proper flycheck integration. As
I basically still had to use an eglot internal to trigger a flycheck-checker on time, I'm pasting the summary of my approach (pushed in a doom PR doomemacs/doomemacs#4837) below :
I'm pretty sure it's not the correct approach at all, relying on a lot of duplicated effects/diagnostics and internals of everything, but I really just wanted to get this working and be done with it. If other people want to use this as a base to make something that works better, they can, but I don't think I'll give more of my time to that shim :) |
OK, I get it that you're tired of this. But all this work to avoid using the internal implementation detail and in the end you're still using it? 🤦 |
Yeah, I guess I couldn't really find a way to make it work in the end. Maybe someone else will have a go at it and actually make a proper flycheck-only solution. At least it's ready now, but I don't want to dig into flycheck checkers to handle the state when checker can be called with lists args, with no way of semantically separate "wipe diagnostics" from "nothing new" cases |
@gagbo With the latest eglot code, this seems to work for me: https://gist.github.com/purcell/78c870fb88ff12906ccc3d07b0389bd8 Maybe give something like that a try locally and see if you observe any issues? |
Well but pinning an Eglot version is a big drawback right? Anyway, do put the commit here I can have a look later on. |
This works but I really don't get why, my brain is burnt. I don't see how what I tried before didn't work but this does. I'll add attribution and adapt this, if it's okay.
Pinning isn't really a drawback, everything is pinned by default, and bumps are made periodically depending on the needed fixes. |
Yes, of course. |
I remembered! It doesn't update automatically. A pretty trivial complaint, I know, but I like to keep the errors window open next to the code for certain types of work, and the flycheck version updates continuously as new checks take place. |
Also, this is starting to come together: https://github.com/purcell/flymake-flycheck/blob/main/flymake-flycheck.el 😁 |
Looks very good. I'll have a look later. Then I'd suggest requesting
comments on emacs-devel and adding to non-gnu elpa or something.
I'll also look at your flymake bug report, I think that buffer should also
update automatically, but I'm away from my computer right now.
João
…On Sun, Apr 4, 2021, 02:18 Steve Purcell ***@***.***> wrote:
Also, this is starting to come together:
https://github.com/purcell/flymake-flycheck/blob/main/flymake-flycheck.el
😁
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#596 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC6PQYN7XDNRVSZRVFCNLDTG6V5NANCNFSM4WHXYAZA>
.
|
Loosen coupling between eglot-flymake-backend and flymake-mode. The flymake-mode check in 'eglot-handle-notification publishDiagnostics' was a hack (and it wasn't even functioning correctly on M-x eglot-shutdown/eglot-reconnect). This should also allow eglot-flymake-backend to be driven by diagnostic-annotating frontends other than Flymake, such as the popular Flycheck package. * eglot.el (eglot--managed-mode): Use eglot--report-to-flymake. (eglot-handle-notification textDocument/publishDiagnostics): Use eglot--report-to-flymake.
Loosen coupling between eglot-flymake-backend and flymake-mode. The flymake-mode check in 'eglot-handle-notification publishDiagnostics' was a hack (and it wasn't even functioning correctly on M-x eglot-shutdown/eglot-reconnect). This should also allow eglot-flymake-backend to be driven by diagnostic-annotating frontends other than Flymake, such as the popular Flycheck package. * eglot.el (eglot--managed-mode): Use eglot--report-to-flymake. (eglot-handle-notification textDocument/publishDiagnostics): Use eglot--report-to-flymake.
Loosen coupling between eglot-flymake-backend and flymake-mode. The flymake-mode check in 'eglot-handle-notification publishDiagnostics' was a hack (and it wasn't even functioning correctly on M-x eglot-shutdown/eglot-reconnect). This should also allow eglot-flymake-backend to be driven by diagnostic-annotating frontends other than Flymake, such as the popular Flycheck package. * eglot.el (eglot--managed-mode): Use eglot--report-to-flymake. (eglot-handle-notification textDocument/publishDiagnostics): Use eglot--report-to-flymake. #596: joaotavora/eglot#596
Loosen coupling between eglot-flymake-backend and flymake-mode. The flymake-mode check in 'eglot-handle-notification publishDiagnostics' was a hack (and it wasn't even functioning correctly on M-x eglot-shutdown/eglot-reconnect). This should also allow eglot-flymake-backend to be driven by diagnostic-annotating frontends other than Flymake, such as the popular Flycheck package. * eglot.el (eglot--managed-mode): Use eglot--report-to-flymake. (eglot-handle-notification textDocument/publishDiagnostics): Use eglot--report-to-flymake. GitHub-reference: fix joaotavora/eglot#596
Since da7ff48, it is a lot harder to use flycheck or any other checker to handle diagnostics when the server sends publishDiagnostics message.
Before that, it was possible, as a client, to only call
eglot-flymake-backend
once on the buffer to properly set the internaleglot--current-flymake-report-fn
and get updates when new diagnostics are received.It would be amazing to be able to do that again :)
Also discussed (in length) in doomemacs/doomemacs#4471
The text was updated successfully, but these errors were encountered: