bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Hi maintainers,

Observed this when working on Eglot, an Emacs LSP client.  Some LSP
servers, when asked about symbol definitions, make the mistake of
sending an non-existent or outdated path and location.

On the other hand, because xrefs come in potentially large numbers, a
sane xref backend will not check if a file exists before making
xref-file-location objects (neither should it check if the file contains
the purported location of the definition).  This can only reasonably be
done when the user decides to visit such a location.

So, currently, if the user chooses to visit a xref-file-location
pointing to a non-existing file, that file buffer is created
automatically and the command will probably error with an cryptic "End
of buffer" because of the `forward-char' call in the corresponding
xref-location-marker method.  The new empty buffer isn't even shown
which would at least help the user recognize the problem.

So:

1. xref-location-marker should check for `file-readable-p' before trying
to open the file, and if that fails issues an error ("File %s can't be
found.").

2. if the file is found, xref-location-marker should detect if the
location is indeed available in the file, and if it isn't, issue a
message.  In that case it should return a marker to the nearest possible
location.

3. Number 2. could turn out to be brittle and annoying if we have
changed the file in the meantime (but probably not more so than jumping
to a wrong location).  So we could have a "hint" field in
xref-file-location (or a xref-hinted-file-location) that helps in
looking around the landing point for, say, a regexp, and puts point
there.  Historically, this technique is successfully used in SLIME.  We
could also reasonably default that field to the identifier being looked
for.

The attached patch fixes 1. and 2.  It should probably go into emacs-26

Then, I'd like to know your opinions on 3., to go into master.

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index b0bdd62ae9..d38328cccd 100644ppp
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -119,13 +119,19 @@ xref-make-file-location
     (with-current-buffer
         (or (get-file-buffer file)
             (let ((find-file-suppress-same-file-warnings t))
+              (unless (file-exists-p file)
+                (error "File %s doesn't exist!" file))
               (find-file-noselect file)))
       (save-restriction
         (widen)
         (save-excursion
           (goto-char (point-min))
           (beginning-of-line line)
-          (forward-char column)
+          (ignore-errors (forward-char column))
+          (unless (and (= (1+ (current-line)) line)
+                       (= (current-column) column))
+            (message "Intended xref location was line=%d, column=%d"
+                     line column))
           (point-marker))))))
 
 (cl-defmethod xref-location-group ((l xref-file-location))

João

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Robert Pluim]

joaotavora@gmail.com (João Távora) writes:

>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index b0bdd62ae9..d38328cccd 100644ppp
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -119,13 +119,19 @@ xref-make-file-location
>      (with-current-buffer
>          (or (get-file-buffer file)
>              (let ((find-file-suppress-same-file-warnings t))
> +              (unless (file-exists-p file)
> +                (error "File %s doesn't exist!" file))
>                (find-file-noselect file)))
>        (save-restriction
>          (widen)
>          (save-excursion
>            (goto-char (point-min))
>            (beginning-of-line line)
> -          (forward-char column)
> +          (ignore-errors (forward-char column))
> +          (unless (and (= (1+ (current-line)) line)
> +                       (= (current-column) column))
> +            (message "Intended xref location was line=%d, column=%d"
> +                     line column))
>            (point-marker))))))

I think the message could be clearer, the current one doesnʼt express
that something unexpected happened. How about

"Xref intended location line=%d, column=%d is out of range"

or similar.

Regards

Robert

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Robert Pluim <rpluim@gmail.com> writes:

>> +            (message "Intended xref location was line=%d, column=%d"
>> +                     line column))
>>            (point-marker))))))
>
> I think the message could be clearer, the current one doesnʼt express
> that something unexpected happened. How about
>
> "Xref intended location line=%d, column=%d is out of range"
>
> or similar.

OK, sounds reasonable.

João

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Date: Mon, 02 Jul 2018 14:46:52 +0100
> Cc: dgutov@yandex.ru
> 
> The attached patch fixes 1. and 2.  It should probably go into emacs-26
> 
> Then, I'd like to know your opinions on 3., to go into master.
> 
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index b0bdd62ae9..d38328cccd 100644ppp
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -119,13 +119,19 @@ xref-make-file-location
>      (with-current-buffer
>          (or (get-file-buffer file)
>              (let ((find-file-suppress-same-file-warnings t))
> +              (unless (file-exists-p file)
> +                (error "File %s doesn't exist!" file))
>                (find-file-noselect file)))
>        (save-restriction
>          (widen)
>          (save-excursion
>            (goto-char (point-min))
>            (beginning-of-line line)
> -          (forward-char column)
> +          (ignore-errors (forward-char column))
> +          (unless (and (= (1+ (current-line)) line)
> +                       (= (current-column) column))
> +            (message "Intended xref location was line=%d, column=%d"
> +                     line column))
>            (point-marker))))))

What will the last hunk do when a file changed, but the identifier was
still found?  AFAIR, the etags back-end is capable of doing that
(because it searches the file in the vicinity of the line/column if
not found at the exact location), and it's a valuable feature, for
obvious reasons.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>>            (beginning-of-line line)
>> -          (forward-char column)
>> +          (ignore-errors (forward-char column))
>> +          (unless (and (= (1+ (current-line)) line)
>> +                       (= (current-column) column))
>> +            (message "Intended xref location was line=%d, column=%d"
>> +                     line column))
>>            (point-marker))))))
>
> What will the last hunk do when a file changed, but the identifier was
> still found?

The function is completely oblivious to the identifier being found or
not.  So it would do what it does now: jump to the wrong location
(unless you're lucky and the change preserved the location of the
definition you're after).

> AFAIR, the etags back-end is capable of doing that
> (because it searches the file in the vicinity of the line/column if
> not found at the exact location), and it's a valuable feature, for
> obvious reasons.

IIUC, that's what I'm proposing in my point 3: add a "hint" field to
xref-file-location and default that hint to the identifier being looked
for.  If etags has code for that already, then great, we could try to
share it, but that would be for master right?  Or can 1, 2 and 3 go into
emacs-26?

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: 32034@debbugs.gnu.org,  dgutov@yandex.ru
> Date: Mon, 02 Jul 2018 16:28:53 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>            (beginning-of-line line)
> >> -          (forward-char column)
> >> +          (ignore-errors (forward-char column))
> >> +          (unless (and (= (1+ (current-line)) line)
> >> +                       (= (current-column) column))
> >> +            (message "Intended xref location was line=%d, column=%d"
> >> +                     line column))
> >>            (point-marker))))))
> >
> > What will the last hunk do when a file changed, but the identifier was
> > still found?
> 
> The function is completely oblivious to the identifier being found or
> not.  So it would do what it does now: jump to the wrong location

And display the message, right?  If so, the message would be an
annoyance if displayed unconditionally, because at least the etags
back-end can cope with these calamities, and users rely on that (it
lets one re-generate TAGS only once in a blue moon).

Or maybe I misunderstand the context in which this code runs, in
which case ignore me.

> > AFAIR, the etags back-end is capable of doing that
> > (because it searches the file in the vicinity of the line/column if
> > not found at the exact location), and it's a valuable feature, for
> > obvious reasons.
> 
> IIUC, that's what I'm proposing in my point 3: add a "hint" field to
> xref-file-location and default that hint to the identifier being looked
> for.  If etags has code for that already, then great, we could try to
> share it

I don't think you can share that code, because it relies on the
contents of the TAGS file, where the 'etags' utility records not only
the position of the identifier, but also a string to search for it.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joaotavora@gmail.com (João Távora)
>> Cc: 32034@debbugs.gnu.org,  dgutov@yandex.ru
>> Date: Mon, 02 Jul 2018 16:28:53 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >>            (beginning-of-line line)
>> >> -          (forward-char column)
>> >> +          (ignore-errors (forward-char column))
>> >> +          (unless (and (= (1+ (current-line)) line)
>> >> +                       (= (current-column) column))
>> >> +            (message "Intended xref location was line=%d, column=%d"
>> >> +                     line column))
>> >>            (point-marker))))))
>> >
>> > What will the last hunk do when a file changed, but the identifier was
>> > still found?
>> 
>> The function is completely oblivious to the identifier being found or
>> not.  So it would do what it does now: jump to the wrong location
>
> And display the message, right?

Not necessarily, if that line and column still exists.

> If so, the message would be an annoyance if displayed unconditionally,
> because at least the etags back-end can cope with these calamities,
> and users rely on that (it lets one re-generate TAGS only once in a
> blue moon).

That's what I wrote in the original message.  I added it under the
assumption that it would be less of an annoyance than silently jumping
to the wrong spot int he file.  The feature you want to add to the
relevant xref-location-marker method makes a lot of sense, but it's a
new feature nevertheless.

> Or maybe I misunderstand the context in which this code runs, in
> which case ignore me.

No, I think your understanding is fine.  The current code is really
quite dumb.  The reason you don't come across it often is that the elisp
and etags backend use their own "location" class, xref-etags-location
and xref-elisp-location.  But eglot uses the xref-file-location class
bundled with xref (it could also use its own, but then what's the point
of having a built-in class for this?  perhaps none, in this case I move
to delete it)

>> > AFAIR, the etags back-end is capable of doing that
>> > (because it searches the file in the vicinity of the line/column if
>> > not found at the exact location), and it's a valuable feature, for
>> > obvious reasons.
>> 
>> IIUC, that's what I'm proposing in my point 3: add a "hint" field to
>> xref-file-location and default that hint to the identifier being looked
>> for.  If etags has code for that already, then great, we could try to
>> share it
>
> I don't think you can share that code, because it relies on the
> contents of the TAGS file, where the 'etags' utility records not only
> the position of the identifier, but also a string to search for it.

I meant the code that searches for the string then, if it isn't too
trivial.

When you can, please comment on the possibility of fixing this in
emacs-26 or master.

João

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: 32034@debbugs.gnu.org,  dgutov@yandex.ru
> Date: Mon, 02 Jul 2018 16:50:27 +0100
> 
> > And display the message, right?
> 
> Not necessarily, if that line and column still exists.

I was explicitly thinking about the case where they don't, for some
reason, or that the line is there, but the column isn't (e.g., due to
some TABs).

> > If so, the message would be an annoyance if displayed unconditionally,
> > because at least the etags back-end can cope with these calamities,
> > and users rely on that (it lets one re-generate TAGS only once in a
> > blue moon).
> 
> That's what I wrote in the original message.  I added it under the
> assumption that it would be less of an annoyance than silently jumping
> to the wrong spot int he file.

If the back-end can do its job even under these conditions, then the
message would look like a regression.

> No, I think your understanding is fine.  The current code is really
> quite dumb.  The reason you don't come across it often is that the elisp
> and etags backend use their own "location" class, xref-etags-location
> and xref-elisp-location.  But eglot uses the xref-file-location class
> bundled with xref (it could also use its own, but then what's the point
> of having a built-in class for this?  perhaps none, in this case I move
> to delete it)

So neither etags nor elisp back-ends will ever go through this code,
and will never show this message?  If so, maybe your patch is fine as
it is.  Otherwise, maybe we should exempt those two back-ends from
displaying the message?

> When you can, please comment on the possibility of fixing this in
> emacs-26 or master.

In case it wasn't clear, what I wrote _was_ my comment on that.  The
code is a no-brainer, so the only aspect I worry about is whether it
could introduce annoying messages where none are needed.  Other than
that, I have no objections with having this on emacs-26, but I'd like
to give Dmitry time to comment.

Thanks.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joaotavora@gmail.com (João Távora)
>> Cc: 32034@debbugs.gnu.org,  dgutov@yandex.ru
>> Date: Mon, 02 Jul 2018 16:50:27 +0100
>> 
>> > And display the message, right?
>> Not necessarily, if that line and column still exists.
> I was explicitly thinking about the case where they don't, for some
> reason, or that the line is there, but the column isn't (e.g., due to
> some TABs).

Yeah, but in those cases I want the message to display.  The definition
isn't there probably, so it's nice to display the message.

>> > If so, the message would be an annoyance if displayed unconditionally,
>> > because at least the etags back-end can cope with these calamities,
>> > and users rely on that (it lets one re-generate TAGS only once in a
>> > blue moon).
>> That's what I wrote in the original message.  I added it under the
>> assumption that it would be less of an annoyance than silently jumping
>> to the wrong spot int he file.
> If the back-end can do its job even under these conditions, then the
> message would look like a regression.

Well it can't, so it won't :-)

>> No, I think your understanding is fine.  The current code is really
>> quite dumb.  The reason you don't come across it often is that the elisp
>> and etags backend use their own "location" class, xref-etags-location
>> and xref-elisp-location.  But eglot uses the xref-file-location class
>> bundled with xref (it could also use its own, but then what's the point
>> of having a built-in class for this?  perhaps none, in this case I move
>> to delete it)
>
> So neither etags nor elisp back-ends will ever go through this code,
> and will never show this message?  If so, maybe your patch is fine as
> it is.  Otherwise, maybe we should exempt those two back-ends from
> displaying the message?

The first applies.  What's more, noone in Emacs or in the ELPA repo uses
xref-file-location, directly or through inheritance.  

>
>> When you can, please comment on the possibility of fixing this in
>> emacs-26 or master.
>
> In case it wasn't clear, what I wrote _was_ my comment on that.  The
> code is a no-brainer, so the only aspect I worry about is whether it
> could introduce annoying messages where none are needed.  Other than
> that, I have no objections with having this on emacs-26, but I'd like
> to give Dmitry time to comment.

OK, let's give Dmitry time.  But, without trying to annoy you :-) there 
are 3 parts to this:

1. bug fix for when file doesn't exist (should error instead of making it)
2. bug fix for when file exists, but not location (it currently errors)
3. new feature, for robustness, search for id around landing point.

1 and 2 were in my patch.  But now I'm assuming you want 1, 2 and 3 in
emacs-26, in which case I'll try to base the id-searching bit from etags.

João

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: 32034@debbugs.gnu.org,  dgutov@yandex.ru
> Date: Mon, 02 Jul 2018 17:55:09 +0100
> 
> OK, let's give Dmitry time.  But, without trying to annoy you :-) there 
> are 3 parts to this:
> 
> 1. bug fix for when file doesn't exist (should error instead of making it)
> 2. bug fix for when file exists, but not location (it currently errors)
> 3. new feature, for robustness, search for id around landing point.
> 
> 1 and 2 were in my patch.  But now I'm assuming you want 1, 2 and 3 in
> emacs-26, in which case I'll try to base the id-searching bit from etags.

No.  I'm okay with your solution for 1 to go to emacs-26.  I had
reservations about your solution for 2, but now I understand it won't
affect the existing back-ends, so I'm okay with it on emacs-26, too
(but will probably suggest a clearer message text if Dmitry agrees
with the solution).

3 should indeed go to master.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> No.  I'm okay with your solution for 1 to go to emacs-26.  I had
> reservations about your solution for 2, but now I understand it won't
> affect the existing back-ends, so I'm okay with it on emacs-26, too
> (but will probably suggest a clearer message text if Dmitry agrees
> with the solution).
>
> 3 should indeed go to master.

This was over two years ago, and there was some followup discussion
between Dmitry and João, but I think the consensus here was that the
patches fixes a real problem, ans should probably be applied?  As far as
I can see, that never happened.

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

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Dmitry Gutov]

On 10.08.2020 15:59, Lars Ingebrigtsen wrote:
> This was over two years ago, and there was some followup discussion
> between Dmitry and João, but I think the consensus here was that the
> patches fixes a real problem, ans should probably be applied?  As far as
> I can see, that never happened.

Nope, that wasn't the consensus.

The proposed patch had problems, and we haven't reached any next version.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

On 10.08.2020 15:59, Lars Ingebrigtsen wrote:

Hi Lars, thanks for pinging me.

> This was over two years ago, and there was some followup discussion
> between Dmitry and João, but I think the consensus here was that the
> patches fixes a real problem, ans should probably be applied?  As far as
> I can see, that never happened.

I read the discussion. As far as I know the problem reported originally
persists.

A small patch does exist, has two parts: 1 and 2.  Part 2 apparently had
some real problem that I don't understand anymore, not sure if it still
applies.  I seem to have said it was easy to fix, though it wouldn't
much matter if we could devise a much better fix, later known as number
3.

The rest of the discussion consists of hypothetical problems (about
seemingly hypothetical backends), leveled against that nr. 3 patch,
which seems never to have seen the light of day: I made the mistake of
tagging along with the hypotheticals and then probably ran out of time.

If I recall correctly, that better patch, were it to exist, would do
what is done by SLIME, the thing xref.el was based on when it was
originally written by Helmut Eller.  SLIME has done it for a very long
time and quite successfully: the objects that represent cross-references
contain optional extra bits of information -- hints -- which help,
heuristically, when locating the thing being looked for.

My fork of SLIME, SLY, hasn't yet switched to xref.el partly because of
xref.el's limitations in this regard.  Eglot would also benefit (the
original report concerns Eglot).

Alas, that nr 3 patch is vapourware.  Maybe I or someone will make it in
the future, and then we can talk about real stuff. I think we should
look to SLIME (and maybe Helmut?) for inspiration in fixing this, but I
don't have time to do this right now.

The original thread where xref.el is originally discussed is relevant
here, for posterity:

https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg01253.html

João

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Dmitry Gutov]

Hi guys,

I've read up on the discussion.

On 7/2/18 4:46 PM, João Távora wrote:

> 1. xref-location-marker should check for `file-readable-p' before trying
> to open the file, and if that fails issues an error ("File %s can't be
> found.").

I'm fine with this, naturally.

> 2. if the file is found, xref-location-marker should detect if the
> location is indeed available in the file, and if it isn't, issue a
> message.  In that case it should return a marker to the nearest possible
> location.

When xref-location-marker is inaccurate, it may lead to problems, like 
xref-query-replace-in-results sometimes performing replacements at the 
"auto-corrected", wrong positions.

Maybe we can add a laxer version of this function that is only used when 
we know the user will be looking at the result directly (e.g. from 
xref-find-definitions, but not from xref-query-replace-in-results). I'm 
on the fence about xref-fined-references regarding this, because it also 
supports automatic replacement.

> 3. Number 2. could turn out to be brittle and annoying if we have
> changed the file in the meantime (but probably not more so than jumping
> to a wrong location).  So we could have a "hint" field in
> xref-file-location (or a xref-hinted-file-location) that helps in
> looking around the landing point for, say, a regexp, and puts point
> there.  Historically, this technique is successfully used in SLIME.  We
> could also reasonably default that field to the identifier being looked
> for.

I'm not sure this is a good idea. Certainly not the "defaulting to the 
identifier" bit. Because the identifier could e.g. look like 
namespace-name/symbol-name, where only "symbol-name" appears verbatim in 
the definition. I don't have much experience with LSP, but I imagine 
this could happen there, too (unless it only supports navigation to 
unqualified identifiers).

Now, if hint is optional (and disabled by default), and extracting the 
relevant code from Etags is natural, I say go for it.

But overall, I think individual backends that want "smarter" behavior 
should create their own "location" class, like Elisp does.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

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

On Tue, Jul 3, 2018, 14:56 Dmitry Gutov <dgutov@yandex.ru> wrote:

>
>
> When xref-location-marker is inaccurate, it may lead to problems, like
> xref-query-replace-in-results sometimes performing replacements at the
> "auto-corrected", wrong positions.
>
> Maybe we can add a laxer version of this function that is only used when
> we know the user will be looking at the result directly (e.g. from
> xref-find-definitions, but not from xref-query-replace-in-results). I'm
> on the fence about xref-fined-references regarding this, because it also
> supports automatic replacement.
>

Right, this should go into xref-goto-xref (or whatever it is called) or
xref-find-definitions.  Or scratched, if it's too much work, because it's
not terribly useful.

>
>
> > 3. Number 2. could turn out to be brittle and annoying if we have
> > changed the file in the meantime (but probably not more so than jumping
> > to a wrong location).  So we could have a "hint" field in
> > xref-file-location (or a xref-hinted-file-location) that helps in
> > looking around the landing point for, say, a regexp, and puts point
> > there.  Historically, this technique is successfully used in SLIME.  We
> > could also reasonably default that field to the identifier being looked
> > for.
>
> I'm not sure this is a good idea. Certainly not the "defaulting to the
> identifier" bit. Because the identifier could e.g. look like
> namespace-name/symbol-name, where only "symbol-name" appears verbatim in
> the definition.



In that special case we will do no worse than the current version, wouldn't
we? So still a win on my book. And to do better for languages which do
present this problem would take very little work, especially if we take
advantage of inheritance and eieio slot methods.


I don't have much experience with LSP, but I imagine
> this could happen there, too (unless it only supports navigation to
> unqualified identifiers).
>
> Now, if hint is optional (and disabled by default), and extracting the
> relevant code from Etags is natural, I say go for it.
>
> But overall, I think individual backends that want "smarter" behavior
> should create their own "location" class, like Elisp does.
>

At this point, I'm thinking of dismissing the whole thing, and voting to
deprecate xref-file-location entirely. Nobody uses it and Eglot will
probably use something else before this issue is solved.  It's a shame we
can't share code, but if we can't give a default class any kind of useful
behavior, we might as well not have this class in the first place.

>

[-- Attachment #2: Type: text/html, Size: 3876 bytes --]

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Dmitry Gutov]

On 7/3/18 5:50 PM, João Távora wrote:

> Right, this should go into xref-goto-xref (or whatever it is called) or 
> xref-find-definitions.  Or scratched, if it's too much work, because 
> it's not terribly useful.

I'm sure it will be useful in some situations, I'm just not sure about 
the odds.

> In that special case we will do no worse than the current version, 
> wouldn't we?

We would.

Say, we're looking for clojure.core/cons, and the marker leads us to 
"(defn cons" (the example is largely made up). The code looks for 
clojure.core/cons, doesn't find it, and signals an error (will it?).

Or worse, searches around and finds a verbatim reference to 
clojure.core/cons in a docstring somewhere in that file, and returns 
*that* location.

> At this point, I'm thinking of dismissing the whole thing, and voting to 
> deprecate xref-file-location entirely. Nobody uses it and Eglot will 
> probably use something else before this issue is solved. 

Err, it is used: in xref--collect-matches-1. And through it, in 
xref-collect-matches and xref-collect-references.

> It's a shame 
> we can't share code, but if we can't give a default class any kind of 
> useful behavior, we might as well not have this class in the first place.

We could. As long as we don't default to the identifier, and the backend 
has to explicitly provide the hint, we could be fine.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

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

On Tue, Jul 3, 2018, 16:07 Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 7/3/18 5:50 PM, João Távora wrote:
>
> > Right, this should go into xref-goto-xref (or whatever it is called) or
> > xref-find-definitions.  Or scratched, if it's too much work, because
> > it's not terribly useful.
>
> I'm sure it will be useful in some situations, I'm just not sure about
> the odds.
>
> > In that special case we will do no worse than the current version,
> > wouldn't we?
>
> We would.
>
> Say, we're looking for clojure.core/cons, and the marker leads us to
> "(defn cons" (the example is largely made up). The code looks for
> clojure.core/cons, doesn't find it, and signals an error (will it?).
>

No, no error. Stays put. So no worse.

>
> Or worse, searches around and finds a verbatim reference to
> clojure.core/cons in a docstring somewhere in that file, and returns
> *that* location.
>

We could avoid that (particulary far fetched) case (and the string case) by
checking syntax.


> > At this point, I'm thinking of dismissing the whole thing, and voting to
> > deprecate xref-file-location entirely. Nobody uses it and Eglot will
> > probably use something else before this issue is solved.
>
> Err, it is used: in xref--collect-matches-1. And through it, in
> xref-collect-matches and xref-collect-references.
>

Oh, I must have misgrepped then. Excuse my ignorance, but are these
grep-like tools? If so, they shouldn't ever suffer from the "outdated"
malaise, right?

>
> > It's a shame
> > we can't share code, but if we can't give a default class any kind of
> > useful behavior, we might as well not have this class in the first place.
>
> We could. As long as we don't default to the identifier, and the backend
> has to explicitly provide the hint, we could be fine.
>

I guess a new xref-hinted-location subclass would be the way to go, if not
too much work.  But still think we should do something by default that will
do just as bad on the worst case. And if we use a new subclass, we're
guaranteed that.

>

[-- Attachment #2: Type: text/html, Size: 3599 bytes --]

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Dmitry Gutov]

On 7/3/18 6:43 PM, João Távora wrote:

> No, no error. Stays put. So no worse.

If it doesn't find the qualified symbol name somewhere else verbatim.

> We could avoid that (particulary far fetched) case (and the string case) 
> by checking syntax.

I wouldn't say it's far-fetched. More importantly, it adds a non-obvious 
condition on how symbol names should be presented in the completion 
table. Even if this gun doesn't shoot most of the time.

Maybe it's not in a docstring, but in some macro definition, or passed 
verbatim in some other language construct. Maybe it's part of some other 
definition name (separate definitions for methods in C++ come to mind).

> Oh, I must have misgrepped then. Excuse my ignorance, but are these 
> grep-like tools? If so, they shouldn't ever suffer from the "outdated" 
> malaise, right?

Well, the grep results buffer can (and often does) live after the user 
has opened one of the search results and made a change there.

It can certainly become outdated if the user has added or deleted a 
couple of lines there.

> I guess a new xref-hinted-location subclass would be the way to go, if 
> not too much work.  But still think we should do something by default 
> that will do just as bad on the worst case.
"won't do"? I'm unclear on your meaning here.

I'm not sure we really need a subclass if a new optional field would 
work just as well. It might be shorter implementation-wise, too.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 7/3/18 6:43 PM, João Távora wrote:
>
> Maybe it's not in a docstring, but in some macro definition, or passed
> verbatim in some other language construct. Maybe it's part of some
> other definition name (separate definitions for methods in C++ come to
> mind).

Still better than failing randomly.  Especially if you hint that the
match was approximate (much like the way diff tell you about "fuzz").

Anyway, those totally hypothetic future users of the class could well
and polish the default behaviour if they decided it's worth it.

> It can certainly become outdated if the user has added or deleted a
> couple of lines there.

But then, in that case, isn't it much better to find an actual match
(and perhaps warn) than to land the user in nowhereland?

Sure it won't probably won't beat the etags backend in the percentage of
times it gets it right.  But that's a quantitative difference, not
qualitative.

> I'm not sure we really need a subclass if a new optional field would
> work just as well.

Because I want something with some default behaviour.  Behaviour that is
admittely half-decent, but nevertheless better than current behaviour.
But since you showed that xref-location-marker is used by your grep
substitute, I don't want to cause any regressions in its existing,
broken behaviour, whatever that may be exactly.

> It might be shorter implementation-wise, too.

This is how I imagine the implementation:

   (defclass xref-hinted-location (xref-file-location)
     ((hint :initarg :hint))))
    
   (cl-defmethod xref-location-marker :around ((l xref-hinted-location))
      <...code to search around...>)

Compared to the "add optional field" approach, there would be about 1 extra line,
the defclass one.

João

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Dmitry Gutov]

On 7/4/18 4:37 PM, João Távora wrote:

> Still better than failing randomly.  Especially if you hint that the
> match was approximate (much like the way diff tell you about "fuzz").

The problem is it will "break" correct navigations sometimes.

> Anyway, those totally hypothetic future users of the class could well
> and polish the default behaviour if they decided it's worth it.

Suppose we use a new class. If the hint argument is mandatory, then 
there should be no problem: the backend explicitly asked for this behavior.

Similarly if xref-file-location grows a new optional field which 
defaults to nil.

>> It can certainly become outdated if the user has added or deleted a
>> couple of lines there.
> 
> But then, in that case, isn't it much better to find an actual match
> (and perhaps warn) than to land the user in nowhereland?

Yes. And in Grep results, it might be beneficial to use the new 
behavior. The code creating the locations will pass line's contents as 
the "hint" (and maybe we should make the hint a regexp, to be able to 
use the line-beginning and line-ending anchors). In that use case, 
though, it would be better to error out if the hint is not found.

I'm somewhat worried about what will happen if a hint misdetects a match 
anyway, and we're in many-automatic-edits land (such as 
xref-query-replace-in-results), but on the balance, it'll probably be 
better than the current behavior anyway.

Except for, err... lines with several matches on them. Not sure what to 
do about those. A special class, probably.

> Because I want something with some default behaviour.  Behaviour that is
> admittely half-decent, but nevertheless better than current behaviour.
> But since you showed that xref-location-marker is used by your grep
> substitute, I don't want to cause any regressions in its existing,
> broken behaviour, whatever that may be exactly.
> 
>> It might be shorter implementation-wise, too.
> 
> This is how I imagine the implementation:
> 
>     (defclass xref-hinted-location (xref-file-location)
>       ((hint :initarg :hint))))
>      
>     (cl-defmethod xref-location-marker :around ((l xref-hinted-location))
>        <...code to search around...>)
> 
> Compared to the "add optional field" approach, there would be about 1 extra line,
> the defclass one.

If we use an optional field, we could call ignore-errors around 
forward-char if that field is present (your proposal number 1).

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 7/4/18 4:37 PM, João Távora wrote:
>
>> Still better than failing randomly.  Especially if you hint that the
>> match was approximate (much like the way diff tell you about "fuzz").
>
> The problem is it will "break" correct navigations sometimes.

I don't understand.  Will _successful_ navigation ever land me on a spot
where the identifier I looked for _isn't_ there?

>> Anyway, those totally hypothetic future users of the class could well
>> and polish the default behaviour if they decided it's worth it.
>
> Suppose we use a new class. If the hint argument is mandatory, then
> there should be no problem: the backend explicitly asked for this
> behavior.

Yes, we agree.

> Similarly if xref-file-location grows a new optional field which
> defaults to nil.

Only in this case it's more code in the backend, and repeated across
backends.

>>> It can certainly become outdated if the user has added or deleted a
>>> couple of lines there.
>>
>> But then, in that case, isn't it much better to find an actual match
>> (and perhaps warn) than to land the user in nowhereland?
>
> Yes. And in Grep results, it might be beneficial to use the new
> behavior. The code creating the locations will pass line's contents as
> the "hint" (and maybe we should make the hint a regexp, to be able to
> use the line-beginning and line-ending anchors). In that use case,
> though, it would be better to error out if the hint is not found.
>
> I'm somewhat worried about what will happen if a hint misdetects a
> match anyway, and we're in many-automatic-edits land (such as
> xref-query-replace-in-results), but on the balance, it'll probably be
> better than the current behavior anyway.
>
> Except for, err... lines with several matches on them. Not sure what
> to do about those. A special class, probably.

So it would be a good idea to have this in grep/xref grep results after
all?

>> Because I want something with some default behaviour.  Behaviour that is
>> admittely half-decent, but nevertheless better than current behaviour.
>> But since you showed that xref-location-marker is used by your grep
>> substitute, I don't want to cause any regressions in its existing,
>> broken behaviour, whatever that may be exactly.
>>
>>> It might be shorter implementation-wise, too.
>>
>> This is how I imagine the implementation:
>>
>>     (defclass xref-hinted-location (xref-file-location)
>>       ((hint :initarg :hint))))
>>          (cl-defmethod xref-location-marker :around ((l
>> xref-hinted-location))
>>        <...code to search around...>)
>>
>> Compared to the "add optional field" approach, there would be about 1 extra line,
>> the defclass one.
>
> If we use an optional field, we could call ignore-errors around
> forward-char if that field is present (your proposal number 1).

I don't fully understand this, but I just remembered that it's better to
have a separate class for another, probably more interesting reason.  We
should just make it a mix-in: that way, we can give "hinted" semantics
to existing location classes, not just xref-file-location.

Regarding ignore-errors, we should (quite independently of the remaining
discussion) first agree if xref-location-marker should be allowed to
error at all, and what should happen if it does.

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[Dmitry Gutov]

On 7/4/18 5:53 PM, João Távora wrote:

> I don't understand.  Will _successful_ navigation ever land me on a spot
> where the identifier I looked for _isn't_ there?

A similar string in the buffer, yes. But it might be not its definition.

Like in the example I made up, clojure.core/cons -> "(defn cons".

And I *do* think, when a hint is not found, the method should raise an 
error. This will be helpful for the search-replace functionality.

>> Similarly if xref-file-location grows a new optional field which
>> defaults to nil.
> 
> Only in this case it's more code in the backend, and repeated across
> backends.

Err, no? In particular, if the new code is in xref-file-location, any 
class inheriting from it could use it automatically. No need to repeat it.

> So it would be a good idea to have this in grep/xref grep results after
> all?

A good one, yes, but not so easily-implemented one.

>> If we use an optional field, we could call ignore-errors around
>> forward-char if that field is present (your proposal number 1).
> 
> I don't fully understand this, but I just remembered that it's better to
> have a separate class for another, probably more interesting reason.  We
> should just make it a mix-in: that way, we can give "hinted" semantics
> to existing location classes, not just xref-file-location.

It sounds useful, but I'm not sure how useful it's going to be in 
practice. E.g. elisp locations already have their own logic for that. 
Etags does, too.

And if another backend decides not to use xref-file-location, it will 
likely do so for reasons incompatible with this mixin's implementation too.

Anyway, if you'd like to propose a patch, that could be easier to discuss.

> Regarding ignore-errors, we should (quite independently of the remaining
> discussion) first agree if xref-location-marker should be allowed to
> error at all, and what should happen if it does.

I think it should.

   (cl-defmethod xref-location-marker ((l xref-bogus-location))
     (user-error "%s" (oref l message)))

is the canonical example.

And then xref-query-replace-in-results should catch them. I thought it's 
doing that already. :-(

bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 7/4/18 5:53 PM, João Távora wrote:
>
>> I don't understand.  Will _successful_ navigation ever land me on a spot
>> where the identifier I looked for _isn't_ there?
> A similar string in the buffer, yes. But it might be not its definition.
>
> Like in the example I made up, clojure.core/cons -> "(defn cons".

If this backend already exists, then I think it is flawed.  If it
doesn't, then it shouldn't be designed like this.  In a better design
xref-backend-identifier-at-point would return "cons", propertized with
some contextual information (in this case the fact that it belongs to
the "clojure.core/" namespace).

> And I *do* think, when a hint is not found, the method should raise an
> error. This will be helpful for the search-replace functionality.

Then maybe that functionality should bind some global variable to
control this behavior (in CL this would be done more elegantly with
restarts, but we don't have those yet).

>>> Similarly if xref-file-location grows a new optional field which
>>> defaults to nil.
>>
>> Only in this case it's more code in the backend, and repeated across
>> backends.
>
> Err, no? In particular, if the new code is in xref-file-location, any
> class inheriting from it could use it automatically. No need to repeat
> it.

We seem to be misunderstanding each other, and perhaps this is not so
important.  I merely meant that if the behaviour is not the default,
then backends will have to repeatedly activate it explicitly.
Simplistically, if the default behaviour were good enough for more than
50% of such backends, we're better off making it the default.

>> So it would be a good idea to have this in grep/xref grep results after
>> all?
>
> A good one, yes, but not so easily-implemented one.

Indeed, I don't fully understand what you have in mind.  But maybe
(actually hopefully) it could be implemented on top of my simplistic
approach.

>>> If we use an optional field, we could call ignore-errors around
>>> forward-char if that field is present (your proposal number 1).
>>
>> I don't fully understand this, but I just remembered that it's better to
>> have a separate class for another, probably more interesting reason.  We
>> should just make it a mix-in: that way, we can give "hinted" semantics
>> to existing location classes, not just xref-file-location.
>
> It sounds useful, but I'm not sure how useful it's going to be in
> practice. E.g. elisp locations already have their own logic for
> that. Etags does, too.

Yes, and it could/should be refactored to use the new mixin.  Also
you're ignoring stuff outside Emacs.  The main reason I'm interested in
this is, obviously, my own itch.  Which in this case is SLY and Eglot.
Both need locations and both need hinting.  Incidentally, SLY's ancestor
SLIME is where all this xref rubbish was lifted from in the first place
(mighty fine rubbish mind you :-)).

> And if another backend decides not to use xref-file-location, it will
> likely do so for reasons incompatible with this mixin's implementation
> too.

No.  Precisely the contrary.  In this proposal, the two classes are
independent so you needn't scratch one if you just don't like the other.
>
> Anyway, if you'd like to propose a patch, that could be easier to discuss.
>> Regarding ignore-errors, we should (quite independently of the remaining
>> discussion) first agree if xref-location-marker should be allowed to
>> error at all, and what should happen if it does.
>
> I think it should.
>
>   (cl-defmethod xref-location-marker ((l xref-bogus-location))
>     (user-error "%s" (oref l message)))
>
> is the canonical example.

Yes, but we have to agree how/if to unwind.  There are at least three
situations:

a) Error before the file is found (and buffer created)
b) Error after the file is found
c) No error

I think a) should always happen, but b) should only happen if a global
flag is bound (that seems to help your replacer, right?)

> And then xref-query-replace-in-results should catch them. I thought
> it's doing that already. :-(

João