[Emacs-diffs] scratch/flymake-refactor 7d3d3d3 38/52: Fix flymake problems when checking C header files

[João Távora]

branch: scratch/flymake-refactor
commit 7d3d3d3e40eeb241cdd66f2df8bd55615a34c0b3
Author: João Távora <address@hidden>
Commit: João Távora <address@hidden>

    Fix flymake problems when checking C header files
    
    The first of these problems is longstanding: if an error-less B.h is
    included from error-ridden A.h, flymake's legacy parser will panic
    (and disable itself) since it sees a non-zero exit for a clean file.
    To fix this, recommend returning 'true' in the documentation for the
    check-syntax target.
    
    Another problem was introduced by the parser rewrite.  For error
    patterns spanning more than one line, point may be left in the middle
    of a line and thus render other patterns useless.  Those patterns were
    written for the old line-by-line parser.  To make them useful again,
    move to the beginning of line in those situations.
    
    The third problem was also longstanding and happened on newer GCC's:
    The "In file included from" prefix confused
    flymake-proc-get-real-file-name.  Fix this.
    
    Also updated flymake--diag-region to fallback to highlighting a full
    line less often.
    
    Add automatic tests to check this.
    
    * lisp/progmodes/flymake-proc.el
    (flymake-proc--diagnostics-for-pattern): Fix bug when patterns
    accidentally spans more than one line.  Don't create
    diagnostics without error messages.
    (flymake-proc-real-file-name-considering-includes): New
    helper.
    (flymake-proc-allowed-file-name-masks): Use it.
    
    * lisp/progmodes/flymake-ui.el (flymake-diag-region): Make COL
    argument explicitly optional.  Only fall back to full line in extreme
    cases.  (flymake-error): Fix silly error.
    
    * test/lisp/progmodes/flymake-tests.el
    (included-c-header-files): New test.
    (different-diagnostic-types): Update.
    
    * test/lisp/progmodes/flymake-resources/Makefile
    (check-syntax): Always return success (0) error code.
    (CC_OPTS): Add -Wextra
    
    * test/lisp/progmodes/flymake-resources/errors-and-warnings.c
    (main): Rewrite comments.
    
    * test/lisp/progmodes/flymake-resources/errors-and-warnings.c:
    Include some dummy header files.
    
    * test/lisp/progmodes/flymake-resources/no-problems.h: New file.
    
    * test/lisp/progmodes/flymake-resources/some-problems.h: New file.
    
    * doc/misc/flymake.texi (Example---Configuring a tool called
    via make): Recommend adding "|| true" to the check-syntax target.
---
 doc/misc/flymake.texi                              |  4 +--
 lisp/progmodes/flymake-proc.el                     | 32 ++++++++++++++++++----
 lisp/progmodes/flymake-ui.el                       | 27 ++++++++++--------
 test/lisp/progmodes/flymake-resources/Makefile     |  4 +--
 .../flymake-resources/errors-and-warnings.c        | 15 ++++++----
 .../lisp/progmodes/flymake-resources/no-problems.h |  1 +
 .../progmodes/flymake-resources/some-problems.h    |  5 ++++
 test/lisp/progmodes/flymake-tests.el               | 20 ++++++++++++++
 8 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/doc/misc/flymake.texi b/doc/misc/flymake.texi
index a1abf04..61133a0 100644
--- a/doc/misc/flymake.texi
+++ b/doc/misc/flymake.texi
@@ -488,7 +488,7 @@ our case this target might look like this:
 
 @verbatim
 check-syntax:
-       gcc -o /dev/null -S ${CHK_SOURCES}
+       gcc -o /dev/null -S ${CHK_SOURCES} || true
 @end verbatim
 
 @noindent
@@ -500,7 +500,7 @@ Automake variable @code{COMPILE}:
 
 @verbatim
 check-syntax:
-       $(COMPILE) -o /dev/null -S ${CHK_SOURCES}
+       $(COMPILE) -o /dev/null -S ${CHK_SOURCES} || true
 @end verbatim
 
 @node Flymake Implementation
diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el
index bbba180..ff12f47 100644
--- a/lisp/progmodes/flymake-proc.el
+++ b/lisp/progmodes/flymake-proc.el
@@ -66,7 +66,10 @@
   :type 'integer)
 
 (defcustom flymake-proc-allowed-file-name-masks
-  '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'" 
flymake-proc-simple-make-init)
+  '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'"
+     flymake-proc-simple-make-init
+     nil
+     flymake-proc-real-file-name-considering-includes)
     ("\\.xml\\'" flymake-proc-xml-init)
     ("\\.html?\\'" flymake-proc-xml-init)
     ("\\.cs\\'" flymake-proc-simple-make-init)
@@ -419,12 +422,25 @@ Create parent directories as needed."
     (condition-case-unless-debug err
         (cl-loop
          with (regexp file-idx line-idx col-idx message-idx) = pattern
-         while (search-forward-regexp regexp nil t)
+         while (and
+                (search-forward-regexp regexp nil t)
+                ;; If the preceding search spanned more than one line,
+                ;; move to the start of the line we ended up in. This
+                ;; preserves the usefulness of the patterns in
+                ;; `flymake-proc-err-line-patterns', which were
+                ;; written primarily for flymake's original
+                ;; line-by-line parsing and thus never spanned
+                ;; multiple lines.
+                (if (/= (line-number-at-pos (match-beginning 0))
+                        (line-number-at-pos))
+                    (goto-char (line-beginning-position))
+                  t))
          for fname = (and file-idx (match-string file-idx))
          for message = (and message-idx (match-string message-idx))
          for line-string = (and line-idx (match-string line-idx))
-         for line-number = (and line-string
-                                (string-to-number line-string))
+         for line-number = (or (and line-string
+                                    (string-to-number line-string))
+                               1)
          for col-string = (and col-idx (match-string col-idx))
          for col-number = (and col-string
                                (string-to-number col-string))
@@ -436,7 +452,7 @@ Create parent directories as needed."
                                  fname)))
          for buffer = (and full-file
                            (find-buffer-visiting full-file))
-         if (eq buffer (process-buffer proc))
+         if (and (eq buffer (process-buffer proc)) message)
          collect (with-current-buffer buffer
                    (pcase-let ((`(,beg . ,end)
                                 (flymake-diag-region line-number col-number)))
@@ -1030,6 +1046,12 @@ Use CREATE-TEMP-F for creating temp copy."
    '("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'")
    "[ \t]*#[ \t]*include[ \t]*\"\\([[:word:]0-9/\\_.]*%s\\)\""))
 
+(defun flymake-proc-real-file-name-considering-includes (scraped)
+  (flymake-proc-get-real-file-name
+   (replace-regexp-in-string "^in file included from[ \t*]"
+                             ""
+                             scraped)))
+
 ;;;; .java/make specific
 (defun flymake-proc-simple-make-java-init ()
   (flymake-proc-simple-make-init-impl 
'flymake-proc-create-temp-with-folder-structure nil nil "Makefile" 
'flymake-proc-get-make-cmdline))
diff --git a/lisp/progmodes/flymake-ui.el b/lisp/progmodes/flymake-ui.el
index 4ac9e0a..96711f8 100644
--- a/lisp/progmodes/flymake-ui.el
+++ b/lisp/progmodes/flymake-ui.el
@@ -148,10 +148,9 @@ are the string substitutions (see the function `format')."
 
 (defun flymake-error (text &rest args)
   "Signal an error for flymake."
-  (let ((msg (format-message text args)))
+  (let ((msg (apply #'format-message text args)))
     (flymake-log :error msg)
-    (error (concat "[flymake] "
-                   (format text args)))))
+    (error (concat "[flymake] " msg))))
 
 (cl-defstruct (flymake--diag
                (:constructor flymake--diag-make))
@@ -235,9 +234,10 @@ verify FILTER, sort them by COMPARE (using KEY)."
 (define-obsolete-face-alias 'flymake-warnline 'flymake-warning "26.1")
 (define-obsolete-face-alias 'flymake-errline 'flymake-error "26.1")
 
-(defun flymake-diag-region (line col)
+(defun flymake-diag-region (line &optional col)
   "Compute region (BEG . END) corresponding to LINE and COL.
-Or nil if the region is invalid."
+If COL is nil, return a region just for LINE.
+Return nil if the region is invalid."
   (condition-case-unless-debug _err
       (let ((line (min (max line 1)
                        (line-number-at-pos (point-max) 'absolute))))
@@ -254,13 +254,18 @@ Or nil if the region is invalid."
                        (if (eq (point) beg)
                            (line-beginning-position 2)
                          (point)))))
-            (if col
-                (let* ((beg (progn (forward-char (1- col)) (point)))
+            (if (and col (cl-plusp col))
+                (let* ((beg (progn (forward-char (1- col))
+                                   (point)))
                        (sexp-end (ignore-errors (end-of-thing 'sexp)))
-                       (end (or sexp-end
-                                (fallback-eol beg))))
-                  (cons (if sexp-end beg (fallback-bol))
-                        end))
+                       (end (or (and sexp-end
+                                     (not (= sexp-end beg))
+                                     sexp-end)
+                                (ignore-errors (goto-char (1+ beg)))))
+                       (safe-end (or end
+                                     (fallback-eol beg))))
+                  (cons (if end beg (fallback-bol))
+                        safe-end))
               (let* ((beg (fallback-bol))
                      (end (fallback-eol beg)))
                 (cons beg end))))))
diff --git a/test/lisp/progmodes/flymake-resources/Makefile 
b/test/lisp/progmodes/flymake-resources/Makefile
index 0f3f397..4944075 100644
--- a/test/lisp/progmodes/flymake-resources/Makefile
+++ b/test/lisp/progmodes/flymake-resources/Makefile
@@ -1,6 +1,6 @@
 # Makefile for flymake tests
 
-CC_OPTS = -Wall
+CC_OPTS = -Wall -Wextra
 
 ## Recent gcc (e.g. 4.8.2 on RHEL7) can automatically colorize their output,
 ## which can confuse flymake.  Set GCC_COLORS to disable that.
@@ -8,6 +8,6 @@ CC_OPTS = -Wall
 ## normally use flymake, so it seems like just avoiding the issue
 ## in this test is fine.  Set flymake-log-level to 3 to investigate.
 check-syntax:
-       GCC_COLORS= $(CC) $(CC_OPTS) ${CHK_SOURCES}
+       GCC_COLORS= $(CC) $(CC_OPTS) ${CHK_SOURCES} || true
 
 # eof
diff --git a/test/lisp/progmodes/flymake-resources/errors-and-warnings.c 
b/test/lisp/progmodes/flymake-resources/errors-and-warnings.c
index 6454dd2..1d38bd6 100644
--- a/test/lisp/progmodes/flymake-resources/errors-and-warnings.c
+++ b/test/lisp/progmodes/flymake-resources/errors-and-warnings.c
@@ -1,10 +1,13 @@
- int main()
+/* Flymake should notice an error on the next line, since
+   that file has at least one warning.*/
+#include "some-problems.h"
+/* But not this one */
+#include "no-problems.h"
+
+int main()
 {
-  char c = 1000;
+  char c = 1000; /* a note and a warning */
   int bla;
-  /* The following line should have one warning and one error. The
-     warning spans the full line because gcc (at least 6.3.0) points
-     places the error at the =, which isn't a sexp.*/
-  char c; if (bla == (void*)3);
+  char c; if (bla == (void*)3); /* an error, and two warnings */
   return c;
 }
diff --git a/test/lisp/progmodes/flymake-resources/no-problems.h 
b/test/lisp/progmodes/flymake-resources/no-problems.h
new file mode 100644
index 0000000..19ddc61
--- /dev/null
+++ b/test/lisp/progmodes/flymake-resources/no-problems.h
@@ -0,0 +1 @@
+typedef int no_problems;
diff --git a/test/lisp/progmodes/flymake-resources/some-problems.h 
b/test/lisp/progmodes/flymake-resources/some-problems.h
new file mode 100644
index 0000000..165d8dd
--- /dev/null
+++ b/test/lisp/progmodes/flymake-resources/some-problems.h
@@ -0,0 +1,5 @@
+#include <stdio.h>
+
+strange;
+
+sint main();
diff --git a/test/lisp/progmodes/flymake-tests.el 
b/test/lisp/progmodes/flymake-tests.el
index e313846..515aa08 100644
--- a/test/lisp/progmodes/flymake-tests.el
+++ b/test/lisp/progmodes/flymake-tests.el
@@ -121,14 +121,34 @@ SEVERITY-PREDICATE is used to setup
   (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)))
+    (let ((flymake-wrap-around nil))
+      (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") (executable-find "make")))
+  (flymake-tests--with-flymake
+      ("some-problems.h")
+    (flymake-goto-next-error)
     (should (eq 'flymake-warning (face-at-point)))
     (flymake-goto-next-error)
     (should (eq 'flymake-error (face-at-point)))
     (let ((flymake-wrap-around nil))
+      (should-error (flymake-goto-next-error nil nil t))) )
+  (flymake-tests--with-flymake
+      ("no-problems.h")
+    (let ((flymake-wrap-around nil))
       (should-error (flymake-goto-next-error nil nil t))) ))
 
 (defmacro flymake-tests--assert-set (set