bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Sometimes next-error goes to the wrong line when stepping through rgrep
matches.

Using git bisect and the code below, I found the commit that causes the
problem (rev 105677). Reverting that (and before that a commit that
depended on it) makes the problem disappear.

rev 105685: (grep-regexp-alist): Move dangling comment to the previous rule.
rev 105677: progmodes/grep.el (grep-regexp-alist): Calculate column positions

I used the following test case to find the problematic commit. It
creates a file with two lines matching "mapcar" and then rgreps for it.
After that it calls next-error two times. The second call goes into the
line starting with " (list 'face ...".
                          ^- here

--- code begins ----

(let ((dir (expand-file-name
	    (format "tmp%d" (emacs-pid))
	    temporary-file-directory)))
  (ignore-errors (make-directory dir))

  (with-temp-file (expand-file-name "test.el" dir)
    (insert
"(defun my-diary-lib--date-to-string (date)
  (destructuring-bind (month day year)
      (mapcar 'number-to-string date)
    (concat day \".\" month \".\" year)))

\(defun my-diary-lib--string-to-date (datestr)
  (destructuring-bind (day month year)
      (mapcar #'string-to-number (split-string datestr \"\\\\.\"))
    (list month day year)))

\(defun my-diary-lib-add-dayname (datestr)
  (list 'face 'font-lock-comment-face
	'display (concat datestr \" \" (calendar-day-name
					(my-diary-lib--string-to-date datestr)))))
"))

  (grep-compute-defaults)
  (rgrep "mapcar" "test.el" dir)
  (run-with-idle-timer 0.5 nil #'next-error 1)
  (run-with-idle-timer 1.0 nil #'next-error 1))

--- code ends ---


In GNU Emacs 24.0.90.7 (x86_64-unknown-linux-gnu, GTK+ Version 3.2.0)
 of 2011-10-06 on arirop
Windowing system distributor `Fedora Project', version 11.0.11000000
configured using `configure  '--with-x-toolkit=gtk3''

-- 
Ari Roponen

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Juri Linkov]

> Sometimes next-error goes to the wrong line when stepping through rgrep
> matches.
>
> Using git bisect and the code below, I found the commit that causes the
> problem (rev 105677). Reverting that (and before that a commit that
> depended on it) makes the problem disappear.
>
> rev 105685: (grep-regexp-alist): Move dangling comment to the previous rule.
> rev 105677: progmodes/grep.el (grep-regexp-alist): Calculate column positions
>
> I used the following test case to find the problematic commit. It
> creates a file with two lines matching "mapcar" and then rgreps for it.
> After that it calls next-error two times. The second call goes into the
> line starting with " (list 'face ...".
>                           ^- here

Thanks for the comprehensive test case.  I tried it but the second call
goes into the line starting with "(mapcar #'string-to-number ...".

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Juri Linkov <juri@jurta.org> writes:

> Thanks for the comprehensive test case.  I tried it but the second call
> goes into the line starting with "(mapcar #'string-to-number ...".

Hi,

I compared the behavior between the version with commits reverted and
not reverted.

In the reverted commits case, typing M-g M-p and M-g M-n goes to the
correct line and column, but highlights the whole line.

In the non-reverted commits case, typing those commands goes to the
wrong line but right column. The highlight seems to start at the end of
correct match and end at the cursor position.

I found something that may be relevant to the problem. These are the
values of *grep*-buffer's variable compilation-locs in both cases:

* Reverted commits case:

Value: #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
	      (
	       ("./test.el")
	       (("./test.el" nil)
		nil
		(8
		 (nil 8 #1 #<marker at 251 in test.el> nil . t))
		(3
		 (nil 3 #1 #<marker at 89 in test.el> nil . t)))))

* Non-reverted commits case:

Value: #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
	      (("Grep started at Fri Oct  7 07")
	       (("Grep started at Fri Oct  7 07" nil)
		nil
		(31
		 (nil 31 #1 nil nil)))
	       ("./test.el")
	       (("./test.el" nil)
		nil
		(8
		 (13 8 #1 #<marker at 258 in test.el> nil)
		 (7 8 #1 #<marker at 387 in test.el> nil . t))
		(3
		 (13 3 #1 #<marker at 96 in test.el> nil)
		 (7 3 #1 #<marker at 90 in test.el> nil . t)))
	       ("Grep finished (matches found) at Fri Oct  7 07")
	       (("Grep finished (matches found) at Fri Oct  7 07" nil)
		nil
		(31
		 (nil 31 #1 nil nil)))))

In the non-reverted commits case, The marker pairs seem to delimit the
matched text. The first pair (from 258 to 387) starts at the end of
correct match and goes too far. The second pair (from 90 to 96)
highlight correctly the first match.

I noticed that killing the buffer the markers use seems to fix the
problem:

1. Run the test case from the original bug report.
2. Kill the "test.el"-buffer
3. Type M-g M-p M-g M-n

Now the cursor is at the beginning of the match and the match is
highlighted correctly. The value of compilation-locs in *grep*-buffer is
also correct:

Value: #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
	      (("Grep started at Fri Oct  7 07")
	       (("Grep started at Fri Oct  7 07" nil)
		nil
		(56
		 (nil 56 #1 nil nil)))
	       ("./test.el")
	       (("./test.el" nil)
		nil
		(8
		 (13 8 #1 #<marker at 258 in test.el> nil)
		 (7 8 #1 #<marker at 252 in test.el> nil . t))
		(3
		 (13 3 #1 #<marker at 96 in test.el> nil)
		 (7 3 #1 #<marker at 90 in test.el> nil . t)))
	       ("Grep finished (matches found) at Fri Oct  7 07")
	       (("Grep finished (matches found) at Fri Oct  7 07" nil)
		nil
		(56
		 (nil 56 #1 nil nil)))))

I guess the problem has something to do with those markers.

-- 
Ari Roponen

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Ari Roponen <ari.roponen@gmail.com> writes:

> I guess the problem has something to do with those markers.

I found a way to fix the problem locally without reverting any commits.

The function `compilation-next-error-function' contains this:

    ;; If loc contains no marker, no error in that file has been visited.
    ;; If the marker is invalid the buffer has been killed.
    ;; So, recalculate all markers for that file.
    (unless (and (compilation--loc->marker loc)
                 (marker-buffer (compilation--loc->marker loc))
                 ;; FIXME-omake: For "omake -P", which automatically recompiles
                 ;; when the file is modified, the line numbers of new output
                 ;; may not be related to line numbers from earlier output

When I add "nil" after "and", the problem goes away.

-- 
Ari Roponen

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Juri Linkov]

> 	       ("./test.el")
> 	       (("./test.el" nil)
> 		nil
> 		(8
> 		 (13 8 #1 #<marker at 258 in test.el> nil)
> 		 (7 8 #1 #<marker at 387 in test.el> nil . t))
> 		(3
> 		 (13 3 #1 #<marker at 96 in test.el> nil)
> 		 (7 3 #1 #<marker at 90 in test.el> nil . t)))

This is odd, I have in the non-reverted commits case
after visiting the second match:

  #s(hash-table size 65 test equal weakness value rehash-size 1.5 rehash-threshold 0.8 data
   (("./test.el")
    (("./test.el" nil)
     nil
     (8
      (13 8 #1 #<marker at 258 in test.el> nil)
      (7 8 #1 #<marker at 252 in test.el> nil . t))
     (3
      (13 3 #1 #<marker at 96 in test.el> nil)
      (7 3 #1 #<marker at 90 in test.el> nil . t)))))

So unfortunately I can't reproduce this bug.

I suspect it may be related to screen columns because somehow
`compilation-move-to-column' in `compilation-next-error-function'
goes to the wrong line and column, i.e. to the position 387.
What values of `compilation-error-screen-columns' and
`grep-error-screen-columns' do you have in the *grep* buffer?
I have both of them equal to `nil'.

Could you please get the values of arguments that
`compilation-move-to-column' receives here by debugging
or adding (message "...") and seeing what values it prints
in the *Message* buffer?

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Juri Linkov <juri@jurta.org> writes:

> I suspect it may be related to screen columns because somehow
> `compilation-move-to-column' in `compilation-next-error-function'
> goes to the wrong line and column, i.e. to the position 387.
> What values of `compilation-error-screen-columns' and
> `grep-error-screen-columns' do you have in the *grep* buffer?
> I have both of them equal to `nil'.

I run the test case with "emacs -Q -l bug.el", so the variables
use their default values:

compilation-error-screen-columns is a variable defined in `compile.el'.
Its value is nil
Original value was t
Local in buffer *grep*; global value is t

grep-error-screen-columns is a variable defined in `grep.el'.
Its value is nil

>
> Could you please get the values of arguments that
> `compilation-move-to-column' receives here by debugging
> or adding (message "...") and seeing what values it prints
> in the *Message* buffer?

(defun compilation-move-to-column (col screen)
  "Go to column COL on the current line.
If SCREEN is non-nil, columns are screen columns, otherwise, they are
just char-counts."
  (message "col: %S, screen %S" col screen) ; Display parameters.
  (if screen
      (move-to-column (max col 0))
    (goto-char (min (+ (line-beginning-position) col) (line-end-position)))))

=>
Grep finished (matches found)
col: 13, screen nil
col: 7, screen nil
col: 13, screen t
col: 7, screen t

I get the same values also with the simpler "fix" that I mentioned in
the other post.

-- 
Ari Roponen

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Juri Linkov]

> col: 13, screen nil
> col: 7, screen nil
> col: 13, screen t
> col: 7, screen t

Something is completely broken in your test case, because I get:

col: 13, screen nil
col: 7, screen nil
col: 13, screen nil
col: 7, screen nil

i.e. `nil' is what's expected for the second match.

I don't understand how `t' is possible.  It looks like
`compilation-next-error-function' is not in `grep-mode' at that time.

Could you please add the following `message' to `compilation-next-error-function':

(defun compilation-next-error-function (n &optional reset)
  "Advance to the next error message and visit the file where the error was.
This is the value of `next-error-function' in Compilation buffers."
  (interactive "p")
  (when reset
    (setq compilation-current-error nil))
  (message "mode: %S, buffer: %S" major-mode (current-buffer))
  ...

I get the following output:

mode: grep-mode, buffer: #<buffer *grep*>
col: 13, screen nil
col: 7, screen nil
col: 13, screen nil
col: 7, screen nil
mode: grep-mode, buffer: #<buffer *grep*>

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Juri Linkov <juri@jurta.org> writes:

> I don't understand how `t' is possible.  It looks like
> `compilation-next-error-function' is not in `grep-mode' at that time.
>
> Could you please add the following `message' to `compilation-next-error-function':
>
> (defun compilation-next-error-function (n &optional reset)
>   "Advance to the next error message and visit the file where the error was.
> This is the value of `next-error-function' in Compilation buffers."
>   (interactive "p")
>   (when reset
>     (setq compilation-current-error nil))
>   (message "mode: %S, buffer: %S" major-mode (current-buffer))
>   ...
>
> I get the following output:
>
> mode: grep-mode, buffer: #<buffer *grep*>
> col: 13, screen nil
> col: 7, screen nil
> col: 13, screen nil
> col: 7, screen nil
> mode: grep-mode, buffer: #<buffer *grep*>

Grep finished (matches found)
mode: grep-mode, buffer: #<buffer *grep*>
col: 13, screen nil
col: 7, screen nil
col: 13, screen t
col: 7, screen t
mode: grep-mode, buffer: #<buffer *grep*>

-- 
Ari Roponen

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Juri Linkov]

>> mode: grep-mode, buffer: #<buffer *grep*>
>> col: 13, screen nil
>> col: 7, screen nil
>> col: 13, screen nil
>> col: 7, screen nil
>> mode: grep-mode, buffer: #<buffer *grep*>
>
> Grep finished (matches found)
> mode: grep-mode, buffer: #<buffer *grep*>
> col: 13, screen nil
> col: 7, screen nil
> col: 13, screen t
> col: 7, screen t
> mode: grep-mode, buffer: #<buffer *grep*>

Thanks, I can now reproduce this bug.  Its occurrence depends on the
frame's size.  When frame's dimensions are 80x43 with 43 lines or more, so
the first grep hit is visible in the *grep* buffer, then font-lock kicks in,
calculates absolute column positions, after that `compilation-next-error-function'
calculates relative column positions with markers for BOTH grep hits,
where the final position of the second grep hit is correct.

But when frame's dimensions are 80x42 with 42 lines or less, so the first
grep hit is not visible in the *grep* buffer, then fontification doesn't
start, and when you call `next-error', `compilation-next-error-function'
calculates column positions with markers only for the FIRST grep hit,
because the second grep hit is not fontified yet in the *grep* buffer.

The second grep hit gets processes later with `jit-lock-fontify-now'
that calls `compilation-internal-error-properties' and uses
`compilation-move-to-column' to calculate columns.

The problem is that `compilation-internal-error-properties' calculates
column marker positions differently than `compilation-next-error-function'.
Its markers have wrong positions as a result.

`compilation-next-error-function' is always called in the *grep* buffer
where it gets the buffer-local value of `compilation-error-screen-columns'
and keeps its value locally.  After that it visits the source buffer and
correctly calculates column positions using `compilation-move-to-column'.

`compilation-internal-error-properties' tries to do the same,
but uses a different algorithm.  It gets the global value
of `compilation-error-screen-columns' instead of buffer-local
from the *grep* buffer.

Another difference that contributes to the wrong position
is using:
	    (beginning-of-line (if end-line
				   (- line end-line -1)
				 (- loc marker-line -1)))
Contrary to this, `compilation-next-error-function' doesn't
call `beginning-of-line' after `compilation-move-to-column'
moves to the correct column.

Unfortunately, I can't fix this, because I don't understand
why there are two implementations of column position calculations.

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Juri Linkov <juri@jurta.org> writes:

> Thanks, I can now reproduce this bug.

Thank you for looking at this problem.

>
> Unfortunately, I can't fix this, because I don't understand
> why there are two implementations of column position calculations.

No worries. As they say, a known bug is better than an unknown
regression. :-)

-- 
Ari Roponen

bug#9679: 24.0.90; After rgrep, next-error goes to the wrong line

[Ari Roponen]

Ari Roponen <ari.roponen@gmail.com> writes:

> Juri Linkov <juri@jurta.org> writes:
>
>> Thanks, I can now reproduce this bug.
>
> Thank you for looking at this problem.
>
>>
>> Unfortunately, I can't fix this, because I don't understand
>> why there are two implementations of column position calculations.
>
> No worries. As they say, a known bug is better than an unknown
> regression. :-)

Hi,

I can't reproduce this bug any more with todays checkout. It seems that
this commit fixed the bug:

revno: 108138
fixes bug(s): http://debbugs.gnu.org/11382
author: Troels Nielsen <bn.troels@gmail.com>
committer: Chong Yidong <cyd@gnu.org>
branch nick: trunk
timestamp: Sun 2012-05-06 12:52:58 +0800
message:
  Fix match highlighting in compilation buffers.
  
  * progmodes/compile.el (compilation-internal-error-properties):
  Calculate start position correctly when end-col is set but
  end-line is not.

-- 
Ari Roponen

bug#9679: acknowledged by developer (close 9679)

[Ari Roponen]

This bug ("24.0.90; After rgrep, next-error goes to the wrong line") was
fixed in trunk, but it still happens in emacs-24 branch.

Maybe the patch from the following revision could be backported/applied
into the emacs-24 branch.  I just tested it locally, and it indeed fixed
the problem there, too.


revno: 108138
fixes bug(s): http://debbugs.gnu.org/11382
author: Troels Nielsen <bn.troels@gmail.com>
committer: Chong Yidong <cyd@gnu.org>
branch nick: trunk
timestamp: Sun 2012-05-06 12:52:58 +0800
message:
  Fix match highlighting in compilation buffers.
  
  * progmodes/compile.el (compilation-internal-error-properties):
  Calculate start position correctly when end-col is set but
  end-line is not.

-- 
Ari Roponen

bug#9679: acknowledged by developer (close 9679)

[Chong Yidong]

Ari Roponen <ari.roponen@gmail.com> writes:

> This bug ("24.0.90; After rgrep, next-error goes to the wrong line") was
> fixed in trunk, but it still happens in emacs-24 branch.
>
> Maybe the patch from the following revision could be backported/applied
> into the emacs-24 branch.  I just tested it locally, and it indeed fixed
> the problem there, too.

Ah, I didn't realize this was a regression against Emacs 23.  I've
backported the fix the emacs-24 branch; thanks for the heads-up.