From 01eaa23ac3982549f1107072981e460bfc962e1e Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jo=C3=A3o=20T=C3=A1vora?= Date: Thu, 18 Jul 2024 01:09:10 +0100 Subject: [PATCH] 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) --- lisp/progmodes/flymake.el | 77 ++++++++++++---------------- test/lisp/progmodes/flymake-tests.el | 63 +++++++++++++++++++++++ 2 files changed, 97 insertions(+), 43 deletions(-) diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el index 2ea991fb912..0d1b4d6f430 100644 --- 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 c8a20c3f614..0e0dba6b57d 100644 --- 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) -- 2.39.5