Flycheck is stuck with custom generic checker

[lockie]

Hey.
I'm trying to write custom generic flycheck checker that works with eglot language server package, as it itself is tied to flymake and its author won't do it himself.
I've manually unhardcoded flymake calls in eglot in my local elpa directory (for now) and then ended up with the following code in my init.el:

(use-package flycheck
  :ensure t
  :init (global-flycheck-mode))

(use-package eglot
  :ensure t
  :config
  (defun my/start-eglot-flycheck (checker callback)
    (defun my/point->line-col (buffer beg)
      (with-current-buffer buffer
        (save-excursion
          (goto-char beg)
          (cons (line-number-at-pos)
                (- (point)
                   (line-beginning-position))))))
    (defun my/flymake-diagnostic->flycheck-error (diag)
      (pcase-let* ((msg (flymake-diagnostic-text diag))
                   (beg (flymake-diagnostic-beg diag))
                   (buffer (flymake-diagnostic-buffer diag))
                   (`(,line . ,col) (my/point->line-col buffer beg)))
        (flycheck-error-new-at
         line col
         (pcase (flymake-diagnostic-type diag)
           ('eglot-error 'error)
           ('eglot-warning 'warning)
           ('eglot-note 'info))
         msg
         :checker checker)))
    (eglot-flymake-backend
     (lambda (report-action &key region)
         (funcall
          callback 'finished
          (mapcar
           #'my/flymake-diagnostic->flycheck-error
           report-action)))))
  (flycheck-define-generic-checker 'eglot
    "A generic eglot flycheck checker"
    :start #'my/start-eglot-flycheck
    :modes '(python-mode))
  (add-to-list 'flycheck-checkers 'eglot)
  (add-to-list 'flycheck-disabled-checkers 'python-flake8)
  (add-to-list 'flycheck-disabled-checkers 'python-pylint)
  (add-to-list 'flycheck-disabled-checkers 'python-pycompile)
  :hook
  (python-mode . eglot-ensure))

It seems to work all right, I can see error messages from language server in the buffer and error counter on the modeline, but there's one problem. I figured it happens when I type anything in buffer while flycheck (seemingly) is running my custom checker. The problem is that in that situation flycheck somehow gets stuck, error count in the modeline is displayed as "???" (I'm using spaceline to display flycheck error count at the modeline), and M-x flycheck-list-errors displays empty error list even though there are errors displayed in the buffer.
I'm not that much of an elisp hacker, so I suppose there's something wrong in my code. Would you please point me at what could be wrong? Does callback passed to :start function require some sort of synchronization or locking?

Here's additional info:

• M-x flycheck-verify-setup

Syntax checkers for buffer models.py in python-mode:

First checker to run:

  eglot
    - may enable: yes

Checkers that are compatible with this mode, but will not run until properly configured:

  python-flake8 (disabled)
    - may enable:         Automatically disabled!
    - executable:         Found at /home/lock/.pyenv/shims/python
    - configuration file: Not found
    - `flake8' module:    Found at "/usr/lib64/python3.6/site-packages/flake8/__init__.py"

  python-pylint (disabled)
    - may enable:         Automatically disabled!
    - executable:         Found at /home/lock/.pyenv/shims/python
    - configuration file: Not found
    - `pylint' module:    Found at "/usr/lib64/python3.6/site-packages/pylint/__init__.py"

  python-pycompile (disabled)
    - may enable: Automatically disabled!
    - executable: Found at /home/lock/.pyenv/shims/python

  python-mypy (disabled)
    - may enable:         Automatically disabled!
    - predicate:          t
    - executable:         Not found
    - configuration file: Not found
    - next checkers:      python-flake8

Flycheck Mode is enabled.  Use M-x universal-argument C-c ! x to
enable disabled checkers.

--------------------

Flycheck version: 32snapshot (package: 20190619.1410)
Emacs version:    26.1
System:           x86_64-pc-linux-gnu
Window system:    x

• My OS: Gentoo stable 64-bit
• M-x emacs-version: GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu) of 2018-09-29
• M-x flycheck-version: Flycheck version: 32snapshot (package: 20190619.1410)

Thanks in advance.

[lockie]

NVM, I've migrated to flymake.

[fmdkdd]

Too bad you've closed this, since I would actually like to have a lightweight generic checker that works with LSP, and eglot is a good candidate.

Regarding your issue, I think flycheck is getting stuck in the ??? phase as the callback is not called by eglot-flymake-backend. Flycheck will trigger the :start property of your eglot checker as soon as you type (depending on flycheck-check-syntax-automatically), but I'm guessing eglot-flymake-backend will not necessarily report in time, leaving the callback to hang?

For LSP, I think it's better to just let the LSP backend do its stuff, and then for the Flycheck checker to report the errors as soon as they get back to Emacs from the LSP server. This is what I did for lsp-ui:

https://github.com/emacs-lsp/lsp-ui/blob/3ccc3e3386732c3ee22c151e6b5215a0e4c99173/lsp-ui-flycheck.el#L175-L200

In lsp-ui, errors are gathered in lsp-diagnostics. As soon as the Flycheck checker is called, it simply converts these diagnostics into flycheck-error objects, and calls the callback directly. You can then instruct the lsp package to trigger Flycheck as soon as errors arrive, and disable the usual trigger mechanism (on save, on type, etc.):

https://github.com/emacs-lsp/lsp-ui/blob/3ccc3e3386732c3ee22c151e6b5215a0e4c99173/lsp-ui-flycheck.el#L223-L230

[lockie]

but I'm guessing eglot-flymake-backend will not necessarily report in time, leaving the callback to hang?

Yes, that's precisely what it does: eglot-flymake-backend can report a bunch of yet unreported diagnostics right at the moment it is called (see here), but otherwise it will call its callback argument some time later (or, if there are no diagnostics, won't call it at all).

This is what I did for lsp-ui:

So, just to clarify: in Flycheck, you can't save the callback posted to :start function and call it sometime later after that function exited?

Too bad you've closed this, since I would actually like to have a lightweight generic checker that works with LSP, and eglot is a good candidate.

Anyhow, besides writing the proper Flycheck checker, that would require architectural changes in eglot itself - it is way too tightly tied with flymake.

[fmdkdd]

in Flycheck, you can't save the callback posted to :start function and call it sometime later after that function exited?

Oh you can. Asynchronous checkers (i.e., all command checkers) actually rely on this. But I'm not sure what happens when you make several calls to :start without calling the callbacks of the previous ones, which is maybe what happens here. Or maybe the problem is elsewhere.

that would require architectural changes in eglot itself - it is way too tightly tied with flymake.

Good to know. Thanks for looking into this!

[lockie]

But I'm not sure what happens when you make several calls to :start without calling the callbacks of the previous ones, which is maybe what happens here.

Yeah, I think this is what most probably happens there.

Thanks for looking into this!

You're welcome!
The changes I've mentioned are not that dramatic, but they still need to be done or at least accepted by the eglot author.

[marsam]

FWIW I have something similar to your snippet:

(defun flycheck-eglot-start (checker callback)
  "Run a check with CHECKER and pass the status onto CALLBACK."
  (cl-labels
      ((flymake-diag->flycheck-err
        (diag)
        (pcase-let ((`(,line . ,column)
                     (with-current-buffer (flymake-diagnostic-buffer diag)
                       (save-excursion
                         (goto-char (flymake-diagnostic-beg diag))
                         (cons (line-number-at-pos)
                               (- (point)
                                  (line-beginning-position)))))))
          (flycheck-error-new-at
           line column
           (pcase (flymake-diagnostic-type diag)
             (`eglot-note    'info)
             (`eglot-warning 'warning)
             (`eglot-error   'error))
           (flymake-diagnostic-text diag)
           :checker checker))))
    (eglot-flymake-backend (lambda (report-fn &rest _)
                             (funcall callback 'finished (mapcar #'flymake-diag->flycheck-err report-fn))))))

(flycheck-define-generic-checker 'eglot
  "Syntax checker using eglot.

See: `https://github.com/joaotavora/eglot'."
  :start #'flycheck-eglot-start
  :predicate (bound-and-true-p eglot--managed-mode)
  :modes (flatten-tree (mapcar 'car eglot-server-programs)))

and disable flymake from eglot

(add-hook 'eglot--managed-mode-hook (lambda () (flymake-mode -1)))

flatten-tree is from Emacs27

[fmdkdd]

Thanks @marsam! Do you think we could integrate that into Flycheck already? Or is it too hackish (specifically, hooking into the private eglot--managed-mode-hook) and we should coordinate with the eglot folks instead, as @lockie suggested?

[eyal0]

The code above did not work for me but I made a different one based on something that I found on github: https://gist.github.com/eyal0/8f58d4209dc8cdb5d9c474b61c49b2dc

I needed to modify flycheck to accept a beginning position and end position. Even if this hack isn't clean enough to be added to flycheck, I think that it would still be nice to have flycheck support another mode other than line, column, symbol, sexp, so that checkers can provide both the start and end position.

[fmdkdd]

@eyal0 Well, rejoice, because #1676 is upon us.

[eyal0]

Cool! @fmdkdd one question: Does flycheck convert those line-and-column numbers into markers in the buffer? I think that it does because when I modify the buffer, the errors seem to follow the text around correctly.

It's important to follow the text around.

It's kind of silly that the specification is line and column number instead of just character position because the checker that I'm using (eglot) is already providing character position. So I have to convert it to line number and column number and then flycheck is presumably converts it back in order to put a marker in the buffer. And in the converting to and from line number and column number, I have to know about tabs versus spaces and also whether the first line is 0 or 1 and the first column is 0 or 1.

The interface ought to just be start-pos and end-pos. That would be great. I can understand the pain in having to support two different interfaces and the line-column interface already exists and converting everything would be too difficult by far! So at least there ought to be reliable functions to convert to and from.

[cpitclaudel]

The interface ought to just be start-pos and end-pos. That would be great.

I think you'll find the (long) discussion in the linked thread informative :) We implemented that then dropped it because of backwards compatibility concerns. we want to get there eventually.

So at least there ought to be reliable functions to convert to and from.

Yes, something like this?

(defun flycheck-position-to-line-column (point)
  "Return a (LINE . COLUMN) cons corresponding to POINT."
  (let ((inhibit-field-text-motion t))
    (save-excursion
      (goto-char point)
      (cons (line-number-at-pos) (- (point) (line-beginning-position))))))

[eyal0]

Yes, something like this?

(defun flycheck-position-to-line-column (point)
  "Return a (LINE . COLUMN) cons corresponding to POINT."
  (let ((inhibit-field-text-motion t))
    (save-excursion
      (goto-char point)
      (cons (line-number-at-pos) (- (point) (line-beginning-position))))))

Yes please. In the source code.

[eyal0]

@cpitclaudel Also, did you write it correctly or do you have any off-by-one errors in there? Compare with this:

https://github.com/flycheck/flycheck/pull/1676/files#diff-374cdd9563b826c712b1dc6a373a0bc1R1505

[cpitclaudel]

Also, did you write it correctly or do you have any off-by-one errors in there?

I didn't try it :)

[eyal0]

Okay. After #1676 is merged, I will add the function if no one else has done it, along with some tests. I think that the correct function is this:

(defun flycheck-position-to-line-column (point)
  "Return a (LINE . COLUMN) cons corresponding to POINT."
  (let ((inhibit-field-text-motion t))
    (save-excursion
      (goto-char point)
      (cons (line-number-at-pos) (1+ (- (point) (line-beginning-position)))))))

Need to add one. This should be tested in a file that has tabs, also, because the column is listed as 8 after a tab even though the number of characters is just 1.

[gagbo]

Hello, I'll try to use your code to include eglot resutls in flycheck, but I wonder if you eventually had to/added the code above in your gist ? Should I modify it in any way or is it usable as is ?

(cc doomemacs/doomemacs#3020 )

[cpitclaudel]

@gagbo Happy to help if needed

[gagbo]

Looks like just adding the :end-line and :end-column properties in the constructor of flycheck-error struct made it https://github.com/hlissner/doom-emacs/blob/90d9b9e4ee6a8bbb513290f54d1412fc1d2c4cb0/modules/tools/lsp/flycheck-eglot.el That flycheck-position-to-line-column function was pretty useful, but I guess just for the column part

[cpitclaudel]

That code looks suspicious: why do you have a redefinition of the flycheck-error struct?

[eyal0]

The original version off which I based my gist didn't have a good definition of flycheck-error. I needed the begin pos start and the begin pos end so that I could store markers instead of just integers.

If you look into the history, you'll that the struct was recently changed. That change was done after I had already completed my work.

Storing the start and end character positions is a lot better than storing pairs of line and column number for a few reasons:

1. A character position can be stored as a marker. A marker is like a magic byte position that follows the text around even if the buffer is modified.
2. Line numbers and column numbers have confusion where there is disagreement if the first value is 0 or 1. emacs works with the first line being one and the first column being zero but flycheck doesn't do this.
3. Storing a single value is easier than storing a pair of values, especially if you want to store both the start and the end.

I think that flycheck, with the latest changes, is moving toward the direction of using bite positions and markers.

[cpitclaudel]

If you look into the history, you'll that the struct was recently changed. That change was done after I had already completed my work.

If you mean Flycheck's history, then no need to look into it: I wrote the code :)

Storing the start and end character positions is a lot better than storing pairs of line and column

I agree with you, but not for the reasons below ^^

A character position can be stored as a marker. A marker is like a magic byte position that follows the text around even if the buffer is modified.

Yup, that's what Flycheck's error overlays do (overlays are pairs of markers)

Line numbers and column numbers have confusion where there is disagreement if the first value is 0 or 1. emacs works with the first line being one and the first column being zero but flycheck doesn't do this.

Can you clarify what confusion you mean? As long as the convention is clear, I don't think that's a significant issue, is it?

Storing a single value is easier than storing a pair of values, especially if you want to store both the start and the end.

I do agree with that one ^^ But you still need the line and column numbers, because they are used in the error list.

I think that flycheck, with the latest changes, is moving toward the direction of using bite positions and markers.

We might converge on this, but I wouldn't dismiss the current design too fast; it took us a long time to come up with a version that was sufficiently backwards compatible. And one problem with markers is that they are tied to a buffer, while LSP servers will typically get batches of errors for a set of files and buffers — you want to be able to create flycheck-error objects even if there's no buffer for that file.

In any case, the problem with redefining the structure is that it will break Flycheck and all other packages that depend on it, so please don't do that ^^ I'd much rather brainstorm a solution together; at the moment, I'm not sure I understand the issue clearly enough to be able to help.

(As an example of suggestion: you're creating a new set of markers, but we already have those, through the overlays that we create — it would be much easier to store a buffer-local map from error objects to their overlays)

[cpitclaudel]

Btw @marsam, any chance to publish your code as an extension?

[gagbo]

Btw @marsam, any chance to publish your code as an extension?

With this code there are still a few tweaks to add regarding overlays it seems :

Debugger entered--Lisp error: (wrong-type-argument overlayp nil)
  overlay-start(nil)
  flymake-diagnostic-beg(#s(flymake--diag :buffer #<buffer FileName.cpp> :beg 4815 :end 4821 :type eglot-error :text "ccls: 'this' argument to member function 'decode' ..." :backend nil :data ((eglot-lsp-diag :range (:start (:line 118 :character 33) :end (:line 118 :character 39)) :severity 1 :code 2 :source "ccls" :message "'this' argument to member function 'decode' has ty..." :relatedInformation [])) :overlay-properties nil :overlay nil))
  (goto-char (flymake-diagnostic-beg diag))
  (save-excursion (goto-char (flymake-diagnostic-beg diag)) (cons (line-number-at-pos) (- (point) (line-beginning-position))))
  (save-current-buffer (set-buffer (flymake-diagnostic-buffer diag)) (save-excursion (goto-char (flymake-diagnostic-beg diag)) (cons (line-number-at-pos) (- (point) (line-beginning-position)))))

But at least it's a lot simpler, I'll try to start again from this

[gagbo]

The overlay backtrace is due to using flymake-diagnostic-beg diag instead of accessing the bare slot with flymake--diag-beg diag. But now my eglot errors seem to get swallowed somewhere. I'll continue investigating

[cpitclaudel]

Thanks for your work.

[gagbo]

Should be a lot cleaner now

My issue of "eglot errors swallowed somewhere" was that I didn't forcefully trigger a check in the buffer at the end or my flycheck-checker :start function. (flycheck-buffer) is a little hard to find (I was just fuzzy searching for trigger and force and now in function names to find it :) )

[cpitclaudel]

Looking nice, indeed 👏! One quick tip: you should be able to use flycheck-error-new-at-pos instead of flycheck-error-new. Call it with (flymake--diag-buffer diag) current and give it (flymake--diag-beg diag) as its first argument, and :end-pos (flymake--diag-end diag) and I think you won't need to do the pos→line/column conversion.

[gagbo]

Indeed. Eventually @marsam snippet can be condensed into :

(defun flycheck-eglot--start (checker callback)
  "Clean up errors when done.
CHECKER is the checker (eglot).
CALLBACK is the function that we need to call when we are done, on all the errors."
  (cl-labels
      ((flymake-diag->flycheck-err
        (diag)
        (with-current-buffer (flymake--diag-buffer diag)
          (flycheck-error-new-at-pos
           (flymake--diag-beg diag)
           (pcase (flymake--diag-type diag)
             ('eglot-note 'info)
             ('eglot-warning 'warning)
             ('eglot-error 'error)
             (_ (error "Unknown diagnostic type, %S" diag)))
           (flymake--diag-text diag)
           :end-pos (flymake--diag-end diag)
           :checker checker
           :buffer (current-buffer)
           :filename (buffer-file-name)))))
    ;; NOTE: Setting up eglot to automatically create flycheck errors for the buffer.
    (eglot-flymake-backend (lambda (flymake-diags &rest _)
                             (funcall callback 'finished (mapcar #'flymake-diag->flycheck-err flymake-diags))))
    ;; NOTE: Forcefully trigger a check in the buffer (function name is confusing)
    (flycheck-buffer)))

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

(Not using emacs-27 specific code for compatibility here). I left the :buffer and :filename arguments even if they match the default values because I prefer "freezing" the default values downstream in client code, but technically those lines can be removed

[marsam]

Hi, sorry for the late response

any chance to publish your code as an extension?

Sure, I'm going to create an extension this weekend

[mohkale]

I've tried using @gagbo's fix for this but flycheck just seems to hang forever when checking for errors. Emacs doesn't freeze or anything, it's just my mode-line always shows ? indicating it's still loading errors and the flycheck-list-errors buffer appears to be empty.

[gagbo]

There really should be a way to disable commit mentions. If there's one each time I rebase a feature branch it's going to get messy quickly. I don't know what you exactly tested, but isn't the eglot checker merged in flycheck now ?

If it isn't, the code used in Doom is there, the only "trick" is add-hook!, which is just like add-hook but allows to use defun syntax in the call.

[mohkale]

@gagbo

I'm not seeing a pull-request for eglot support, which PR was supposed to have merged this?

I copied and pasted what you shared exactly (I think). I've got a eglot flychecker function and running flycheck-verify-checker shows it is running in my buffer, but I still have the issues described above. I also disabled flymake-mode as mentioned above (maybe that's causing it, I'll double check now).

S.N. you can hit unsubscribe above to the right and you'll stop recieving upate notiifications unless someone explicitly @ adds you.

S.S.N. I @ added you, I'm sorry for the annoyance.

[mohkale]

Here's an example of the same file, the one on the top is with flymake the bottom is with flycheck and the shared patch. There's no syntax highlighting and the mode-line indicates no errors have been found yet.

[gagbo]

Remove the last call to (flycheck-buffer). I don't know why it used to work and stopped since, but it seems that it solves the issue. (You will have to trigger a publishDiagnostic initially though)

Calling manually flycheck-buffer makes it go back to an infinite pending state though, so there is something else to do to have a cleaner fix

[gagbo]

The root cause for this issue (eglot checker in flycheck stays in idle state) is that flymake-mode check is hardcoded here https://github.com/joaotavora/eglot/blob/4c85df2b04e467b8ed0eca68bd202fd0e7b671f4/eglot.el#L1623

The change happened 8 months ago joaotavora/eglot@da7ff48

So the code in flycheck-eglot doesn't run on publishDiagnostics basically. The fix is to set a variable watch on eglot--unreported-diagnostics to call eglot-flymake-backend at that point. I'll push a patch to Doom when I find out how to do this.