Review request: javac in compilation-error-regexp-alist-alist

Review request: javac in compilation-error-regexp-alist-alist

[Filipp Gunbin]

Hi, I've made an attempt to teach compilation-parse-errors to handle
javac output.

We don't have column number there in the compiler output, and it's very
inconvenient, so I'm deducing it from where the visual pointer ("^")
points.

Is this approach fine, or too dirty to be committed?

BTW I fixed a small bug where we check whether file/line/col is consp,
and at the same time allow it to be function, so lambda/closure will
erroneously match.

Besides that, `java' symbol in the said list is a misnomer: it handles
java exceptions (why? we would never normally have them in the
compilation output), and valgrind output.  Looks like we should just
rename it to `valgrind'.

Thanks,
Filipp


diff --git a/etc/compilation.txt b/etc/compilation.txt
index ebce6a14d0..ed2eecd778 100644
--- a/etc/compilation.txt
+++ b/etc/compilation.txt
@@ -237,6 +237,19 @@ Register 6 contains wrong type
 ==1332==    by 0x8008621: main (vtest.c:180)
 
 
+* javac Java compiler
+
+symbol: javac
+
+Should also work when compiling Java with Gradle.  We don't have
+column number, but we try to deduce it from position of "^".
+
+Test.java:5: error: ';' expected
+        foo foo
+               ^
+1 error
+
+
 * IBM jikes
 
 symbols: jikes-file jikes-line
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 455f181f50..890b04864a 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -265,6 +265,23 @@ compilation-error-regexp-alist-alist
     (java
      "^\\(?:[ \t]+at \\|==[0-9]+== +\\(?:at\\|b\\(y\\)\\)\\).+(\\([^()\n]+\\):\\([0-9]+\\))$" 2 3 nil (1))
 
+    (javac
+     ,(concat
+       ;; line1
+       "^\\(\\(?:[A-Za-z]:\\)?[^:\n]+\\): *"      ;file
+       "\\([0-9]+\\): *"                          ;line
+       "\\(?:error:\\|\\(warning\\):\\)?[^\n]*\n" ;type (optional) and message
+       ;; line2: source line containing error
+       "[^\n]*\n"
+       ;; line3: single "^" under error position in line2
+       "[[:space:]]*\\^\n")
+     1 2
+     (lambda ()
+       (save-excursion
+         (backward-char 2)              ; move back over "^\n"
+         (current-column)))
+     (3))
+
     (jikes-file
      "^\\(?:Found\\|Issued\\) .* compiling \"\\(.+\\)\":$" 1 nil nil 0)
 
@@ -1455,9 +1472,15 @@ compilation-parse-errors
         nil) ;; Not anchored or anchored but already allows empty spaces.
        (t (setq pat (concat "^\\(?:      \\)?" (substring pat 1)))))
 
-      (if (consp file)	(setq fmt (cdr file)	  file (car file)))
-      (if (consp line)	(setq end-line (cdr line) line (car line)))
-      (if (consp col)	(setq end-col (cdr col)	  col (car col)))
+      (if (and (consp file) (not (functionp file)))
+	  (setq fmt (cdr file)
+                file (car file)))
+      (if (and (consp line) (not (functionp line)))
+          (setq end-line (cdr line)
+                line (car line)))
+      (if (and (consp col) (not (functionp col)))
+          (setq end-col (cdr col)
+                col (car col)))
 
       (unless (or (null (nth 5 item)) (integerp (nth 5 item)))
         (error "HYPERLINK should be an integer: %s" (nth 5 item)))
diff --git a/test/lisp/progmodes/compile-tests.el b/test/lisp/progmodes/compile-tests.el
index 75962566f1..81452c1c38 100644
--- a/test/lisp/progmodes/compile-tests.el
+++ b/test/lisp/progmodes/compile-tests.el
@@ -176,6 +176,9 @@ compile-tests--test-regexps-data
      13 nil 217 "../src/Lib/System.cpp")
     ("==1332==    by 0x8008621: main (vtest.c:180)"
      13 nil 180 "vtest.c")
+    ;; javac
+    ("/src/Test.java:5: ';' expected\n        foo foo\n               ^\n" 1 15 5 "/src/Test.java" 2)
+    ("e:\\src\\Test.java: 7: warning: ';' expected\n   foo foo\n          ^\n" 1 10 7 "e:\\src\\Test.java" 1)
     ;; jikes-file jikes-line
     ("Found 2 semantic errors compiling \"../javax/swing/BorderFactory.java\":"
      1 nil nil "../javax/swing/BorderFactory.java")
@@ -431,8 +434,8 @@ compile-test-error-regexps
           (compilation-num-warnings-found 0)
           (compilation-num-infos-found 0))
       (mapc #'compile--test-error-line compile-tests--test-regexps-data)
-      (should (eq compilation-num-errors-found 93))
-      (should (eq compilation-num-warnings-found 36))
+      (should (eq compilation-num-errors-found 94))
+      (should (eq compilation-num-warnings-found 37))
       (should (eq compilation-num-infos-found 26)))))
 
 (ert-deftest compile-test-grep-regexps ()

Re: Review request: javac in compilation-error-regexp-alist-alist

[Mattias Engdegård]

27 feb. 2020 kl. 19.17 skrev Filipp Gunbin <fgunbin@fastmail.fm>:

> Besides that, `java' symbol in the said list is a misnomer: it handles
> java exceptions (why? we would never normally have them in the
> compilation output), and valgrind output.  Looks like we should just
> rename it to `valgrind'.

Maybe, but as you can see there are several questionably-named entries in the list. Renaming them may break user customisation.

As it is, it would be useful with a comment explaining the difference between 'java' and 'javac' in the list.

> +    (javac
> +     ,(concat
> +       ;; line1
> +       "^\\(\\(?:[A-Za-z]:\\)?[^:\n]+\\): *"      ;file
> +       "\\([0-9]+\\): *"                          ;line
> +       "\\(?:error:\\|\\(warning\\):\\)?[^\n]*\n" ;type (optional) and message
> +       ;; line2: source line containing error
> +       "[^\n]*\n"
> +       ;; line3: single "^" under error position in line2
> +       "[[:space:]]*\\^\n")

(Could I entice you into writing this regexp in rx? Not mandatory in any way, but at least some of us who read it will thank you.)

Regexps in this list are good to keep tight; don't cast a net wider than you have to. For instance, don't use " *"  unless you know that there may actually be any number of spaces. Most regexps here don't match; optimise for that, rejecting false attempts as early as possible. Be stingy.

There's an unnecessary ambiguity after the line number and colon; a string of spaces can match in multiple ways. Also, avoid [[:space:]] since you don't control that syntax class in the buffer; more likely it should just be plain spaces, as it's very unlikely that the javac caret output routine would use anything else.

> +     1 2
> +     (lambda ()

If you add a comma before the lambda-expression, it gets evaluated, byte-compiled, syntax-checked etc.

> +    ("/src/Test.java:5: ';' expected\n        foo foo\n               ^\n" 1 15 5 "/src/Test.java" 2)
> +    ("e:\\src\\Test.java: 7: warning: ';' expected\n   foo foo\n          ^\n" 1 10 7 "e:\\src\\Test.java" 1)

Why the differences in spacing before the line number? Is it a difference between platforms, error/warning, javac versions, or just different compilers altogether? Comments explaining this would be useful. Be sure to include variants in compilation.txt.

Re: Review request: javac in compilation-error-regexp-alist-alist

[Filipp Gunbin]

Hi Mattias, thanks for the helpful comments, corrected patch below.

On 28/02/2020 18:11 +0100, Mattias Engdegård wrote:

> 27 feb. 2020 kl. 19.17 skrev Filipp Gunbin <fgunbin@fastmail.fm>:
>
>> Besides that, `java' symbol in the said list is a misnomer: it handles
>> java exceptions (why? we would never normally have them in the
>> compilation output), and valgrind output.  Looks like we should just
>> rename it to `valgrind'.
>
> Maybe, but as you can see there are several questionably-named entries
> in the list. Renaming them may break user customisation.

Good point.

> As it is, it would be useful with a comment explaining the difference
> between 'java' and 'javac' in the list.

One mentions java exceptions, another - javac compiler, I guess that's
enough.

> (Could I entice you into writing this regexp in rx? Not mandatory in
> any way, but at least some of us who read it will thank you.)

I'm not experienced with it yet, but noted, will do later.

> There's an unnecessary ambiguity after the line number and colon; a
> string of spaces can match in multiple ways. Also, avoid [[:space:]]
> since you don't control that syntax class in the buffer; more likely
> it should just be plain spaces, as it's very unlikely that the javac
> caret output routine would use anything else.

All corrected.

>> +     1 2
>> +     (lambda ()
>
> If you add a comma before the lambda-expression, it gets evaluated,
> byte-compiled, syntax-checked etc.

Right.

>> +    ("/src/Test.java:5: ';' expected\n        foo foo\n               ^\n" 1 15 5 "/src/Test.java" 2)
>> +    ("e:\\src\\Test.java: 7: warning: ';' expected\n   foo foo\n          ^\n" 1 10 7 "e:\\src\\Test.java" 1)
>
> Why the differences in spacing before the line number? Is it a
> difference between platforms, error/warning, javac versions, or just
> different compilers altogether? Comments explaining this would be
> useful. Be sure to include variants in compilation.txt.

Completely imagined by me, removed.

Filipp


diff --git a/etc/compilation.txt b/etc/compilation.txt
index ebce6a14d0..ed2eecd778 100644
--- a/etc/compilation.txt
+++ b/etc/compilation.txt
@@ -237,6 +237,19 @@ Register 6 contains wrong type
 ==1332==    by 0x8008621: main (vtest.c:180)
 
 
+* javac Java compiler
+
+symbol: javac
+
+Should also work when compiling Java with Gradle.  We don't have
+column number, but we try to deduce it from position of "^".
+
+Test.java:5: error: ';' expected
+        foo foo
+               ^
+1 error
+
+
 * IBM jikes
 
 symbols: jikes-file jikes-line
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 455f181f50..6411c577f7 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -265,6 +265,23 @@ compilation-error-regexp-alist-alist
     (java
      "^\\(?:[ \t]+at \\|==[0-9]+== +\\(?:at\\|b\\(y\\)\\)\\).+(\\([^()\n]+\\):\\([0-9]+\\))$" 2 3 nil (1))
 
+    (javac
+     ,(concat
+       ;; line1
+       "^\\(\\(?:[A-Za-z]:\\)?[^:\n]+\\):" ;file
+       "\\([0-9]+\\): "                    ;line
+       "\\(?:\\(?:error\\|\\(warning\\)\\): \\)?[^\n]*\n" ;type (optional) and message
+       ;; line2: source line containing error
+       "[^\n]*\n"
+       ;; line3: single "^" under error position in line2
+       " *\\^\n")
+     1 2
+     ,(lambda ()
+        (save-excursion
+          (backward-char 2)             ; move back over "^\n"
+          (current-column)))
+     (3))
+
     (jikes-file
      "^\\(?:Found\\|Issued\\) .* compiling \"\\(.+\\)\":$" 1 nil nil 0)
 
@@ -1455,9 +1472,15 @@ compilation-parse-errors
         nil) ;; Not anchored or anchored but already allows empty spaces.
        (t (setq pat (concat "^\\(?:      \\)?" (substring pat 1)))))
 
-      (if (consp file)	(setq fmt (cdr file)	  file (car file)))
-      (if (consp line)	(setq end-line (cdr line) line (car line)))
-      (if (consp col)	(setq end-col (cdr col)	  col (car col)))
+      (if (and (consp file) (not (functionp file)))
+	  (setq fmt (cdr file)
+                file (car file)))
+      (if (and (consp line) (not (functionp line)))
+          (setq end-line (cdr line)
+                line (car line)))
+      (if (and (consp col) (not (functionp col)))
+          (setq end-col (cdr col)
+                col (car col)))
 
       (unless (or (null (nth 5 item)) (integerp (nth 5 item)))
         (error "HYPERLINK should be an integer: %s" (nth 5 item)))
diff --git a/test/lisp/progmodes/compile-tests.el b/test/lisp/progmodes/compile-tests.el
index 75962566f1..cd736497e6 100644
--- a/test/lisp/progmodes/compile-tests.el
+++ b/test/lisp/progmodes/compile-tests.el
@@ -176,6 +176,9 @@ compile-tests--test-regexps-data
      13 nil 217 "../src/Lib/System.cpp")
     ("==1332==    by 0x8008621: main (vtest.c:180)"
      13 nil 180 "vtest.c")
+    ;; javac
+    ("/src/Test.java:5: ';' expected\n        foo foo\n               ^\n" 1 15 5 "/src/Test.java" 2)
+    ("e:\\src\\Test.java:7: warning: ';' expected\n   foo foo\n          ^\n" 1 10 7 "e:\\src\\Test.java" 1)
     ;; jikes-file jikes-line
     ("Found 2 semantic errors compiling \"../javax/swing/BorderFactory.java\":"
      1 nil nil "../javax/swing/BorderFactory.java")
@@ -431,8 +434,8 @@ compile-test-error-regexps
           (compilation-num-warnings-found 0)
           (compilation-num-infos-found 0))
       (mapc #'compile--test-error-line compile-tests--test-regexps-data)
-      (should (eq compilation-num-errors-found 93))
-      (should (eq compilation-num-warnings-found 36))
+      (should (eq compilation-num-errors-found 94))
+      (should (eq compilation-num-warnings-found 37))
       (should (eq compilation-num-infos-found 26)))))
 
 (ert-deftest compile-test-grep-regexps ()

Re: Review request: javac in compilation-error-regexp-alist-alist

[Mattias Engdegård]

29 feb. 2020 kl. 00.02 skrev Filipp Gunbin <fgunbin@fastmail.fm>:

>> (Could I entice you into writing this regexp in rx? Not mandatory in
>> any way, but at least some of us who read it will thank you.)
> 
> I'm not experienced with it yet, but noted, will do later.

It's entirely optional. You could try the 'xr' package to help you translate.

> +    (javac
> +     ,(concat
> +       ;; line1
> +       "^\\(\\(?:[A-Za-z]:\\)?[^:\n]+\\):" ;file
> +       "\\([0-9]+\\): "                    ;line
> +       "\\(?:\\(?:error\\|\\(warning\\)\\): \\)?[^\n]*\n" ;type (optional) and message

Thank you, much better. You may want to use '.' instead of '[^\n]'; they mean exactly the same thing.
There is also no need to match "error" specifically; "\\(warning: \\)?" suffices.

> +       ;; line2: source line containing error
> +       "[^\n]*\n"
> +       ;; line3: single "^" under error position in line2
> +       " *\\^\n")
> +     1 2
> +     ,(lambda ()
> +        (save-excursion
> +          (backward-char 2)             ; move back over "^\n"
> +          (current-column)))

There seems to be an off-by-one error in the column number; try it and you'll see. I think current-column is 0-based but Emacs expects a 1-based column from the compilation error matcher.

A minor bug (probably in compilation-mode) is that the mouse highlighting includes the first character of the line following the caret line. To avoid this, try replacing the final '\n' with '$'. As an extra benefit, the need for save-excursion and backward-char would go away.

This rule does present some performance concerns (perhaps unfounded): the first line, the "file:line: message" part, is likely to match many non-java messages. The entire next line is then matched regardless of contents, and the missing caret finally causes backtracking. I'm not sure how serious this is, or what can be done about it.

Re: Review request: javac in compilation-error-regexp-alist-alist

[Filipp Gunbin]

Mattias, sorry for the long delay, and thanks again for comments,
they're much appreciated.

I've now pushed this as 319a2a7427 in master, except for this thing:

> There seems to be an off-by-one error in the column number; try it and
> you'll see. I think current-column is 0-based but Emacs expects a
> 1-based column from the compilation error matcher.

I didn't notice this because I do (set (make-local-variable
'compilation-first-column) 0) in java-mode, but then other java rules
(ant, maven) may also be buggy, I'll look at this in more detail.

Filipp

Re: Review request: javac in compilation-error-regexp-alist-alist

[Mattias Engdegård]

1 apr. 2020 kl. 02.34 skrev Filipp Gunbin <fgunbin@fastmail.fm>:

>> There seems to be an off-by-one error in the column number; try it and
>> you'll see. I think current-column is 0-based but Emacs expects a
>> 1-based column from the compilation error matcher.
> 
> I didn't notice this because I do (set (make-local-variable
> 'compilation-first-column) 0) in java-mode, but then other java rules
> (ant, maven) may also be buggy, I'll look at this in more detail.

(What java-mode are we talking about here? Some out-of-tree variant?)
There should be no need to set compilation-first-column since the rule does not match a column number directly but has to compute it using a function. In fact, if you remove the change to that variable, the function in the rule is neatly reduced from

  (lambda () (1- (current-column)))

to just

  #'current-column

This is also a benefit for other tools that use the same format, if any -- scalac maybe?

If you like, the same regexp in rx might be

(rx bol                              ; line 1
   (group-n 1 (? (in "A-Za-z") ":")  ; file
              (+ (not (in "\n:"))))
   ":"
   (group-n 2 (+ digit))             ; line
   ": "
   (? (group-n 3 "warning: "))       ; optional warning type
   (* nonl) "\n"                     ; message
   (* nonl) "\n"                     ; line 2: source line
   (* " ") "^" eol)                  ; line 3: Single ^ under error position

Here group-n has been used to make the group numbering stand out for extra clarity.