bug#66759: closed (30.0.50; Flymake (with Eglot) error cleaning up old overlay)

[GNU bug Tracking System]

Your message dated Thu, 26 Oct 2023 18:10:46 +0100
with message-id <87il6t1f8p.fsf@gmail.com>
and subject line Re: 30.0.50; Flymake (with Eglot) error cleaning up old overlay
has caused the debbugs.gnu.org bug report #66759,
regarding 30.0.50; Flymake (with Eglot) error cleaning up old overlay
to be marked as done.

(If you believe you have received this mail in error, please contact
help-debbugs@gnu.org.)


-- 
66759: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66759
GNU Bug Tracking System
Contact help-debbugs@gnu.org with problems

--- Begin Message --- Subject: 30.0.50; Flymake (with Eglot) error cleaning up old overlay Date: Thu, 26 Oct 2023 13:09:07 +0100

I'm afraid I'm unable to consistently reproduce this error. I hope you
can see the issue and devise a testcase from the following
description.

The function `flymake--publish-diagnostics' runs in two phases.
In phase 1 it deletes all the overlays referenced by the (now stale)
existing diagnostics. In phase 2 it creates new overlays for the newly
published diagnostics.

Phase 1 invokes `overlay-buffer' on the value of the :overlay property
of each diagnostic. This signals `wrong-type-argument' if the value is
nil.

Phase 2 intends to set the :overlay property of each diagnostic by
calling `flymake--highlight-line'.

The function `flymake--highlight-line' may return without setting the
:overlay property. For example:

    (when (= (overlay-start ov) (overlay-end ov))
      ;; Some backends report diagnostics with invalid bounds.  Don't
      ;; bother.
      (delete-overlay ov)
      (debug) ;; -- SEE BELOW (last line)
      (cl-return-from flymake--highlight-line nil))

This is likely to occur when `flymake-start` is called by the idle
timer, if there was a diagnostic near the end of the file and the user
has just made the file shorter. In that case, phase 2 inserts a
diagnostic with a null overlay into `flymake--state-diags'.

When `flymake--stated-diags' contains a diagnostic with a null
overlay, the next call to `flymake--publish-diagnostics' signals
`wrong-type-argument' during phase 1. If this next call also has
`flymake-start' in its call stack, the signal is caught in
`flymake--run-backend', which disables the backend.

If the backend continues to call its reporting function (as does Eglot
whenever the language server publishes diagnostics), this leads to a
flood of "Unexpected report from disabled backend %s" errors and an
unusable Emacs session.

A possible fix is to check if `flymake--highlight-line' created an
overlay before inserting a diagnostic into `flymake--state-diags',
in phase 2.

Another is to check that the :overlay property is not null before
attempting to examine and delete the overlay, in phase 1.

Attached is a backtrace from a real-life occurrence of the issue,
obtained by inserting `(debug)' (SEE ABOVE (quoted code)).

backtrace.txt
Description: Text document

--- End Message ---

> --- Begin Message ---
>
>
>
> Subject: 
>
> Re: 30.0.50; Flymake (with Eglot) error cleaning up old overlay
>
>
>
>
> Date: 
>
> Thu, 26 Oct 2023 18:10:46 +0100
>
>
>
>
> User-agent: 
>
> Gnus/5.13 (Gnus v5.13)
>
>
>
>
> Richard Copley <rcopley@gmail.com> writes:
>
> > On Thu, 26 Oct 2023 at 14:24, João Távora <joaotavora@gmail.com> wrote:
> >> Please keep an eye out of this bug.
> >
> > Thanks, will do.
>
> I've now reproduced the bug consistently.  It happens exactly as you
> conjectured.  To test, I temporarily hacked Eglot to take 2 seconds
> longer to process each request/notification with this patch:
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index eba66503bf7..48845a889a8 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -1488,7 +1488,7 @@ eglot--connect
>                       ,@more-initargs)))))
>           (spread (lambda (fn) (lambda (server method params)
>                                  (let ((eglot--cached-server server))
> -                                  (apply fn server method (append params 
> nil))))))
> +                                  (run-at-time 2 nil #'apply fn server method 
> (append params nil))))))
>           (server
>            (apply
>             #'make-instance class
>
> And then made sure there was a diagnostic at the end of the file,
> changed the file and quickly enough (but only after the changes were
> sent to server), deleted the region with the diagnostic at the end of
> the file.
>
> Without my fix, I get exactly the (overlayp nil) error you reported.
> With the fix, everything works correctly.
>
> So I think with the fix I pushed earlier this bug can be closed, which
> I'm doing now.
>
> >> What language server are you using with Eglot btw?
>
> > [1] https://leanprover-community.github.io/learn.html
> > [2] https://github.com/leanprover/lean4-mode
> > [3] https://github.com/bustercopley/lean4-mode
> > [4] https://github.com/joaotavora/eglot/issues/615
> > [5] https://github.com/joaotavora/eglot/pull/839
>
> Yeah I know about these PRs.  If you want can start a new report in the
> Emacs bug tracker proper (i.e. here) )and direct people to it.  Write a
> summary of the situation if you can, and present this Lean4 use case as
> a argument.
>
> This gives visibility to more Emacs maintainers, and allows me to ping
> specialists like Eli more easily on the matter.
>
> João
>
>
> --- End Message ---