Subject: bug#69809: 30.0.50; flymake: error in process sentinel

From: João Távora <[email-masked]>
Date: Thu, 18 Jul 2024 01:10:49 +0100

And here's another more ambitious cleanup patch.
Be more careful with this one, test it with as many
Flymake backends as you can find.

On Thu, Jul 18, 2024 at 12:54 AM João Távora <[email-masked]> wrote:
>
> On Wed, Jul 17, 2024 at 6:38 PM João Távora <[email-masked]> wrote:
>
> > > > Anyway, can you try this patch?
> > >
> > > That seems to work too :)
> >
> > I understand the source of _this_ problem, and the line I changed
> > addresses it.  My worry is that my fix also creates more problems,
> > but it seems cleaner.
>
> Indeed it did create some subtle problems with "foreign diagnostics".
> I made a better patch, attached. It should fix the Eglot/flymake-cc
> scenario and be a net improvement for Flymake.  Also adds a new
> Flymake test.
>
>  Spencer please have a look and push it if you agree.
>
> João

-- 
João Távora
From 6def8bd5bd221ed401c843bb9c7014efb78ed28f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <[email-masked]>
Date: Thu, 18 Jul 2024 01:09:10 +0100
Subject: [PATCH] Flymake: more ambitious cleanup in flymake-mode (bug#69809)

Should be more idempotent than before, because it doesn't nuke
existing overlays.  This means multiple flymake-mode does the
same as one with minimal or no side effects, which is good for
people with lots of 'flymake-mode' in hooks.

The foreign diagnostic importation has been moved to the "really
start" section of 'flymake-start'.  The duplication problem
appears to be avoided by some heuristics in
flymake-highlight-line.

* lisp/progmodes/flymake.el (flymake--import-foreign-diagnostics): New helper
(flymake-start): Use it.
(flymake-mode): Don't nuke overlays here.
---
 lisp/progmodes/flymake.el | 77 +++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 43 deletions(-)

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 93d8691838e..5488230ae23 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -1257,6 +1257,37 @@ flymake--recent-changes
   "Recent changes collected by `flymake-after-change-function'.")
 (defvar flymake-mode)
 
+(defun flymake--import-foreign-diagnostics ()
+  ;; Other diagnostic sources may already target this buffer's file
+  ;; before we turned on: these sources may be of two types...
+  (let ((source (current-buffer))
+        (bfn buffer-file-name))
+    ;; 1. For `flymake-list-only-diagnostics': here, we do nothing.
+    ;; FIXME: We could remove the corresponding entry from that
+    ;; variable, as we assume that new diagnostics will come in soon
+    ;; via the brand new `flymake-mode' setup.  For simplicity's
+    ;; sake, we have opted to leave the backend for now.
+    nil
+    ;; 2. other buffers where a backend has created "foreign
+    ;; diagnostics" and pointed them here.  We must highlight them in
+    ;; this buffer, i.e. create overlays for them.  Those other
+    ;; buffers and backends are still responsible for them, i.e. the
+    ;; current buffer does not "own" these foreign diags.
+    (dolist (buffer (buffer-list))
+      (with-current-buffer buffer
+        (when flymake-mode
+          (maphash (lambda (_backend state)
+                     (maphash (lambda (file diags)
+                                (when (or (eq file source)
+                                          (string= bfn (expand-file-name file)))
+                                  (with-current-buffer source
+                                    (mapc (lambda (diag)
+                                            (flymake--highlight-line diag
+                                                                     'foreign))
+                                          diags))))
+                              (flymake--state-foreign-diags state)))
+                   flymake--state))))))
+
 (defun flymake-start (&optional deferred force)
   "Start a syntax check for the current buffer.
 DEFERRED is a list of symbols designating conditions to wait for
@@ -1330,7 +1361,8 @@ flymake-start
                                  backend))
                    (t
                     (flymake--run-backend backend backend-args)))
-                  nil))))))))
+                  nil)))
+             (flymake--import-foreign-diagnostics))))))
 
 (defvar flymake-mode-map
   (let ((map (make-sparse-keymap)))
@@ -1396,49 +1428,8 @@ flymake-mode
     ;; already active.  I.e. `flymake-mode' function should be as
     ;; idempotent as possible.  See bug#69809.
     (unless flymake--state (setq flymake--state (make-hash-table)))
-
-    ;; On a related note to bug#69809, deleting all Flymake overlays is
-    ;; a violation of that idempotence.  This could be addressed in the
-    ;; future.  However, there is at least one known reason for doing so
-    ;; currently: since "foreign diagnostics" are created here, we opt
-    ;; to delete everything to avoid duplicating overlays.  In
-    ;; principle, the next `flymake-start' should re-synch everything
-    ;; (and with high likelyhood that is right around the corner if
-    ;; `flymake-start-on-flymake-mode' is t).
-    (mapc #'flymake--delete-overlay (flymake--really-all-overlays))
     (setq flymake--recent-changes nil)
-
-    (when flymake-start-on-flymake-mode (flymake-start t))
-
-    ;; Other diagnostic sources may already target this buffer's file
-    ;; before we turned on: these sources may be of two types...
-    (let ((source (current-buffer))
-          (bfn buffer-file-name))
-      ;; 1. For `flymake-list-only-diagnostics': here, we do nothing.
-      ;; FIXME: We could remove the corresponding entry from that
-      ;; variable, as we assume that new diagnostics will come in soon
-      ;; via the brand new `flymake-mode' setup.  For simplicity's
-      ;; sake, we have opted to leave the backend for now.
-      nil
-      ;; 2. other buffers where a backend has created "foreign
-      ;; diagnostics" and pointed them here.  We must highlight them in
-      ;; this buffer, i.e. create overlays for them.  Those other
-      ;; buffers and backends are still responsible for them, i.e. the
-      ;; current buffer does not "own" these foreign diags.
-      (dolist (buffer (buffer-list))
-        (with-current-buffer buffer
-          (when flymake-mode
-            (maphash (lambda (_backend state)
-                       (maphash (lambda (file diags)
-                                  (when (or (eq file source)
-                                            (string= bfn (expand-file-name file)))
-                                    (with-current-buffer source
-                                      (mapc (lambda (diag)
-                                              (flymake--highlight-line diag
-                                                                       'foreign))
-                                            diags))))
-                                (flymake--state-foreign-diags state)))
-                     flymake--state))))))
+    (when flymake-start-on-flymake-mode (flymake-start t)))
 
    ;; Turning the mode OFF.
    (t
-- 
2.45.2