bug#32510: xref-find-definitions should return file names, too

bug#32510: xref-find-definitions should return file names, too

[Ludovic Brenta]

Package: emacs
Version: 26.1
Severity: wishlist

Hello,

It would be nice if xref-find-definitions returned files in
addition to language-specific "definitions".  For example:

M-. foo-bar.adb RET

should open the file foo-bar.adb (wherever it is in the
potentially complex directory structure of the project) and
leave point at the beginning of the file.

This is a feature of find-tag but find-tag is now deprecated
in favor of xref-find-definitions; so this feature is missing
and xref-find-definitions is not yet a complete replacement for
find-tag.

Thanks for consideration.

-- 
Ludovic Brenta.

bug#32510: xref-find-definitions should return file names, too

[Lars Ingebrigtsen]

Ludovic Brenta <ludovic@ludovic-brenta.org> writes:

> It would be nice if xref-find-definitions returned files in
> addition to language-specific "definitions".  For example:
>
> M-. foo-bar.adb RET
>
> should open the file foo-bar.adb (wherever it is in the
> potentially complex directory structure of the project) and
> leave point at the beginning of the file.
>
> This is a feature of find-tag but find-tag is now deprecated
> in favor of xref-find-definitions; so this feature is missing
> and xref-find-definitions is not yet a complete replacement for
> find-tag.

Hm...  it seems to me that a command like that seems to belong more in
something like project.el than in xref, which has a different scope.  So
I'm closing this bug report; if somebody else disagrees, please reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#32510: acknowledged by developer (control message for bug #32510)

[Ludovic Brenta]

reopen 32510
thanks

Please to not close this bug so summarily.  Quoting the doc-string of
find-tag:

This function is obsolete since 25.1;
use ‘xref-find-definitions’ instead.

This bug report states that a useful functionality of find-tag is *not*
provided by its official replacement, xref-find-definitions.  This is a
regression.  Just because you think this missing functionality should be
provided elsewhere is not a good reason to close this bug without
providing any solution.

-- 
Ludovic Brenta.

bug#32510: acknowledged by developer (control message for bug #32510)

[Drew Adams]

> reopen 32510
> thanks
> 
> Please [d]o not close this bug so summarily.  Quoting the doc-string of
> find-tag:
> 
> This function is obsolete since 25.1;
> use ‘xref-find-definitions’ instead.
> 
> This bug report states that a useful functionality of find-tag is *not*
> provided by its official replacement, xref-find-definitions.  This is a
> regression.  Just because you think this missing functionality should be
> provided elsewhere is not a good reason to close this bug without
> providing any solution.

+1

And I don't think that `find-tag' should be deprecated
(obsolete).  Its "replacement" is simply a different
command, with some things in common and some things
different.  Both have their uses; each can be preferred
by some users for some things.

(I also don't think that the default key binding of
`find-tag' should have been hijacked for its
"replacement", but that's a different and lesser problem.)

bug#32510: acknowledged by developer (control message for bug #32510)

[Eli Zaretskii]

> From: Ludovic Brenta <ludovic@ludovic-brenta.org>
> Date: Sat, 13 Jul 2019 21:34:26 +0200
> 
> This bug report states that a useful functionality of find-tag is *not*
> provided by its official replacement, xref-find-definitions.  This is a
> regression.  Just because you think this missing functionality should be
> provided elsewhere is not a good reason to close this bug without
> providing any solution.

With the patch below, you should be able to have what you want if you
add tag-partial-file-name-match-p to the list in
etags-xref-find-definitions-tag-order.  Please try this patch and see
if it works for you.

Thanks.

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index 7bf5753..b092c63 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2070,13 +2070,16 @@ etags--xref-find-definitions
               (beginning-of-line)
               (pcase-let* ((tag-info (etags-snarf-tag))
                            (`(,hint ,line . _) tag-info))
-                (unless (eq hint t) ; hint==t if we are in a filename line
+                (unless (and (eq hint t) ; we are in a filename line
+                             (not (eq order-fun
+                                      'tag-partial-file-name-match-p)))
                   (let* ((file (file-of-tag))
                          (mark-key (cons file line)))
                     (unless (gethash mark-key marks)
                       (let ((loc (xref-make-etags-location
                                   tag-info (expand-file-name file))))
-                        (push (xref-make hint loc) xrefs)
+                        (push (xref-make (if (eq hint t) pattern hint) loc)
+                               xrefs)
                         (puthash mark-key t marks)))))))))))
     (nreverse xrefs)))
 

bug#32510: Tags: wontfix -> patch

[Ludovic Brenta]

tags 32510 -wontfix patch
thanks

-- 
Ludovic Brenta.

bug#32510: xref-find-definitions should return file names, too

[Ludovic Brenta]

tags 32510 - wontfix
thanks

I can confirm that the patch by Eli Zaretskii works, with a
difference compared to find-tag: find-tag opens the first
file whose name matches the searched string whereas
xref-find-definitions opens a new buffer with all matches,
forcing the user to use many keystrokes (or worse: reach
for the mouse :)) to choose a match.

I suppose this change of behavior is intentional, consistent
with all other cross-references, and only affects ergonomy;
the patch more importantly restores the functionality that
was previously lost.

-- 
Ludovic Brenta.

bug#32510: xref-find-definitions should return file names, too

[Eli Zaretskii]

> Date: Thu, 18 Jul 2019 16:53:42 +0200
> From: Ludovic Brenta <ludovic@ludovic-brenta.org>
> 
> I can confirm that the patch by Eli Zaretskii works, with a
> difference compared to find-tag: find-tag opens the first
> file whose name matches the searched string whereas
> xref-find-definitions opens a new buffer with all matches,
> forcing the user to use many keystrokes (or worse: reach
> for the mouse :)) to choose a match.

That's not what happened to me after the patch.  For me, M-. just
visited the one file whose name I typed.

Can you show the exact sequence of commands you typed, preferably
using the Emacs sources and corresponding TAGS tables and file names
as the basis, so that I could repeat it here?

> I suppose this change of behavior is intentional, consistent
> with all other cross-references, and only affects ergonomy;
> the patch more importantly restores the functionality that
> was previously lost.

I cannot tell whether it's intentional until I see the behavior you
describe.  What I can say is that if there's only one match, xref goes
there automatically and immediately, but if there are several
candidate matches, xref shows them and allows you to select the one(s)
you want.  The xref behavior is better when the match you want is not
one of the first few, because find-tag required you to continuously
type "C-u M-." in that case, and moreover do that blindly, since you
had no idea how far away is your match.  With xref you can select the
match you are after without iterating through all the previous ones.

However, I would expect the user to type the full file name in this
use case, since that's what this feature is about: finding a file
given its name.  In that case, both commands behave almost
identically.

Dmitry, any comments on the patch?  I admit I didn't study in detail
the role of the PATTERN slot of the object generated by the function
where I proposed to make the change, so perhaps I'm missing some use
case where the patch will not DTRT?

Thanks.

bug#32510: xref-find-definitions should return file names, too

[Ludovic Brenta]

Le 2019-07-18 17:16, Eli Zaretskii a écrit :
> [...] if there's only one match, xref goes
> there automatically and immediately, but if there are several
> candidate matches, xref shows them and allows you to select the one(s)
> you want.

Yes, this is exactly what happens.  We have thousands of source files
in our tree and most have names longer than 20 characters.  Our normal
usage pattern is to use partial matching.  Also your patch uses
tag-partial-file-name-match-p, not tag-full-file-name-match-p, so
it's not surprising that it should do partial matching with possibly
more than one match :)

With etags we were used to using "C-u M-." a couple times too, or
start over with a longer substring of the file name we wanted.

I'm not complaining about this new behavior; it will just take a
little getting used to.  Personally I like the fact that M-g M-n
works with the *xref* buffer like it does in a *compilation* buffer.

-- 
Ludovic Brenta.

bug#32510: xref-find-definitions should return file names, too

[Eli Zaretskii]

> Date: Thu, 18 Jul 2019 17:54:59 +0200
> From: Ludovic Brenta <ludovic@ludovic-brenta.org>
> 
> Yes, this is exactly what happens.  We have thousands of source files
> in our tree and most have names longer than 20 characters.  Our normal
> usage pattern is to use partial matching.  Also your patch uses
> tag-partial-file-name-match-p, not tag-full-file-name-match-p, so
> it's not surprising that it should do partial matching with possibly
> more than one match :)
> 
> With etags we were used to using "C-u M-." a couple times too, or
> start over with a longer substring of the file name we wanted.
> 
> I'm not complaining about this new behavior; it will just take a
> little getting used to.  Personally I like the fact that M-g M-n
> works with the *xref* buffer like it does in a *compilation* buffer.

OK, so I hope Dmitry will approve the change.

Thanks.

bug#32510: xref-find-definitions should return file names, too

[Dmitry Gutov]

[-- Attachment #1: Type: text/html, Size: 2939 bytes --]

bug#32510: xref-find-definitions should return file names, too

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Cc: "32510@debbugs.gnu.org" <32510@debbugs.gnu.org>
> Date: Sat, 20 Jul 2019 01:23:32 +0300
> 
> Sorry, I'm on a vacation in the next several days, and away from my computer, so I can't test it.
> 
> But the idea behind the patch seems sound, and if it works fine for you (in particular, with partial file name
> inputs), it's probably good.

Thanks.  I prefer to wait for you to review the code when you have
time.

bug#32510: acknowledged by developer (control message for bug #32510)

[Dmitry Gutov]

On 14.07.2019 8:21, Eli Zaretskii wrote:
>> From: Ludovic Brenta <ludovic@ludovic-brenta.org>
>> Date: Sat, 13 Jul 2019 21:34:26 +0200
>>
>> This bug report states that a useful functionality of find-tag is *not*
>> provided by its official replacement, xref-find-definitions.  This is a
>> regression.  Just because you think this missing functionality should be
>> provided elsewhere is not a good reason to close this bug without
>> providing any solution.
> 
> With the patch below, you should be able to have what you want if you
> add tag-partial-file-name-match-p to the list in

Finally got around to reviewing it...

> diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
> index 7bf5753..b092c63 100644
> --- a/lisp/progmodes/etags.el
> +++ b/lisp/progmodes/etags.el
> @@ -2070,13 +2070,16 @@ etags--xref-find-definitions
>                 (beginning-of-line)
>                 (pcase-let* ((tag-info (etags-snarf-tag))
>                              (`(,hint ,line . _) tag-info))
> -                (unless (eq hint t) ; hint==t if we are in a filename line
> +                (unless (and (eq hint t) ; we are in a filename line
> +                             (not (eq order-fun
> +                                      'tag-partial-file-name-match-p)))

First, I was thinking we shouldn't check for the exact order-fun value 
(because others could be used, too) and replace it with something like

                               (save-excursion
                                (forward-line 0)
                                (forward-char -2)
                                (not (looking-at "\f\n")))

But then, I'm not sure why that check is there in the first place (the 
order functions make sure not to match the wrong like). Maybe because 
the code inside couldn't handle hint=t? So if it does now, the (unless 
...) conditional can be removed.

>                     (let* ((file (file-of-tag))
>                            (mark-key (cons file line)))
>                       (unless (gethash mark-key marks)
>                         (let ((loc (xref-make-etags-location
>                                     tag-info (expand-file-name file))))
> -                        (push (xref-make hint loc) xrefs)
> +                        (push (xref-make (if (eq hint t) pattern hint) loc)
> +                               xrefs)

I'm not sure using pattern as a hint works well. How about we say 
something like "(file name match)" instead? Or you could pick a better 
wording. The full proposed patch is below.

I see that it doesn't work exactly perfectly, e.g. moving point within 
the quotes in

   #include "composite.h"

and pressing M-. brings up three matches (composite.c, composite.h and 
composite.el), whereas only one of them is correct, but find-tag 
probably has the same problem anyway. Maybe CC Mode should set up 
find-tag-default-function to return the full file name when inside 
#include statements.

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index 7bf575340e..a052ad2ce5 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2070,14 +2070,15 @@ etags--xref-find-definitions
                (beginning-of-line)
                (pcase-let* ((tag-info (etags-snarf-tag))
                             (`(,hint ,line . _) tag-info))
-                (unless (eq hint t) ; hint==t if we are in a filename line
-                  (let* ((file (file-of-tag))
-                         (mark-key (cons file line)))
-                    (unless (gethash mark-key marks)
-                      (let ((loc (xref-make-etags-location
-                                  tag-info (expand-file-name file))))
-                        (push (xref-make hint loc) xrefs)
-                        (puthash mark-key t marks)))))))))))
+                (let* ((file (file-of-tag))
+                       (mark-key (cons file line)))
+                  (unless (gethash mark-key marks)
+                    (let ((loc (xref-make-etags-location
+                                tag-info (expand-file-name file))))
+                      (push (xref-make (if (eq hint t) "(filename 
match)" hint)
+                                       loc)
+                            xrefs)
+                      (puthash mark-key t marks))))))))))
      (nreverse xrefs)))

  (defclass xref-etags-location (xref-location)

bug#32510: acknowledged by developer (control message for bug #32510)

[Dmitry Gutov]

On 30.07.2019 3:06, Dmitry Gutov wrote:
> The full proposed patch is below.

I've pushed that change now to master. Please try it out.

bug#32510: acknowledged by developer (control message for bug #32510)

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Cc: 32510@debbugs.gnu.org
> Date: Tue, 30 Jul 2019 17:00:28 +0300
> 
> On 30.07.2019 3:06, Dmitry Gutov wrote:
> > The full proposed patch is below.
> 
> I've pushed that change now to master. Please try it out.

Thanks, it LGTM, so I'm closing the bug.