bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

X-Debbugs-Cc: Philip Kaludercic <philipk@posteo.net>

This is not reproducible in Emacs 28, so looks like a recent regression:

0. emacs -Q
1. M-x TAB
2. move point to the beginning of the *Completions* buffer
3. type S-TAB (<backtab>)

Debugger entered--Lisp error: (args-out-of-range 0 0)
  get-text-property(0 mouse-face)
  next-completion(-1)
  previous-completion(1)
  funcall-interactively(previous-completion 1)
  command-execute(previous-completion)
  completing-read-default("M-x " ...)
  read-extended-command()
  command-execute(execute-extended-command)

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Philip Kaludercic]

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

Juri Linkov <juri@linkov.net> writes:

> X-Debbugs-Cc: Philip Kaludercic <philipk@posteo.net>
>
> This is not reproducible in Emacs 28, so looks like a recent regression:

This seems to fix it:


[-- Attachment #2: Type: text/plain, Size: 595 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index accc119e2b..7d47aba1ee 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9125,7 +9125,7 @@ next-completion
         (unless (get-text-property (point) 'mouse-face)
           (goto-char (next-single-property-change (point) 'mouse-face nil end)))
         (setq n (1- n)))
-      (while (< n 0)
+      (while (and (< n 0) (< 1 (point)))
         (let ((prop (get-text-property (1- (point)) 'mouse-face)))
           ;; If in a completion, move to the start of it.
           (when (and prop (eq prop (get-text-property (point) 'mouse-face)))

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



> 0. emacs -Q
> 1. M-x TAB
> 2. move point to the beginning of the *Completions* buffer
> 3. type S-TAB (<backtab>)
>
> Debugger entered--Lisp error: (args-out-of-range 0 0)
>   get-text-property(0 mouse-face)
>   next-completion(-1)
>   previous-completion(1)
>   funcall-interactively(previous-completion 1)
>   command-execute(previous-completion)
>   completing-read-default("M-x " ...)
>   read-extended-command()
>   command-execute(execute-extended-command)
>
>

-- 
	Philip Kaludercic

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 13 Mar 2022 20:39:53 +0000
> Cc: 54374@debbugs.gnu.org
> 
> -      (while (< n 0)
> +      (while (and (< n 0) (< 1 (point)))

Please always use point-min, never a literal 1.

Thanks.

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sun, 13 Mar 2022 20:39:53 +0000
>> Cc: 54374@debbugs.gnu.org
>> 
>> -      (while (< n 0)
>> +      (while (and (< n 0) (< 1 (point)))
>
> Please always use point-min, never a literal 1.

Can do, but does it make a difference in this case?  The only reason the
comparison is made is so that (1- (point)) doesn't return an invalid
point value.

> Thanks.

-- 
	Philip Kaludercic

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  54374@debbugs.gnu.org
> Date: Mon, 14 Mar 2022 08:20:29 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Philip Kaludercic <philipk@posteo.net>
> >> Date: Sun, 13 Mar 2022 20:39:53 +0000
> >> Cc: 54374@debbugs.gnu.org
> >> 
> >> -      (while (< n 0)
> >> +      (while (and (< n 0) (< 1 (point)))
> >
> > Please always use point-min, never a literal 1.
> 
> Can do, but does it make a difference in this case?  The only reason the
> comparison is made is so that (1- (point)) doesn't return an invalid
> point value.

Any buffer position outside of the accessible portion of the buffer is
almost always invalid; the number of APIs which tolerate such values
of buffer position is very small.

Here, try this:

  emacs -Q
  M-: (narrow-to-region 20 (point-max)) RET
  M-: (get-text-property 1 'mouse-face) RET

You will get the same args-out-of-range error.  That's because
get-text-property validates the position against the accessible
portion of the buffer, not against some absolute values.

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  54374@debbugs.gnu.org
>> Date: Mon, 14 Mar 2022 08:20:29 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Philip Kaludercic <philipk@posteo.net>
>> >> Date: Sun, 13 Mar 2022 20:39:53 +0000
>> >> Cc: 54374@debbugs.gnu.org
>> >> 
>> >> -      (while (< n 0)
>> >> +      (while (and (< n 0) (< 1 (point)))
>> >
>> > Please always use point-min, never a literal 1.
>> 
>> Can do, but does it make a difference in this case?  The only reason the
>> comparison is made is so that (1- (point)) doesn't return an invalid
>> point value.
>
> Any buffer position outside of the accessible portion of the buffer is
> almost always invalid; the number of APIs which tolerate such values
> of buffer position is very small.
>
> Here, try this:
>
>   emacs -Q
>   M-: (narrow-to-region 20 (point-max)) RET
>   M-: (get-text-property 1 'mouse-face) RET
>
> You will get the same args-out-of-range error.  That's because
> get-text-property validates the position against the accessible
> portion of the buffer, not against some absolute values.

I didn't know that, thank you for the background information!

-- 
	Philip Kaludercic

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Andreas Schwab]

On Mär 14 2022, Eli Zaretskii wrote:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sun, 13 Mar 2022 20:39:53 +0000
>> Cc: 54374@debbugs.gnu.org
>> 
>> -      (while (< n 0)
>> +      (while (and (< n 0) (< 1 (point)))
>
> Please always use point-min, never a literal 1.

But it should be written as (not (bobp)).

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

> This seems to fix it:
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index accc119e2b..7d47aba1ee 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -9125,7 +9125,7 @@ next-completion
>          (unless (get-text-property (point) 'mouse-face)
>            (goto-char (next-single-property-change (point) 'mouse-face nil end)))
>          (setq n (1- n)))
> -      (while (< n 0)
> +      (while (and (< n 0) (< 1 (point)))
>          (let ((prop (get-text-property (1- (point)) 'mouse-face)))
>            ;; If in a completion, move to the start of it.
>            (when (and prop (eq prop (get-text-property (point) 'mouse-face)))

Thanks, I confirm that it doesn't fail.

However, there is some strange behaviour: when point is at the beginning
of the completion buffer, then previous-completion switches to the minibuffer.
But if point is at the first completion, then previous-completion wraps
to the last completion.  Shouldn't point at the beginning of the buffer
wrap to the last completion as well?

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Philip Kaludercic]

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

Juri Linkov <juri@linkov.net> writes:

>> This seems to fix it:
>>
>> diff --git a/lisp/simple.el b/lisp/simple.el
>> index accc119e2b..7d47aba1ee 100644
>> --- a/lisp/simple.el
>> +++ b/lisp/simple.el
>> @@ -9125,7 +9125,7 @@ next-completion
>>          (unless (get-text-property (point) 'mouse-face)
>>            (goto-char (next-single-property-change (point) 'mouse-face nil end)))
>>          (setq n (1- n)))
>> -      (while (< n 0)
>> +      (while (and (< n 0) (< 1 (point)))
>>          (let ((prop (get-text-property (1- (point)) 'mouse-face)))
>>            ;; If in a completion, move to the start of it.
>>            (when (and prop (eq prop (get-text-property (point) 'mouse-face)))
>
> Thanks, I confirm that it doesn't fail.
>
> However, there is some strange behaviour: when point is at the beginning
> of the completion buffer, then previous-completion switches to the minibuffer.
> But if point is at the first completion, then previous-completion wraps
> to the last completion.  Shouldn't point at the beginning of the buffer
> wrap to the last completion as well?

I didn't notice that, because completion-auto-select was enabled on my
end.  How about this:


[-- Attachment #2: Type: text/plain, Size: 632 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index accc119e2b..5fba27b868 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9108,6 +9108,13 @@ next-completion
 With prefix argument N, move N items (negative N means move
 backward)."
   (interactive "p")
+  (let ((prev (previous-single-property-change (point) 'mouse-face)))
+    (goto-char (cond
+                ((not prev)
+                 (1- (next-single-property-change (point) 'mouse-face)))
+                ((/= prev (point))
+                 (point))
+                (t prev))))
   (let ((beg (point-min)) (end (point-max)))
     (catch 'bound
       (while (> n 0)

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


(Note that this also fixes the "issue" jumping back to the beginning of
the same completion option, if the point is not at the beginning of said
option.)

-- 
	Philip Kaludercic

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

> I didn't notice that, because completion-auto-select was enabled on my
> end.  How about this:
>
> (Note that this also fixes the "issue" jumping back to the beginning of
> the same completion option, if the point is not at the beginning of said
> option.)

Thanks, this fixed the issue.  And I noticed another one: at the
beginning of the buffer previous-completion wraps to the end of the buffer,
not to the beginning of the last completion.

Also when point at the last completion, next-completion
jumps to the end of the buffer instead of wrapping to the
first completion.

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>> I didn't notice that, because completion-auto-select was enabled on my
>> end.  How about this:
>>
>> (Note that this also fixes the "issue" jumping back to the beginning of
>> the same completion option, if the point is not at the beginning of said
>> option.)
>
> Thanks, this fixed the issue.  And I noticed another one: at the
> beginning of the buffer previous-completion wraps to the end of the buffer,
> not to the beginning of the last completion.
>
> Also when point at the last completion, next-completion
> jumps to the end of the buffer instead of wrapping to the
> first completion.

I cannot replicate this issue in every case, but only if the last option
has completion annotation.  It should be fixable by checking for these
kinds of edge-cases, but it might also be better to reconsider if
next-completion should be using `mouse-face' for navigation, or if a
special text property might be more elegant?

-- 
	Philip Kaludercic

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

>> Thanks, this fixed the issue.  And I noticed another one: at the
>> beginning of the buffer previous-completion wraps to the end of the buffer,
>> not to the beginning of the last completion.
>>
>> Also when point at the last completion, next-completion
>> jumps to the end of the buffer instead of wrapping to the
>> first completion.
>
> I cannot replicate this issue in every case, but only if the last option
> has completion annotation.  It should be fixable by checking for these
> kinds of edge-cases, but it might also be better to reconsider if
> next-completion should be using `mouse-face' for navigation, or if a
> special text property might be more elegant?

I agree that `mouse-face' is unsuitable for detecting the completion
boundaries, and it causes other problems: typing RET on the completion prefix
or on the completion suffix/annotation causes the error:

  Debugger entered--Lisp error: (error "No completion here")
    error("No completion here")
    choose-completion(13 nil)
    funcall-interactively(choose-completion 13 nil)
    command-execute(choose-completion)

So a special text property could also help to detect the completion for RET,
if there are no such text properties already.  But I see there is the
text property 'completion--string'.  So maybe it could be extended to cover
prefix and suffix/annotation as well.

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

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

>>> Thanks, this fixed the issue.  And I noticed another one: at the
>>> beginning of the buffer previous-completion wraps to the end of the buffer,
>>> not to the beginning of the last completion.
>>>
>>> Also when point at the last completion, next-completion
>>> jumps to the end of the buffer instead of wrapping to the
>>> first completion.
>>
>> I cannot replicate this issue in every case, but only if the last option
>> has completion annotation.  It should be fixable by checking for these
>> kinds of edge-cases, but it might also be better to reconsider if
>> next-completion should be using `mouse-face' for navigation, or if a
>> special text property might be more elegant?
>
> I agree that `mouse-face' is unsuitable for detecting the completion
> boundaries, and it causes other problems: typing RET on the completion prefix
> or on the completion suffix/annotation causes the error:
>
>   Debugger entered--Lisp error: (error "No completion here")
>     error("No completion here")
>     choose-completion(13 nil)
>     funcall-interactively(choose-completion 13 nil)
>     command-execute(choose-completion)
>
> So a special text property could also help to detect the completion for RET,
> if there are no such text properties already.  But I see there is the
> text property 'completion--string'.  So maybe it could be extended to cover
> prefix and suffix/annotation as well.

Here is the patch that fixes 4 bugs: 2 bugs described above,
and also bug#55289 and bug#55430.


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

diff --git a/lisp/ido.el b/lisp/ido.el
index e5717d6e53..73cd163d46 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -3939,7 +3939,7 @@ ido-switch-to-completions
       ;; In the new buffer, go to the first completion.
       ;; FIXME: Perhaps this should be done in `ido-completion-help'.
       (when (bobp)
-	(next-completion 1)))))
+	(first-completion)))))
 
 (defun ido-completion-auto-help ()
   "Call `ido-completion-help' if `completion-auto-help' is non-nil."
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 8287007d32..8948d16949 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2070,11 +2151,11 @@ completion--insert
       (when prefix
         (let ((beg (point))
               (end (progn (insert prefix) (point))))
-          (put-text-property beg end 'mouse-face nil)))
+          (add-text-properties beg end `(mouse-face nil completion--string ,(car str)))))
       (completion--insert (car str) group-fun)
       (let ((beg (point))
             (end (progn (insert suffix) (point))))
-        (put-text-property beg end 'mouse-face nil)
+        (add-text-properties beg end `(mouse-face nil completion--string ,(car str)))
         ;; Put the predefined face only when suffix
         ;; is added via annotation-function without prefix,
         ;; and when the caller doesn't use own face.
diff --git a/lisp/simple.el b/lisp/simple.el
index 1efd900030..75b548aea6 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9523,6 +9523,20 @@ completion-auto-select
   :version "29.1"
   :group 'completion)
 
+(defun first-completion ()
+  (interactive)
+  (goto-char (point-min))
+  (unless (get-text-property (point) 'completion--string)
+    (let ((next (next-single-property-change (point) 'completion--string)))
+      (when next (goto-char next)))))
+
+(defun last-completion ()
+  (interactive)
+  (goto-char (point-max))
+  (unless (get-text-property (point) 'completion--string)
+    (let ((prev (previous-single-property-change (point) 'completion--string)))
+      (when prev (goto-char prev)))))
+
 (defun previous-completion (n)
   "Move to the previous item in the completion list.
 With prefix argument N, move back N items (negative N means move
@@ -9539,14 +9553,6 @@ next-completion
 
 Also see the `completion-wrap-movement' variable."
   (interactive "p")
-  (let ((prev (previous-single-property-change (point) 'mouse-face)))
-    (goto-char (cond
-                ((not prev)
-                 (1- (next-single-property-change (point) 'mouse-face)))
-                ((/= prev (point))
-                 (point))
-                (t prev))))
-
   (let ((beg (point-min))
         (end (point-max))
         (tabcommand (member (this-command-keys) '("\t" [backtab])))
@@ -9554,44 +9560,43 @@ next-completion
     (catch 'bound
       (while (> n 0)
         ;; If in a completion, move to the end of it.
-        (when (get-text-property (point) 'mouse-face)
-          (goto-char (next-single-property-change (point) 'mouse-face nil end)))
+        (when (get-text-property (point) 'completion--string)
+          (goto-char (next-single-property-change (point) 'completion--string nil end)))
         ;; If at the last completion option, wrap or skip to the
-        ;; minibuffer, if requested. We can't use (eobp) because some
-        ;; extra text may be after the last candidate: ex: when
-        ;; completion-detailed
-        (setq prop (next-single-property-change (point) 'mouse-face nil end))
+        ;; minibuffer, if requested.
+        (setq prop (next-single-property-change (point) 'completion--string nil end))
         (when (and completion-wrap-movement (eq end prop))
           (if (and completion-auto-select tabcommand)
               (throw 'bound nil)
             (goto-char (point-min))))
         ;; Move to start of next one.
-        (unless (get-text-property (point) 'mouse-face)
-          (goto-char (next-single-property-change (point) 'mouse-face nil end)))
+        (unless (get-text-property (point) 'completion--string)
+          (goto-char (next-single-property-change (point) 'completion--string nil end)))
         (setq n (1- n)))
 
-      (while (and (< n 0) (not (bobp)))
-        (setq prop (get-text-property (1- (point)) 'mouse-face))
+      (while (< n 0)
+        (unless (bobp)
+          (setq prop (get-text-property (1- (point)) 'completion--string)))
         ;; If in a completion, move to the start of it.
-        (when (and prop (eq prop (get-text-property (point) 'mouse-face)))
+        (when (and prop (eq prop (get-text-property (point) 'completion--string)))
           (goto-char (previous-single-property-change
-                      (point) 'mouse-face nil beg)))
+                      (point) 'completion--string nil beg)))
         ;; Move to end of the previous completion.
-        (unless (or (bobp) (get-text-property (1- (point)) 'mouse-face))
+        (unless (or (bobp) (get-text-property (1- (point)) 'completion--string))
           (goto-char (previous-single-property-change
-                      (point) 'mouse-face nil beg)))
+                      (point) 'completion--string nil beg)))
         ;; If at the first completion option, wrap or skip to the
         ;; minibuffer, if requested.
-        (setq prop (previous-single-property-change (point) 'mouse-face nil beg))
+        (setq prop (previous-single-property-change (point) 'completion--string nil beg))
         (when (and completion-wrap-movement (eq beg prop))
           (if (and completion-auto-select tabcommand)
               (progn
-                (goto-char (next-single-property-change (point) 'mouse-face nil end))
+                (goto-char (next-single-property-change (point) 'completion--string nil end))
                 (throw 'bound nil))
             (goto-char (point-max))))
         ;; Move to the start of that one.
         (goto-char (previous-single-property-change
-                    (point) 'mouse-face nil beg))
+                    (point) 'completion--string nil beg))
         (setq n (1+ n))))
     (when (/= 0 n)
       (switch-to-minibuffer))))
@@ -9620,13 +9625,14 @@ choose-completion
              (goto-char (posn-point (event-start event)))
              (let (beg)
                (cond
-                ((and (not (eobp)) (get-text-property (point) 'mouse-face))
+                ((and (not (eobp)) (get-text-property (point) 'completion--string))
                  (setq beg (1+ (point))))
                 ((and (not (bobp))
-                      (get-text-property (1- (point)) 'mouse-face))
+                      (get-text-property (1- (point)) 'completion--string))
                  (setq beg (point)))
                 (t (error "No completion here")))
-               (setq beg (previous-single-property-change beg 'mouse-face))
+               (setq beg (or (previous-single-property-change beg 'completion--string)
+                             beg))
                (substring-no-properties
                 (get-text-property beg 'completion--string))))))
 
@@ -9832,8 +9838,8 @@ switch-to-completions
        ((and (memq this-command '(completion-at-point minibuffer-complete))
              (equal (this-command-keys) [backtab]))
         (goto-char (point-max))
-        (previous-completion 1))
-       (t (next-completion 1))))))
+        (last-completion))
+       (t (first-completion))))))
 
 (defun read-expression-switch-to-completions ()
   "Select the completion list window while reading an expression."

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

forcemerge 54374 55289 55430
quit

> Here is the patch that fixes 4 bugs: 2 bugs described above,
> and also bug#55289 and bug#55430.

I pushed the test suite that will help to verify that these bugs
are correctly fixed.

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

close 54374 29.0.50
thanks

> Here is the patch that fixes 4 bugs: 2 bugs described above,
> and also bug#55289 and bug#55430.

Now all fixes with tests pushed to master.

bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Juri Linkov]

> (Note that this also fixes the "issue" jumping back to the beginning of
> the same completion option, if the point is not at the beginning of said
> option.)

Thanks for the fixes.  I pushed both patches with the same commit
(that also uses the suggested (not (bobp))).

bug#54374: [External] : bug#54374: 29.0.50; previous-completion fails at beginning of completions buffer

[Drew Adams]

> when point is at the beginning of the completion buffer,
> then previous-completion switches to the minibuffer.

Not good.  Keys that cycle among completions should
do exactly that.  And they should wrap around, at
least by default (with a user option to not do that).

If point is at bob then backward cycling should move
to the last completion.

> But if point is at the first completion, then previous-completion wraps
> to the last completion.  Shouldn't point at the beginning of the buffer
> wrap to the last completion as well?

Of course it should.  If point is there or at any
place before the first completion then backward
cycling should wrap around to the last completion.