bug#44294: No widen by xref-find-definitions

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

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

X-Debbugs-CC: Dmitry Gutov <dgutov@yandex.ru>
Tags: patch

>>> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
>>> @@ -897,8 +897,10 @@ xref-location-marker
>>> -          (goto-char (or (cdr buffer-point) (point-min)))
>>> -          (point-marker))))))
>>> +          (save-restriction
>>> +            (widen)
>>> +            (goto-char (or (cdr buffer-point) (point-min)))
>>> +            (point-marker)))))))
>
> If this works, sure, please install. This piece by itself makes sense.
>
>> Hm...  Does Dmitry have any comments here?  (Added to Cc.)
>
> I'm subscribed to the bug tracker, but I skip over a number of discussions
> automatically (and an extra email in a thread is relatively easy to miss,
> too).
>
> In case I don't respond in similar circumstances in the future, may I ask
> for a personal email? One without 'debbugs.gnu.org' in the From/To/Cc.

Oh, sorry, I noticed this problem is just the tip of the iceberg,
so creating a separate bug report from bug#5042 and bug#9917.

The patch above fixes the problem of using xref-find-definitions
in elisp-mode, but using xref-find-definitions in narrowed
C files still fails with the error "Rerun etags: `%s' not found in %s".

Here is an additional patch that fixes this as well:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: etags-goto-tag-location-widen.patch --]
[-- Type: text/x-diff, Size: 845 bytes --]

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index f6af1f2ea8..1b7f3c72a2 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -1406,8 +1406,12 @@ etags-goto-tag-location
 	      offset (* 3 offset)))	; expand search window
       (or found
 	  (re-search-forward pat nil t)
-	  (user-error "Rerun etags: `%s' not found in %s"
-                      pat buffer-file-name)))
+	  (if (or (= (point-min) 1) (not widen-automatically))
+              (user-error "Rerun etags: `%s' not found in %s"
+                          pat buffer-file-name)
+            ;; Rerun after removing narrowing
+            (widen)
+            (etags-goto-tag-location tag-info))))
     ;; Position point at the right place
     ;; if the search string matched an extra Ctrl-m at the beginning.
     (and (eq selective-display t)

bug#44294: No widen by xref-find-definitions

[Dmitry Gutov]

On 28.10.2020 22:01, Juri Linkov wrote:
> Oh, sorry, I noticed this problem is just the tip of the iceberg,
> so creating a separate bug report from bug#5042 and bug#9917.
> 
> The patch above fixes the problem of using xref-find-definitions
> in elisp-mode, but using xref-find-definitions in narrowed
> C files still fails with the error "Rerun etags: `%s' not found in %s".
> 
> Here is an additional patch that fixes this as well:

Thank you. I'm fine with both patches, please feel free to install.

> -	  (user-error "Rerun etags: `%s' not found in %s"
> -                      pat buffer-file-name)))
> +	  (if (or (= (point-min) 1) (not widen-automatically))

Couldn't a buffer be narrowed, but point-min still be equal to 1?

bug#44294: No widen by xref-find-definitions

[Philipp Stephani]

Am Mi., 28. Okt. 2020 um 22:39 Uhr schrieb Dmitry Gutov <dgutov@yandex.ru>:
>
> > -       (user-error "Rerun etags: `%s' not found in %s"
> > -                      pat buffer-file-name)))
> > +       (if (or (= (point-min) 1) (not widen-automatically))
>
> Couldn't a buffer be narrowed, but point-min still be equal to 1?
>

Yes, this should be (eql (buffer-size) (- (point-max) (point-min)).

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

tags 44294 fixed
close 44294 28.0.50
thanks

>> -	  (user-error "Rerun etags: `%s' not found in %s"
>> -                      pat buffer-file-name)))
>> +	  (if (or (= (point-min) 1) (not widen-automatically))
>
> Couldn't a buffer be narrowed, but point-min still be equal to 1?

Thanks for the good question.  Now I installed the patch that
uses the special function 'buffer-narrowed-p'.

This bug could be reopened when more such cases will be discovered.

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

>> > -       (user-error "Rerun etags: `%s' not found in %s"
>> > -                      pat buffer-file-name)))
>> > +       (if (or (= (point-min) 1) (not widen-automatically))
>>
>> Couldn't a buffer be narrowed, but point-min still be equal to 1?
>
> Yes, this should be (eql (buffer-size) (- (point-max) (point-min)).

It's nice that you arrived to the same code that is used in the
existing function buffer-narrowed-p (I missed it before).

bug#44294: No widen by xref-find-definitions

[Dmitry Gutov]

On 28.10.2020 22:01, Juri Linkov wrote:
> -	  (user-error "Rerun etags: `%s' not found in %s"
> -                      pat buffer-file-name)))
> +	  (if (or (= (point-min) 1) (not widen-automatically))
> +              (user-error "Rerun etags: `%s' not found in %s"
> +                          pat buffer-file-name)
> +            ;; Rerun after removing narrowing
> +            (widen)
> +            (etags-goto-tag-location tag-info))))

By the way... have you tried to use the same method here as in 
elisp-mode? Meaning, widen unconditionally inside 'save-restriction'.

There should be no reason for backends to do it differently. And this 
way, you don't have to always search twice for a missing tag when inside 
a narrowing.

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

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

reopen 44294
stop

>> -	  (user-error "Rerun etags: `%s' not found in %s"
>> -                      pat buffer-file-name)))
>> +	  (if (or (= (point-min) 1) (not widen-automatically))
>> +              (user-error "Rerun etags: `%s' not found in %s"
>> +                          pat buffer-file-name)
>> +            ;; Rerun after removing narrowing
>> +            (widen)
>> +            (etags-goto-tag-location tag-info))))
>
> By the way... have you tried to use the same method here as in elisp-mode?
> Meaning, widen unconditionally inside 'save-restriction'.
>
> There should be no reason for backends to do it differently. And this way,
> you don't have to always search twice for a missing tag when inside
> a narrowing.

It should not widen unnecessarily when the found position is within the
narrowed region.  In this regard, xref--goto-char does the right thing:

  (defun xref--goto-char (pos)
    (cond
     ((and (<= (point-min) pos) (<= pos (point-max))))
     (widen-automatically (widen))
     (t (user-error "Position is outside accessible part of buffer")))
    (goto-char pos))

It widens only when position is outside accessible part of buffer
(and widen-automatically is non-nil).

Now I fixed goto-line to use the same condition as in xref--goto-char

  (and (<= (point-min) p) (<= p (point-max)))

to widen only when the found position is outside accessible part of buffer:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: goto-line-widen-only-accessible.patch --]
[-- Type: text/x-diff, Size: 1335 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 2e40e3261c..0cd1739c4d 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1340,18 +1340,19 @@ goto-line
   ;; Leave mark at previous position
   (or (region-active-p) (push-mark))
   ;; Move to the specified line number in that buffer.
-  (if (and (not relative) (not widen-automatically))
-      (save-restriction
-        (widen)
-        (goto-char (point-min))
-        (if (eq selective-display t)
-            (re-search-forward "[\n\C-m]" nil 'end (1- line))
-          (forward-line (1- line))))
-    (unless relative (widen))
-    (goto-char (point-min))
-    (if (eq selective-display t)
-	(re-search-forward "[\n\C-m]" nil 'end (1- line))
-      (forward-line (1- line)))))
+  (let ((p (save-restriction
+             (unless relative (widen))
+             (goto-char (point-min))
+             (if (eq selective-display t)
+                 (re-search-forward "[\n\C-m]" nil 'end (1- line))
+               (forward-line (1- line)))
+             (point))))
+    (when (and (not relative)
+               (buffer-narrowed-p)
+               widen-automatically
+               (not (and (<= (point-min) p) (<= p (point-max)))))
+      (widen))
+    (goto-char p)))
 
 (defun goto-line-relative (line &optional buffer)
   "Go to LINE, counting from line at (point-min).

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

BTW, I noticed that when isearch fails it doesn't inform the user
that there are possibly more matches outside the narrowed region:

diff --git a/lisp/isearch.el b/lisp/isearch.el
index c3d5ff2d31..dae7618556 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3276,6 +3276,8 @@ isearch-message-prefix
 		    (multi-isearch-file-list "multi-file ")
 		    (multi-isearch-buffer-list "multi-buffer ")
 		    (t ""))
+		   (if (and (not isearch-success) (buffer-narrowed-p) widen-automatically)
+		       "narrowed-buffer " "")
 		   (or isearch-message-prefix-add "")
 		   (if nonincremental "search" "I-search")
 		   (if isearch-forward "" " backward")

bug#44294: No widen by xref-find-definitions

[Dmitry Gutov]

On 29.10.2020 23:18, Juri Linkov wrote:
> reopen 44294
> stop
> 
>>> -	  (user-error "Rerun etags: `%s' not found in %s"
>>> -                      pat buffer-file-name)))
>>> +	  (if (or (= (point-min) 1) (not widen-automatically))
>>> +              (user-error "Rerun etags: `%s' not found in %s"
>>> +                          pat buffer-file-name)
>>> +            ;; Rerun after removing narrowing
>>> +            (widen)
>>> +            (etags-goto-tag-location tag-info))))
>>
>> By the way... have you tried to use the same method here as in elisp-mode?
>> Meaning, widen unconditionally inside 'save-restriction'.
>>
>> There should be no reason for backends to do it differently. And this way,
>> you don't have to always search twice for a missing tag when inside
>> a narrowing.
> 
> It should not widen unnecessarily when the found position is within the
> narrowed region.

Hence the use of save-restriction in elisp-mode which you added.

etags can use the exact same approach.

>  In this regard, xref--goto-char does the right thing:

Yes.

>    (defun xref--goto-char (pos)
>      (cond
>       ((and (<= (point-min) pos) (<= pos (point-max))))
>       (widen-automatically (widen))
>       (t (user-error "Position is outside accessible part of buffer")))
>      (goto-char pos))
> 
> It widens only when position is outside accessible part of buffer
> (and widen-automatically is non-nil).

And this code runs whether the backend is elisp, etags, or whatever else.

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

>>> By the way... have you tried to use the same method here as in elisp-mode?
>>> Meaning, widen unconditionally inside 'save-restriction'.
>>>
>>> There should be no reason for backends to do it differently. And this way,
>>> you don't have to always search twice for a missing tag when inside
>>> a narrowing.
>> It should not widen unnecessarily when the found position is within the
>> narrowed region.
>
> Hence the use of save-restriction in elisp-mode which you added.
>
> etags can use the exact same approach.

I see now what you mean.  Then maybe better to revert the change in
etags-goto-tag-location to restore its original behavior, and
fix only its usage by xref, i.e. exactly the same change
for xref-location-marker ((l xref-etags-location)) as was made
for xref-location-marker ((l xref-elisp-location)):

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index 41ed317766..8879726ad5 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2140,8 +2135,10 @@ xref-location-marker
     (let ((buffer (find-file-noselect file)))
       (with-current-buffer buffer
         (save-excursion
-          (etags-goto-tag-location tag-info)
-          (point-marker))))))
+          (save-restriction
+            (widen)
+            (etags-goto-tag-location tag-info)
+            (point-marker)))))))
 
 (cl-defmethod xref-location-line ((l xref-etags-location))
   (with-slots (tag-info) l

bug#44294: No widen by xref-find-definitions

[Dmitry Gutov]

On 30.10.2020 09:38, Juri Linkov wrote:
> I see now what you mean.  Then maybe better to revert the change in
> etags-goto-tag-location to restore its original behavior, and
> fix only its usage by xref, i.e. exactly the same change
> for xref-location-marker ((l xref-etags-location)) as was made
> for xref-location-marker ((l xref-elisp-location)):

Sounds good. 'find-tag-in-order' calls 'widen' already, so 'find-tag' is 
probably fine.

I would personally combine the revert and the new change in one commit, 
but either option will work, obviously.

bug#44294: No widen by xref-find-definitions

[Juri Linkov]

tags 44294 fixed
close 44294 28.0.50
thanks

>> I see now what you mean.  Then maybe better to revert the change in
>> etags-goto-tag-location to restore its original behavior, and
>> fix only its usage by xref, i.e. exactly the same change
>> for xref-location-marker ((l xref-etags-location)) as was made
>> for xref-location-marker ((l xref-elisp-location)):
>
> Sounds good. 'find-tag-in-order' calls 'widen' already, so 'find-tag' is
> probably fine.
>
> I would personally combine the revert and the new change in one commit, but
> either option will work, obviously.

Thanks for the suggestion.  Now installed and closed.

bug#44294: No widen by xref-find-definitions

[Dmitry Gutov]

On 31.10.2020 21:41, Juri Linkov wrote:
> Thanks for the suggestion.  Now installed and closed.

Thanks!