bug#65631: Xref updates stack in case of error

bug#65631: Xref updates stack in case of error

[Juri Linkov]

X-Debbugs-CC: Dmitry Gutov <dmitry@gutov.dev>

0. emacs -Q
1. move point to any word
2. type 'M-.' (xref-find-definitions)
3. an error is displayed correctly "No definitions found for: This"

But the problem is that an unrecognized word is added to the xref stack.
So the context menu shows "Go Back", and 'M-,' (xref-go-back) goes back
to that word.

bug#65631: Xref updates stack in case of error

[Eli Zaretskii]

> Cc: dmitry gutov <dmitry@gutov.dev>
> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 30 Aug 2023 19:44:13 +0300
> 
> X-Debbugs-CC: Dmitry Gutov <dmitry@gutov.dev>
> 
> 0. emacs -Q
> 1. move point to any word
> 2. type 'M-.' (xref-find-definitions)
> 3. an error is displayed correctly "No definitions found for: This"
> 
> But the problem is that an unrecognized word is added to the xref stack.
> So the context menu shows "Go Back", and 'M-,' (xref-go-back) goes back
> to that word.

FWIW, I'm not sure this is necessarily a bug.  It looks like a bug in
your case, because you deliberately tricked Xref into using a word
that is definitely not a program symbol.  But that is not the case in
a more reasonable situation, where point is on a symbol, but for some
reason the symbol's definition is not found, e.g., because the TAGS
table needs to be regenerated.

bug#65631: Xref updates stack in case of error

[Juri Linkov]

>> 1. move point to any word
>> 2. type 'M-.' (xref-find-definitions)
>> 3. an error is displayed correctly "No definitions found for: This"
>>
>> But the problem is that an unrecognized word is added to the xref stack.
>> So the context menu shows "Go Back", and 'M-,' (xref-go-back) goes back
>> to that word.
>
> FWIW, I'm not sure this is necessarily a bug.  It looks like a bug in
> your case, because you deliberately tricked Xref into using a word
> that is definitely not a program symbol.  But that is not the case in
> a more reasonable situation, where point is on a symbol, but for some
> reason the symbol's definition is not found, e.g., because the TAGS
> table needs to be regenerated.

In case of error, point doesn't move.  So there is no need to go back.

bug#65631: Xref updates stack in case of error

[Dmitry Gutov]

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

Hi!

On 30/08/2023 20:16, Juri Linkov wrote:
>>> 1. move point to any word
>>> 2. type 'M-.' (xref-find-definitions)
>>> 3. an error is displayed correctly "No definitions found for: This"
>>>
>>> But the problem is that an unrecognized word is added to the xref stack.
>>> So the context menu shows "Go Back", and 'M-,' (xref-go-back) goes back
>>> to that word.
>> FWIW, I'm not sure this is necessarily a bug.  It looks like a bug in
>> your case, because you deliberately tricked Xref into using a word
>> that is definitely not a program symbol.  But that is not the case in
>> a more reasonable situation, where point is on a symbol, but for some
>> reason the symbol's definition is not found, e.g., because the TAGS
>> table needs to be regenerated.
> In case of error, point doesn't move.  So there is no need to go back.

At this point, I was kind of feeling that this is a known-but-tolerated 
behavior (that some might already be relying on, also known as "spacebar 
heater" effect), but if it's still annoying, let's see about a fix.

How's the attached patch?

[-- Attachment #2: xref--push-markers.diff --]
[-- Type: text/x-patch, Size: 2078 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index dbafa00c3ad..9b64ef4de10 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1468,7 +1468,6 @@ xref-show-xrefs
   (xref--show-xrefs fetcher display-action))
 
 (defun xref--show-xrefs (fetcher display-action &optional _always-show-list)
-  (xref--push-markers)
   (unless (functionp fetcher)
     ;; Old convention.
     (let ((xrefs fetcher))
@@ -1479,21 +1478,30 @@ xref--show-xrefs
                 (prog1
                     xrefs
                   (setq xrefs 'called-already)))))))
-  (funcall xref-show-xrefs-function fetcher
-           `((window . ,(selected-window))
-             (display-action . ,display-action)
-             (auto-jump . ,xref-auto-jump-to-first-xref))))
+  (let ((cb (current-buffer))
+        (pt (point)))
+    (funcall xref-show-xrefs-function fetcher
+             `((window . ,(selected-window))
+               (display-action . ,display-action)
+               (auto-jump . ,xref-auto-jump-to-first-xref)))
+    (xref--push-markers cb pt)))
 
 (defun xref--show-defs (xrefs display-action)
-  (xref--push-markers)
-  (funcall xref-show-definitions-function xrefs
-           `((window . ,(selected-window))
-             (display-action . ,display-action)
-             (auto-jump . ,xref-auto-jump-to-first-definition))))
-
-(defun xref--push-markers ()
-  (unless (region-active-p) (push-mark nil t))
-  (xref-push-marker-stack))
+  (let ((cb (current-buffer))
+        (pt (point)))
+    (funcall xref-show-definitions-function xrefs
+             `((window . ,(selected-window))
+               (display-action . ,display-action)
+               (auto-jump . ,xref-auto-jump-to-first-definition)))
+    (xref--push-markers cb pt)))
+
+(defun xref--push-markers (buf pt)
+  (when (buffer-live-p buf)
+    (save-excursion
+      (with-no-warnings (set-buffer buf))
+      (goto-char pt)
+      (unless (region-active-p) (push-mark nil t))
+      (xref-push-marker-stack))))
 
 (defun xref--prompt-p (command)
   (or (eq xref-prompt-for-identifier t)

bug#65631: Xref updates stack in case of error

[Juri Linkov]

>>>> 1. move point to any word
>>>> 2. type 'M-.' (xref-find-definitions)
>>>> 3. an error is displayed correctly "No definitions found for: This"
>>>>
>>>> But the problem is that an unrecognized word is added to the xref stack.
>>>> So the context menu shows "Go Back", and 'M-,' (xref-go-back) goes back
>>>> to that word.
>>> FWIW, I'm not sure this is necessarily a bug.  It looks like a bug in
>>> your case, because you deliberately tricked Xref into using a word
>>> that is definitely not a program symbol.  But that is not the case in
>>> a more reasonable situation, where point is on a symbol, but for some
>>> reason the symbol's definition is not found, e.g., because the TAGS
>>> table needs to be regenerated.
>> In case of error, point doesn't move.  So there is no need to go back.
>
> At this point, I was kind of feeling that this is a known-but-tolerated
> behavior (that some might already be relying on, also known as "spacebar
> heater" effect), but if it's still annoying, let's see about a fix.

It's not annoying, just surprises with the requirement to type extra ‘M-,’.

> How's the attached patch?

Thanks.  I expected a smaller change, but if this is still reliable,
then why not.  At least, it works in cases that I tested.

bug#65631: Xref updates stack in case of error

[Dmitry Gutov]

Version: 30.1

On 31/08/2023 19:33, Juri Linkov wrote:
>>>>> 1. move point to any word
>>>>> 2. type 'M-.' (xref-find-definitions)
>>>>> 3. an error is displayed correctly "No definitions found for: This"
>>>>>
>>>>> But the problem is that an unrecognized word is added to the xref stack.
>>>>> So the context menu shows "Go Back", and 'M-,' (xref-go-back) goes back
>>>>> to that word.
>>>> FWIW, I'm not sure this is necessarily a bug.  It looks like a bug in
>>>> your case, because you deliberately tricked Xref into using a word
>>>> that is definitely not a program symbol.  But that is not the case in
>>>> a more reasonable situation, where point is on a symbol, but for some
>>>> reason the symbol's definition is not found, e.g., because the TAGS
>>>> table needs to be regenerated.
>>> In case of error, point doesn't move.  So there is no need to go back.
>>
>> At this point, I was kind of feeling that this is a known-but-tolerated
>> behavior (that some might already be relying on, also known as "spacebar
>> heater" effect), but if it's still annoying, let's see about a fix.
> 
> It's not annoying, just surprises with the requirement to type extra ‘M-,’.
> 
>> How's the attached patch?
> 
> Thanks.  I expected a smaller change, but if this is still reliable,
> then why not.  At least, it works in cases that I tested.

I was looking for a smaller change, too. Thanks for testing.

Pushed to master in 17188e07ab9, closing.

bug#65631: Xref updates stack in case of error

[Mattias Engdegård]

17188e07ab9 appears to break etags-tests on master.

bug#65631: Xref updates stack in case of error

[Dmitry Gutov]

On 01/09/2023 16:25, Mattias Engdegård wrote:
> 17188e07ab9 appears to break etags-tests on master.

Thanks for reporting, should be fixed in f735eb9628.