bug#36967: 27.0.50; Duplicate lines in xref output

bug#36967: 27.0.50; Duplicate lines in xref output

[Juri Linkov]

I tried to use project-find-regexp more often than rgrep,
but unfortunately xref still has a fundamental flaw:

0. emacs -Q
1. M-x project-find-regexp RET regexp RET
2. The output buffer *xref* contains duplicate lines
   when regexp is found on the same line several times,
   each duplicate output line has separate highlighting
   for every regexp occurrence.

bug#36967: 27.0.50; Duplicate lines in xref output

[Dmitry Gutov]

On 08.08.2019 0:52, Juri Linkov wrote:
> I tried to use project-find-regexp more often than rgrep,
> but unfortunately xref still has a fundamental flaw:
> 
> 0. emacs -Q
> 1. M-x project-find-regexp RET regexp RET
> 2. The output buffer *xref* contains duplicate lines
>     when regexp is found on the same line several times,
>     each duplicate output line has separate highlighting
>     for every regexp occurrence.

I don't know how "fundamental" it is, but indeed, it's somewhat of a 
drawback. Suggestions for improving it (API change and/or implementation 
change) are welcome.

bug#36967: 27.0.50; Duplicate lines in xref output

[Juri Linkov]

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

>> I tried to use project-find-regexp more often than rgrep,
>> but unfortunately xref still has a fundamental flaw:
>> 0. emacs -Q
>> 1. M-x project-find-regexp RET regexp RET
>> 2. The output buffer *xref* contains duplicate lines
>>     when regexp is found on the same line several times,
>>     each duplicate output line has separate highlighting
>>     for every regexp occurrence.
>
> I don't know how "fundamental" it is, but indeed, it's somewhat of
> a drawback. Suggestions for improving it (API change and/or implementation
> change) are welcome.

Here is the patch that makes the broken project-find-regexp usable:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xref-usable.patch --]
[-- Type: text/x-diff, Size: 1522 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 3b19debb79..1f5e45f20d 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1461,11 +1461,10 @@ xref--collect-matches
                                  syntax-needed)))))
 
 (defun xref--collect-matches-1 (regexp file line line-beg line-end syntax-needed)
-  (let (matches)
+  (let ((summary (buffer-substring line-beg line-end))
+        matches)
     (when syntax-needed
       (syntax-propertize line-end))
-    ;; FIXME: This results in several lines with the same
-    ;; summary. Solve with composite pattern?
     (while (and
             ;; REGEXP might match an empty string.  Or line.
             (or (null matches)
@@ -1473,12 +1472,12 @@ xref--collect-matches-1
             (re-search-forward regexp line-end t))
       (let* ((beg-column (- (match-beginning 0) line-beg))
              (end-column (- (match-end 0) line-beg))
-             (loc (xref-make-file-location file line beg-column))
-             (summary (buffer-substring line-beg line-end)))
+             (loc (xref-make-file-location file line beg-column)))
         (add-face-text-property beg-column end-column 'xref-match
                                 t summary)
-        (push (xref-make-match summary loc (- end-column beg-column))
-              matches)))
+        (unless matches
+          (push (xref-make-match summary loc (- end-column beg-column))
+                matches))))
     (nreverse matches)))
 
 (defun xref--find-file-buffer (file)

bug#36967: 27.0.50; Duplicate lines in xref output

[Dmitry Gutov]

On 02.12.2020 23:30, Juri Linkov wrote:
>>> I tried to use project-find-regexp more often than rgrep,
>>> but unfortunately xref still has a fundamental flaw:
>>> 0. emacs -Q
>>> 1. M-x project-find-regexp RET regexp RET
>>> 2. The output buffer *xref* contains duplicate lines
>>>      when regexp is found on the same line several times,
>>>      each duplicate output line has separate highlighting
>>>      for every regexp occurrence.
>>
>> I don't know how "fundamental" it is, but indeed, it's somewhat of
>> a drawback. Suggestions for improving it (API change and/or implementation
>> change) are welcome.
> 
> Here is the patch that makes the broken

Pretty harsh there.

> project-find-regexp usable:

Okay, impressions:

When the line has two matches, the new code only collects the first 
match. So 'matches' is always an list with one element (or nil).

Upside: repetitions are not shown anymore, but the match highlighting is 
still applied.

Downside: xref-query-replace-in-results won't work in those cases 
anymore; it will only replace one match. Because the list only contains 
one location, and not all of them. And that command is pretty nice to have.

Here's an alternative proposal:

Combine the lines inside the rendering code instead.

So each xref will have a separate location, but then xref--insert-xrefs 
will see that xref-location-line value repeats across some consecutive 
locations, and will combine them into single line with some text 
property magic (basically, copying the summary from one of them, and 
then applying 'xref-item and 'face properties appropriately). This 
retains the xref item semantics (as opposed to, say, associating an xref 
item with multiple locations). And _hopefully_ the replace-related code 
won't need any changes.

As a bonus, 'n' and 'p' should then automatically change behavior to 
jump between locations when they are on the same line.

bug#36967: 27.0.50; Duplicate lines in xref output

[Juri Linkov]

>> Here is the patch that makes the broken
>
> Pretty harsh there.

But true: I can't use it in the current form, and was waiting
when someone will fix it.

>> project-find-regexp usable:
>
> Downside: xref-query-replace-in-results won't work in those cases anymore;
> it will only replace one match. Because the list only contains one
> location, and not all of them. And that command is pretty nice to have.

Sorry, I missed this use case because I still know too little about details
of xref.el.

> Here's an alternative proposal:
>
> Combine the lines inside the rendering code instead.
>
> So each xref will have a separate location, but then xref--insert-xrefs
> will see that xref-location-line value repeats across some consecutive
> locations, and will combine them into single line with some text property
> magic (basically, copying the summary from one of them, and then applying
> 'xref-item and 'face properties appropriately). This retains the xref item
> semantics (as opposed to, say, associating an xref item with multiple
> locations). And _hopefully_ the replace-related code won't need
> any changes.

I tried to improve xref--insert-xrefs to group matches by lines
by using the most convenient function seq-group-by.  But then
noticed that xref.el doesn't rely on seq.el.  Even xref--alistify
that groups matches by files could be replaced by seq-group-by.
Is it a requirement to avoid using seq functions in xref.el?

bug#36967: 27.0.50; Duplicate lines in xref output

[Dmitry Gutov]

On 03.12.2020 23:30, Juri Linkov wrote:
>>> Here is the patch that makes the broken
>>
>> Pretty harsh there.
> 
> But true: I can't use it in the current form, and was waiting
> when someone will fix it.

It's great to see you get this ball rolling.

>> Here's an alternative proposal:
>>
>> Combine the lines inside the rendering code instead.
>>
>> So each xref will have a separate location, but then xref--insert-xrefs
>> will see that xref-location-line value repeats across some consecutive
>> locations, and will combine them into single line with some text property
>> magic (basically, copying the summary from one of them, and then applying
>> 'xref-item and 'face properties appropriately). This retains the xref item
>> semantics (as opposed to, say, associating an xref item with multiple
>> locations). And _hopefully_ the replace-related code won't need
>> any changes.
> 
> I tried to improve xref--insert-xrefs to group matches by lines
> by using the most convenient function seq-group-by.  But then
> noticed that xref.el doesn't rely on seq.el.  Even xref--alistify
> that groups matches by files could be replaced by seq-group-by.
> Is it a requirement to avoid using seq functions in xref.el?

Not really, no. project.el pulls in seq already, why not have xref do it 
too.

I'm _slightly_ worried about extra garbage if we do seq-group-by twice 
(with an extra list for every line, even those that don't need it), but 
that's what benchmarking is for (can do that later).

bug#36967: 27.0.50; Duplicate lines in xref output

[Juri Linkov]

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

> I'm _slightly_ worried about extra garbage if we do seq-group-by twice
> (with an extra list for every line, even those that don't need it), but
> that's what benchmarking is for (can do that later).

I'm worried about extra garbage too, so this patch doesn't cause extra lists.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multiline-xref-insert-xrefs.patch --]
[-- Type: text/x-diff, Size: 2217 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 9f5fc57142..260623180e 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -832,17 +832,28 @@ xref--insert-xrefs
                                (length (and line (format "%d" line)))))
            for line-format = (and max-line-width
                                   (format "%%%dd: " max-line-width))
+           with prev-line-key = nil
            do
            (xref--insert-propertized '(face xref-file-header xref-group t)
                                      group "\n")
            (cl-loop for (xref . more2) on xrefs do
                     (with-slots (summary location) xref
                       (let* ((line (xref-location-line location))
+                             (line-key (list (xref-location-group location) line))
                              (prefix
                               (if line
                                   (propertize (format line-format line)
                                               'face 'xref-line-number)
                                 "  ")))
+                        (when (equal prev-line-key line-key)
+                          (let ((column (xref-file-location-column location)))
+                            (delete-region
+                             (save-excursion
+                               (forward-line -1)
+                               (move-to-column (+ (length prefix) column))
+                               (point))
+                             (point))
+                            (setq summary (substring summary column) prefix "")))
                         (xref--insert-propertized
                          (list 'xref-item xref
                                'mouse-face 'highlight
@@ -850,7 +861,8 @@ xref--insert-xrefs
                                'help-echo
                                (concat "mouse-2: display in another window, "
                                        "RET or mouse-1: follow reference"))
-                         prefix summary)))
+                         prefix summary)
+                        (setq prev-line-key line-key)))
                     (insert "\n"))))
 
 (defun xref--analyze (xrefs)

[-- Attachment #3: Type: text/plain, Size: 220 bytes --]


However, there is a problem: when xref--insert-xrefs is called by
'M-.' that uses etags, then it signals an error:

  cl-no-applicable-method xref-file-location-column

And I don't know how to fix etags to add columns.

bug#36967: 27.0.50; Duplicate lines in xref output

[Dmitry Gutov]

On 06.12.2020 22:31, Juri Linkov wrote:
>> I'm _slightly_ worried about extra garbage if we do seq-group-by twice
>> (with an extra list for every line, even those that don't need it), but
>> that's what benchmarking is for (can do that later).
> 
> I'm worried about extra garbage too, so this patch doesn't cause extra lists.

Thanks! Seems to work well. Especially with this addition:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 9f5fc57142..9ad956d496 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -595,7 +595,8 @@ xref-prev-line

  (defun xref--item-at-point ()
    (save-excursion
-    (back-to-indentation)
+    (when (eolp)
+      (forward-char -1))
      (get-text-property (point) 'xref-item)))

  (defun xref-goto-xref (&optional quit)

> However, there is a problem: when xref--insert-xrefs is called by
> 'M-.' that uses etags, then it signals an error:

Does it happen when two symbols do indeed get defined on the same line? 
Sounds like a rare situation.

>    cl-no-applicable-method xref-file-location-column
> 
> And I don't know how to fix etags to add columns.

Using xref-file-location-column there kind of breaks the abstraction. I 
was thinking more along the lines of text-prop search for the column 
inside the summary string:

(setq column
       (with-temp-buffer
         (insert summary)
         (goto-char (point-min))
         (let ((match
                (text-property-search-forward
                 'face 'xref-match
                 (lambda (target value)
                   (or (eq target value)
                       (and (listp value)
                            (member target value)))))))
           (and match
                (1- (prop-match-beginning match))))))

That's quite a bit longer, though. And it still will work only with some 
kinds of xrefs: I'm not sure I know how to add columns to etags either. 
Not to mention third-party xref backends.

The rest won't get deduplication, but that's probably all right.

bug#36967: 27.0.50; Duplicate lines in xref output

[Dmitry Gutov]

On 09.12.2020 05:53, Dmitry Gutov wrote:
> Using xref-file-location-column there kind of breaks the abstraction. I 
> was thinking more along the lines of text-prop search for the column 
> inside the summary string:

Although we'll probably make it an optional method, just like 
xref-location-line is already.

Then we won't have to do all that gymnastics with the temp buffer.

bug#36967: 27.0.50; Duplicate lines in xref output

[Dmitry Gutov]

Version: 28.1

On 09.12.2020 15:57, Dmitry Gutov wrote:
> On 09.12.2020 05:53, Dmitry Gutov wrote:
>> Using xref-file-location-column there kind of breaks the abstraction. 
>> I was thinking more along the lines of text-prop search for the column 
>> inside the summary string:
> 
> Although we'll probably make it an optional method, just like 
> xref-location-line is already.

And now done.

I've pushed the slightly tweaked Juri's patch and the change described 
above.

With that, the bug should be fixed now. Please let me know if something 
is still amiss.

Thanks all!