bug#49624: compilation message end-column function off-by-one bug

bug#49624: compilation message end-column function off-by-one bug

[Mattias Engdegård]

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

Compilation message patterns (in compilation-regexp-alist and -alist) can indicate starting and ending line and column numbers, either by supplying regexp match numbers or functions that return the respective line/column numbers when called. In other words, the integer N can be understood as a shorthand for the function

(lambda () (and (match-beginning N) (string-to-number (match-string N))))

except that isn't true for the ending column where there is a difference of 1; an END-COL function returning 13 means that the error's last column is 12.

There does not seem to be a good justification for this nor is it documented so it's probably just a bug; proposed patch attached below. Does it warrant a mention in etc/NEWS?


[-- Attachment #2: 0001-Fix-off-by-one-error-in-compilation-rule-end-column-.patch --]
[-- Type: application/octet-stream, Size: 3418 bytes --]

From a2303f4651e225115207bad778024f913dc6dacb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 18 Jul 2021 20:32:49 +0200
Subject: [PATCH] Fix off-by-one error in compilation rule end-column function

* lisp/progmodes/compile.el (compilation-error-properties):
When the end-column parameter of a compilation message rule is
a function, treat its return value the same way as textually
supplied values by adjusting both values in the same way.
---
 lisp/progmodes/compile.el            | 13 ++++++++-----
 test/lisp/progmodes/compile-tests.el | 27 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index e4363e11b8..02d1c58858 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1248,11 +1248,14 @@ compilation-error-properties
                  (setq col (match-string-no-properties col))
                  (string-to-number col))))
     (setq end-col
-          (or (if (functionp end-col) (funcall end-col)
-                (and end-col
-                     (setq end-col (match-string-no-properties end-col))
-                     (- (string-to-number end-col) -1)))
-              (and end-line -1)))
+          (let ((ec (if (functionp end-col)
+                        (funcall end-col)
+                      (and end-col (match-beginning end-col)
+                           (string-to-number
+                            (match-string-no-properties end-col))))))
+            (if ec
+                (1+ ec)     ; Add one to get an exclusive upper bound.
+              (and end-line -1))))
     (if (consp type)            ; not a static type, check what it is.
 	(setq type (or (and (car type) (match-end (car type)) 1)
 		       (and (cdr type) (match-end (cdr type)) 0)
diff --git a/test/lisp/progmodes/compile-tests.el b/test/lisp/progmodes/compile-tests.el
index 0623cec528..2a3bb3dafa 100644
--- a/test/lisp/progmodes/compile-tests.el
+++ b/test/lisp/progmodes/compile-tests.el
@@ -515,4 +515,31 @@ compile-test-grep-regexps
       (compile--test-error-line testcase))
     (should (eq compilation-num-errors-found 8))))
 
+(ert-deftest compile-test-functions ()
+  "Test rules using functions instead of regexp group numbers."
+  (let* ((file-fun (lambda () '("my-file")))
+         (line-start-fun (lambda () 123))
+         (line-end-fun (lambda () 134))
+         (col-start-fun (lambda () 39))
+         (col-end-fun (lambda () 24))
+         (compilation-error-regexp-alist-alist
+         `((my-rule
+            ,(rx bol "My error message")
+            ,file-fun
+            (,line-start-fun . ,line-end-fun)
+            (,col-start-fun . ,col-end-fun))))
+         (compilation-error-regexp-alist '(my-rule)))
+  (with-temp-buffer
+    (font-lock-mode -1)
+    (let ((compilation-num-errors-found 0)
+          (compilation-num-warnings-found 0)
+          (compilation-num-infos-found 0))
+      (compile--test-error-line
+       '(my-rule
+         "My error message"
+         1 (39 . 24) (123 . 134) "my-file" 2))
+      (should (eq compilation-num-errors-found 1))
+      (should (eq compilation-num-warnings-found 0))
+      (should (eq compilation-num-infos-found 0))))))
+
 ;;; compile-tests.el ends here
-- 
2.21.1 (Apple Git-122.3)

bug#49624: compilation message end-column function off-by-one bug

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 18 Jul 2021 21:00:41 +0200
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Juri Linkov <juri@linkov.net>
> 
> Compilation message patterns (in compilation-regexp-alist and -alist) can indicate starting and ending line and column numbers, either by supplying regexp match numbers or functions that return the respective line/column numbers when called. In other words, the integer N can be understood as a shorthand for the function
> 
> (lambda () (and (match-beginning N) (string-to-number (match-string N))))
> 
> except that isn't true for the ending column where there is a difference of 1; an END-COL function returning 13 means that the error's last column is 12.

Isn't that because END-COL is the first column _beyond_ the last
column?

bug#49624: compilation message end-column function off-by-one bug

[Mattias Engdegård]

18 juli 2021 kl. 21.10 skrev Eli Zaretskii <eliz@gnu.org>:

> Isn't that because END-COL is the first column _beyond_ the last
> column?

Yes, that is probably the origin of the bug. However as that is just the internal representation (column range as a half-open interval) it should not be visible to the user.

bug#49624: compilation message end-column function off-by-one bug

[Eli Zaretskii]

> Feedback-ID:mattiase@acm.or
> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 18 Jul 2021 21:13:36 +0200
> Cc: 49624@debbugs.gnu.org, monnier@iro.umontreal.ca, juri@linkov.net
> 
> 18 juli 2021 kl. 21.10 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Isn't that because END-COL is the first column _beyond_ the last
> > column?
> 
> Yes, that is probably the origin of the bug. However as that is just the internal representation (column range as a half-open interval) it should not be visible to the user.

??? Why not?

bug#49624: compilation message end-column function off-by-one bug

[Mattias Engdegård]

18 juli 2021 kl. 21.30 skrev Eli Zaretskii <eliz@gnu.org>:

> ??? Why not?

Not sure what you mean (and I don't really think it's something we disagree about) but in any case it's beside the point. Emacs should behave as expected by the user, as documented, and in a consistent way. Currently it does neither.

The more I think about it, the clearer it becomes that it's a plain bug which I'm making too much fuss about and should just go ahead and fix the stupid thing. It was mainly reported on the unlikely chance that Juri and/or Stefan would remember something interesting about the code, but that's secondary.

bug#49624: compilation message end-column function off-by-one bug

[Mattias Engdegård]

19 juli 2021 kl. 23.43 skrev Juri Linkov <juri@linkov.net>:

>> Not even 9dc3a46a444a46e00ed3287a3174d73ed9511dac where the funcall for col and end-col originated?
>> In any case, no worry -- I'll deal with it.
> 
> My apologies.  This commit is so old that I even don't recognize it.
> I need to study this code again since I don't remember this part
> of compile.el.

Thank you, but it's not that important. Let's not overthink it -- I'm treating it as the bug it is and have pushed the obvious fix to master.

bug#49624: compilation message end-column function off-by-one bug

[Juri Linkov]

>>> Not even 9dc3a46a444a46e00ed3287a3174d73ed9511dac
>>> where the funcall for col and end-col originated?
>>> In any case, no worry -- I'll deal with it.
>>
>> My apologies.  This commit is so old that I even don't recognize it.
>> I need to study this code again since I don't remember this part
>> of compile.el.
>
> Thank you, but it's not that important.  Let's not overthink it -- I'm
> treating it as the bug it is and have pushed the obvious fix to master.

I'm terribly sorry that it took so much time for me to realize
that my commit 9dc3a46a444a46e00ed3287a3174d73ed9511dac
was part of efforts to add column information to grep matches,
also I needed to inspect the history of column-related code in compile.el
to understand the reason of incrementing the value of end-col.
Here are my findings:

The commit c0090c20f88d1e8c99e9823db5b9cc25d98672bc with the log message

    (compilation-error-properties): Store one more than end-col, if present, so
    that transient-mark-mode will highlight last char too.

turned an exclusive upper bound (e.g. [4, 6) that highlighted 2 chars)
into an inclusive upper bound (e.g. [4, 6] that highlighted 3 chars)
on the assumption that most compilation tools report inclusive ranges.

Then without changing this logic in 9dc3a46a444a46e00ed3287a3174d73ed9511dac
I added a funcall without incrementing its result by 1
on the assumption that the function can return
an already inclusive result that doesn't need to offset by 1.

Now your commit aa5437493b1ca539409495ecdc54cf420ea110b9
broke the highlighting of columns in grep-regexp-alist,
so now visiting a grep match highlights an additional character
that is not part of the grep match.

Maybe there are more existing functions whose backward-compatibility
is broken now.  For example,

    (javac
     ,...
     1 2
     ,#'current-column
     (3))

uses ,#'current-column although not for end-col, so it's not affected.

bug#49624: compilation message end-column function off-by-one bug

[Mattias Engdegård]

26 juli 2021 kl. 19.06 skrev Juri Linkov <juri@linkov.net>:

>  my commit 9dc3a46a444a46e00ed3287a3174d73ed9511dac
> was part of efforts to add column information to grep matches,

> I added a funcall without incrementing its result by 1
> on the assumption that the function can return
> an already inclusive result that doesn't need to offset by 1.

Thank you for the thorough investigation!

> Now your commit aa5437493b1ca539409495ecdc54cf420ea110b9
> broke the highlighting of columns in grep-regexp-alist,
> so now visiting a grep match highlights an additional character
> that is not part of the grep match.

Good spotting! That one has now been fixed.

> Maybe there are more existing functions whose backward-compatibility
> is broken now.

In preparation of my change I went through lots and lots of external packages and found exactly none using a function for END-COL. This is unsurprising since the very ability to use functions for COL and END-COL wasn't documented until 2019.

Somehow I missed grep.el, but that too makes sense -- it's inside Emacs itself that the feature was most likely to be used since it was undocumented. I have now gone through the source tree once more, and there seem to be no more problems.