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

["João Távora" <joaotavora@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Wed, Jul 17, 2024 at 6:38 PM João Távora <joaotavora@gmail.com> 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

[-- Attachment #2: 0001-Flymake-improve-idempotence-of-flymake-mode-bug-6980.patch --]
[-- Type: text/x-patch, Size: 5548 bytes --]

From bec56f895c7bc328fc49db04ea700cafcbad837c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Thu, 18 Jul 2024 00:45:20 +0100
Subject: [PATCH] Flymake: improve idempotence of flymake-mode (bug#69809)

* lisp/progmodes/flymake.el (flymake-mode): Don't smash
flymake--state.  Add some comments.  No need to check for
flymake--state nil.
(flymake--project-diagnostics): No need to check for
flymake--state nil.

* test/lisp/progmodes/flymake-tests.el (foreign-diagnostics): New
test.
---
 lisp/progmodes/flymake.el            | 34 +++++++++++++++++-----------
 test/lisp/progmodes/flymake-tests.el | 20 ++++++++++++++++
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index e72f25fd0cd..93d8691838e 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -1391,12 +1391,21 @@ flymake-mode
     ;; AutoResize margins.
     (flymake--resize-margins)
 
-    ;; If Flymake happened to be already ON, we must cleanup
-    ;; existing diagnostic overlays, lest we forget them by blindly
-    ;; reinitializing `flymake--state' in the next line.
-    ;; See https://github.com/joaotavora/eglot/issues/223.
+    ;; We can't just `clrhash' `flymake--state': there may be in
+    ;; in-transit requests from other backends if `flymake-mode' was
+    ;; 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--state (make-hash-table))
     (setq flymake--recent-changes nil)
 
     (when flymake-start-on-flymake-mode (flymake-start t))
@@ -1411,14 +1420,14 @@ flymake-mode
       ;; 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
+      ;; 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 (and flymake-mode flymake--state)
+          (when flymake-mode
             (maphash (lambda (_backend state)
                        (maphash (lambda (file diags)
                                   (when (or (eq file source)
@@ -1446,10 +1455,9 @@ flymake-mode
       (cancel-timer flymake-timer)
       (setq flymake-timer nil))
     (mapc #'flymake--delete-overlay (flymake--really-all-overlays))
-    (when flymake--state
-      (maphash (lambda (_backend state)
-                 (flymake--clear-foreign-diags state))
-               flymake--state))))
+    (maphash (lambda (_backend state)
+               (flymake--clear-foreign-diags state))
+             flymake--state)))
    ;; turning Flymake on or off has consequences for listings
    (flymake--update-diagnostics-listings (current-buffer)))
 
@@ -2040,7 +2048,7 @@ flymake--project-diagnostics
     (cl-loop
      for buf in visited-buffers
      do (with-current-buffer buf
-          (when (and flymake-mode flymake--state)
+          (when flymake-mode
             (maphash
              (lambda (_backend state)
                (maphash
diff --git a/test/lisp/progmodes/flymake-tests.el b/test/lisp/progmodes/flymake-tests.el
index 93bc9028031..8f824ff5009 100644
--- a/test/lisp/progmodes/flymake-tests.el
+++ b/test/lisp/progmodes/flymake-tests.el
@@ -183,6 +183,26 @@ included-c-header-files
         ("no-problems.h")
       (should-error (flymake-goto-next-error nil nil t)))))
 
+(ert-deftest foreign-diagnostics ()
+  "Test Flymake in one file impacts another"
+  (skip-unless (and (executable-find "gcc")
+                    (not (ert-gcc-is-clang-p))
+                    (executable-find "make")))
+  (flymake-tests--with-flymake
+      ("another-problematic-file.c")
+    (flymake-tests--with-flymake
+        ("some-problems.h")
+      (search-forward "frob")
+      (backward-char 1)
+      (should (eq 'flymake-note (face-at-point)))
+      (let ((diags (flymake-diagnostics (point))))
+        (should (= 1 (length diags)))
+        (should (eq :note (flymake-diagnostic-type (car diags))))
+        ;; This note would never been here if it werent' a foreign
+        ;; diagnostic sourced in 'another-problematic-file.c'.
+        (should (string-match "previous declaration"
+                              (flymake-diagnostic-text (car diags))))))))
+
 (defmacro flymake-tests--assert-set (set
                                      should
                                      should-not)
-- 
2.45.2