Flymake: more ambitious cleanup in flymake-mode (bug#69809)

Further improve flymake-mode idempotency by not nuke existing overlays.
This means multiple flymake-mode invocations do the same as just one
one, with minimal or no additional side effects.  This is good for
people with lots of 'flymake-mode' in hooks.

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

A new test has been added.

* lisp/progmodes/flymake.el (flymake--import-foreign-diagnostics): New helper
  (flymake-start): Use it.
  (flymake-mode): Don't nuke overlays here.

* test/lisp/progmodes/flymake-tests.el (foreign-diagnostics): New
  test.

(cherry picked from commit 0ff82eb48725e15bb87a75d4f937b75c2482c59b)

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 2ea991fb912b315280e68fb7c5d335eb5f214412..0d1b4d6f430313567ee32a696772937dc86a0f9f 100644 (file)
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -1452,6 +1452,37 @@ with a report function."
   "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
@@ -1525,7 +1556,8 @@ Interactively, with a prefix arg, FORCE is t."
                                  backend))
                    (t
                     (flymake--run-backend backend backend-args)))
-                  nil))))))))
+                  nil)))
+             (flymake--import-foreign-diagnostics))))))
 
 (defvar flymake-mode-map
   (let ((map (make-sparse-keymap)))
@@ -1591,49 +1623,8 @@ special *Flymake log* buffer."  :group 'flymake :lighter
     ;; 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

diff --git a/test/lisp/progmodes/flymake-tests.el b/test/lisp/progmodes/flymake-tests.el
index c8a20c3f614d22694cf850514493c2725e926210..0e0dba6b57d916e58b4c201aa66c25fc5d38b8dd 100644 (file)
--- a/test/lisp/progmodes/flymake-tests.el
+++ b/test/lisp/progmodes/flymake-tests.el
@@ -120,6 +120,69 @@ SEVERITY-PREDICATE is used to setup
         (flymake-goto-next-error)
         (should (eq 'flymake-error (face-at-point)))))))
 
+(ert-deftest different-diagnostic-types ()
+  "Test GCC warning via function predicate."
+  (skip-unless (and (executable-find "gcc")
+                    (not (ert-gcc-is-clang-p))
+                    (version<=
+                     "5" (string-trim
+                          (shell-command-to-string "gcc -dumpversion")))
+                    (executable-find "make")))
+  (let ((flymake-wrap-around nil))
+    (flymake-tests--with-flymake
+        ("errors-and-warnings.c")
+      (flymake-goto-next-error)
+      (should (eq 'flymake-error (face-at-point)))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-note (face-at-point)))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-warning (face-at-point)))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-error (face-at-point)))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-warning (face-at-point)))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-warning (face-at-point)))
+      (should-error (flymake-goto-next-error nil nil t)))))
+
+(ert-deftest included-c-header-files ()
+  "Test inclusion of .h header files."
+  (skip-unless (and (executable-find "gcc")
+                    (not (ert-gcc-is-clang-p))
+                    (executable-find "make")))
+  (let ((flymake-wrap-around nil))
+    (flymake-tests--with-flymake
+        ("some-problems.h")
+      (flymake-goto-next-error)
+      ;; implicit-int was promoted from warning to error in GCC 14
+      (should (memq (face-at-point) '(flymake-warning flymake-error)))
+      (flymake-goto-next-error)
+      (should (eq 'flymake-error (face-at-point)))
+      (should-error (flymake-goto-next-error nil nil t)))
+    (flymake-tests--with-flymake
+        ("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)