bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

Follow up on bug#60021

1. M-x eshell
2. Enter `ls *.el` in a directory with Elisp files
3. Press TAB

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p last)
  (pcomplete-arg last)
  (pcomplete-parse-arguments nil)
  (pcomplete-completions)
  (pcomplete-completions-at-point)
  (#<subr completion--capf-wrapper> pcomplete-completions-at-point all)
  (corfu--capf-wrapper-advice #<subr completion--capf-wrapper> pcomplete-completions-at-point all)
  (apply corfu--capf-wrapper-advice #<subr completion--capf-wrapper> (pcomplete-completions-at-point all))
  (completion--capf-wrapper pcomplete-completions-at-point all)
  (completion-at-point)
  (funcall-interactively completion-at-point)
  (command-execute completion-at-point)

In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw scroll bars) of 2022-12-30 built on projects
Repository revision: d086cd6cf877c6ca7af6712f9b79b52dd0caa934
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>
> Follow up on bug#60021
>
> 1. M-x eshell
> 2. Enter `ls *.el` in a directory with Elisp files
> 3. Press TAB
>

Thanks for your bug report.  It was not necessary to open another bug 
report, I've seen your post in bug#60021 and am working on it.  Note that 
with 'ls *.el SPC TAB', which is something that is more likely to be used, 
there is no error.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/1/23 12:16, Gregory Heytings wrote:
> 
>>
>> Follow up on bug#60021
>>
>> 1. M-x eshell
>> 2. Enter `ls *.el` in a directory with Elisp files
>> 3. Press TAB
>>
> 
> Thanks for your bug report.  It was not necessary to open another bug 
> report, I've seen your post in bug#60021 and am working on it.  Note that 
> with 'ls *.el SPC TAB', which is something that is more likely to be used, 
> there is no error.

Okay, I wasn't sure if my other mail got lost. Anyway I wanted to attach
the additional stack trace. The problem occurs only if 'last is passed,
therefore the issue probably doesn't occur for `ls *.el SPC TAB`.

You may want to try my Corfu completion UI for testing, even if you
don't like popups for completion or auto completion. It is helpful for
debugging since the UI asks the completion table for completions very
often, in particular if one configures overly aggressive settings (not
recommended for real usage):

(setq corfu-auto-delay 0)
(setq corfu-auto-prefix 1)
(global-corfu-mode)

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>
> Okay, I wasn't sure if my other mail got lost. Anyway I wanted to attach 
> the additional stack trace. The problem occurs only if 'last is passed, 
> therefore the issue probably doesn't occur for `ls *.el SPC TAB`.
>

It also occurs if 'first is passed, in fact.  Try '$exec-path SPC TAB'.

>
> You may want to try my Corfu completion UI for testing, even if you 
> don't like popups for completion or auto completion. It is helpful for 
> debugging since the UI asks the completion table for completions very 
> often, in particular if one configures overly aggressive settings (not 
> recommended for real usage):
>

As I said in bug#59956 and bug#60021, I don't use Eshell, so I don't think 
using another completion UI would have helped.  That's why I asked for 
feedback, Jim and you both tried the patch and did not report problems.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/1/23 12:35, Gregory Heytings wrote:
>> Okay, I wasn't sure if my other mail got lost. Anyway I wanted to attach 
>> the additional stack trace. The problem occurs only if 'last is passed, 
>> therefore the issue probably doesn't occur for `ls *.el SPC TAB`.
>>
> 
> It also occurs if 'first is passed, in fact.  Try '$exec-path SPC TAB'.

Of course.

>> You may want to try my Corfu completion UI for testing, even if you 
>> don't like popups for completion or auto completion. It is helpful for 
>> debugging since the UI asks the completion table for completions very 
>> often, in particular if one configures overly aggressive settings (not 
>> recommended for real usage):
>>
> 
> As I said in bug#59956 and bug#60021, I don't use Eshell, so I don't think 
> using another completion UI would have helped.  That's why I asked for 
> feedback, Jim and you both tried the patch and did not report problems.

Actually I tried the patch only for a short time and then forgot about
it when recompiling Emacs the next time. I looked into the issue again
when you closed the bug report.

If you don't use Eshell, maybe you use Shell? Pcomplete issues should
also occur there. But even if you try Eshell/Shell for only a short
time, using Corfu would definitely help in its aggressive settings.
There is no doubt about that since I only found these issues thanks to
Corfu. With default completion you simply don't press TAB after each
other key.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>
> If you don't use Eshell, maybe you use Shell? Pcomplete issues should 
> also occur there.
>

They don't, or at least the specific issue of this bug doesn't, AFAICS. 
The pcomplete-parse-arguments-function in Shell is 
shell--parse-pcomplete-arguments, which only splits the command line into 
its whitespace-separated elements.  IOW, that function only returns 
strings.  The problem with Eshell, in which the 
pcomplete-parse-arguments-function is eshell-complete-parse-arguments, is 
that it returns non-strings.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

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


After working a bit more on this bug, I concluded that what Stefan 
initially suggested, to use the string representation of the value, is 
safer than trying to extract the string corresponding to the argument that 
the user typed in from the command line.

But that's not the end of the story.  The problem is that, IIUC, in 
Eshell, in a directory with .el files and without .EL files:

(1) ls *.el TAB should display all these files in *Completions*

(2) ls *.EL TAB should say "No match"

(3) ls *.el SPC TAB should display all files (not just the .el ones) in *Completions*

(4) ls *.EL SPC TAB should display all files (not just the .el ones) in *Completions*

IOW, sometimes pcomplete-arg should in fact return a list and not a single 
string value, because that's what Eshell expects (case (1) above), and 
sometimes it shouldn't.  According to my tests, a non-string value can be 
returned if and only if index is 'last'.  Hopefully, this is also what 
other users of pcomplete expect.

Can you please test the attached patch as extensively as possible, and 
report if you see regressions?

Jim, can you also try the patch and report if you see regressions?

Stefan and Eli, does that patch look right to you?

Thanks.

[-- Attachment #2: Further-improvement-for-non-string-values-in-pcomple.patch --]
[-- Type: text/x-diff, Size: 2027 bytes --]

From ee2877018b1cdb93f11fffc89b9dd4ed461fcc96 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sun, 1 Jan 2023 16:47:36 +0000
Subject: [PATCH] Further improvement for non-string values in pcomplete

* lisp/pcomplete.el (pcomplete-arg): Use the string representation
of the argument value instead of the text representation of the
argument.  Return the value, even when it is not a string, when
index is 'last'.  Fixes bug#60464.
---
 lisp/pcomplete.el | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 5bee515246..bce9aa5b4d 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -648,10 +648,10 @@ pcomplete-arg
 accessing absolute argument positions.
 
 When the argument has been transformed into something that is not
-a string by `pcomplete-parse-arguments-function', the text
-representation of the argument, namely what the user actually
-typed in, is returned, and the value of the argument is stored in
-the pcomplete-arg-value text property of that string."
+a string by `pcomplete-parse-arguments-function' and INDEX is not
+`last', the string representation of that value is returned, and
+the value is stored in the pcomplete-arg-value text property of
+that string."
   (let ((arg
          (nth (+ (pcase index
 	           ('first 0)
@@ -659,12 +659,10 @@ pcomplete-arg
 	           (_      (- pcomplete-index (or index 0))))
 	         (or offset 0))
               pcomplete-args)))
-    (if (stringp arg)
+    (if (or (stringp arg)
+            (eq index 'last))
         arg
-      (propertize
-       (buffer-substring (pcomplete-begin index offset)
-                         (pcomplete-begin (1- (or index 0)) offset))
-       'pcomplete-arg-value arg))))
+      (propertize (format "%S" arg) 'pcomplete-arg-value arg))))
 
 (defun pcomplete-begin (&optional index offset)
   "Return the beginning position of the INDEXth argument.
-- 
2.39.0

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/1/23 17:59, Gregory Heytings wrote:
> After working a bit more on this bug, I concluded that what Stefan 
> initially suggested, to use the string representation of the value, is 
> safer than trying to extract the string corresponding to the argument that 
> the user typed in from the command line.

This approach seems totally wrong to me. You now introduced a third
representation. pcomplete-arg returns the string representation (1) with
the value attached as second representation (2). For other scenarios it
returns the string argument itself from the command line (3).

Also the approach is pointless. Why would the caller of the function
want to get the string representation, given that the original value is
available as text property?

I suggest you go back to the way you implemented this before and return
the actual command line string with the value attached as text property.
I don't see why that should be less safe. It is just a matter of
determining the correct buffer boundaries.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>> After working a bit more on this bug, I concluded that what Stefan 
>> initially suggested, to use the string representation of the value, is 
>> safer than trying to extract the string corresponding to the argument 
>> that the user typed in from the command line.
>
> This approach seems totally wrong to me.
>

Feel free to suggest something else.

>
> You now introduced a third representation.
>

No, there are two representations: pcomplete-arg returns a string 
representation of the value, with that value attached to the string, when 
its caller does not expect a non-string value.

>
> Also the approach is pointless. Why would the caller of the function 
> want to get the string representation, given that the original value is 
> available as text property?
>

Because the pcomplete functions, in particular pcomplete-here-using-help, 
expect strings.

>
> I suggest you go back to the way you implemented this before and return 
> the actual command line string with the value attached as text property. 
> I don't see why that should be less safe. It is just a matter of 
> determining the correct buffer boundaries.
>

Perhaps I should have explained what I mean by "less safe": it is 
unexpectedly complex to compute the correct buffer boundaries in all 
cases.  If you don't believe me, try it yourself.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/1/23 18:28, Gregory Heytings wrote:
> 
>>> After working a bit more on this bug, I concluded that what Stefan 
>>> initially suggested, to use the string representation of the value, is 
>>> safer than trying to extract the string corresponding to the argument 
>>> that the user typed in from the command line.
>>
>> This approach seems totally wrong to me.
>>
> Feel free to suggest something else.

Yes, I did. The command line string should be returned.

>> Also the approach is pointless. Why would the caller of the function 
>> want to get the string representation, given that the original value is 
>> available as text property?
>>
> Because the pcomplete functions, in particular pcomplete-here-using-help, 
> expect strings.

Of course. The only reason is to return a value of the correct type. But
you then as well return "foo".

>> I suggest you go back to the way you implemented this before and return 
>> the actual command line string with the value attached as text property. 
>> I don't see why that should be less safe. It is just a matter of 
>> determining the correct buffer boundaries.
> 
> Perhaps I should have explained what I mean by "less safe": it is 
> unexpectedly complex to compute the correct buffer boundaries in all 
> cases.  If you don't believe me, try it yourself.

I believe you that doing this correctly is non trivial. But this doesn't
justify going with your hack.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>> Feel free to suggest something else.
>
> Yes, I did. The command line string should be returned.
>

I mean, to propose some other code.

>
> I believe you that doing this correctly is non trivial. But this doesn't 
> justify going with your hack.
>

It's extracting the correct part of the command line string that is hacky, 
too hacky to my taste.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I believe you that doing this correctly is non trivial. But this doesn't
>> justify going with your hack.
> It's extracting the correct part of the command line string that is hacky,
> too hacky to my taste.

I wouldn't be surprised if it's difficult/impossible, indeed.
But the question is: when/where does it matter whether we really return
the "correct" part?

In my experience, there are some fundamental mismatches in the Pcomplete
API which we can't fix without a serious redesign, so until we do that
we have to live with "best effort" :-(


        Stefan

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

Hi Gregory!

On 1/1/23 18:45, Gregory Heytings wrote:
> 
>>> Feel free to suggest something else.
>>
>> Yes, I did. The command line string should be returned.
>>
> 
> I mean, to propose some other code.
> 
>>
>> I believe you that doing this correctly is non trivial. But this doesn't 
>> justify going with your hack.
>>
> 
> It's extracting the correct part of the command line string that is hacky, 
> too hacky to my taste.

I only gave this a quick try and I don't make use of pcomplete-* helper
functions which may exist. This is just a best effort solution, which
assumes that we can split at spaces. If that's not the case we will get
wrong results, also in the presence of quotation.

But I believe that this is better than nothing and it should give a
sufficiently good result in most cases. We should acknowledge that
completion is not always perfect. It is okay to return a heuristical
result sometimes. If we observe bugs due to invalid return values we
could improve afterwards. I still consider this better than returning an
"arbitrary" string value as in your most recent proposal.

(defun pcomplete-arg (&optional index offset)
  (let* ((idx (+ (pcase index
                   ('first 0)
                   ('last  pcomplete-last)
                   (_      (- pcomplete-index (or index 0))))
                 (or offset 0)))
         (arg (nth idx pcomplete-args)))
    (if (stringp arg)
        arg
      (propertize
       (buffer-substring-no-properties
        (nth idx pcomplete-begins)
        (save-excursion
          (if (< idx pcomplete-last)
              (goto-char (nth (1+ idx) pcomplete-begins))
            (end-of-line))
          (if (re-search-backward "\\S-" nil t)
              (1+ (point))
            (pos-eol))))
       'pcomplete-arg-value arg))))

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Eli Zaretskii]

> Date: Sun, 01 Jan 2023 16:59:37 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 60464@debbugs.gnu.org
> 
> Stefan and Eli, does that patch look right to you?

I don't consider myself an expert on pcomplete, sorry.  I will defer
to Jim and Stefan.  (Although I must say that I'm not sure I see a bug
here.)

However, I do have two questions:

 . is this suggested for master or for the emacs-29 branch?
 . if the latter, then which past version of Emacs handled this correctly?

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>> Stefan and Eli, does that patch look right to you?
>
> I don't consider myself an expert on pcomplete, sorry.  I will defer to 
> Jim and Stefan.  (Although I must say that I'm not sure I see a bug 
> here.)
>
> However, I do have two questions:
>
> . is this suggested for master or for the emacs-29 branch?
> . if the latter, then which past version of Emacs handled this correctly?
>

It's a sequel of dafa6d6bad, so it should probably go to emacs-29.  I 
can't really answer your second question with 100% accuracy, but I believe 
Emacs 28 behaved correctly in the cases discussed in this bug and the 
earlier ones (bug#59956 and bug#60021).

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Eli Zaretskii]

> Date: Sun, 01 Jan 2023 18:29:53 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: mail@daniel-mendler.de, jporterbugs@gmail.com, monnier@iro.umontreal.ca, 
>     60464@debbugs.gnu.org
> 
> 
> >> Stefan and Eli, does that patch look right to you?
> >
> > I don't consider myself an expert on pcomplete, sorry.  I will defer to 
> > Jim and Stefan.  (Although I must say that I'm not sure I see a bug 
> > here.)
> >
> > However, I do have two questions:
> >
> > . is this suggested for master or for the emacs-29 branch?
> > . if the latter, then which past version of Emacs handled this correctly?
> >
> 
> It's a sequel of dafa6d6bad, so it should probably go to emacs-29.  I 
> can't really answer your second question with 100% accuracy, but I believe 
> Emacs 28 behaved correctly in the cases discussed in this bug and the 
> earlier ones (bug#59956 and bug#60021).

Thanks.  In that case, I'll withhold my opinion until I see the final
variant of the proposed patch.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> After working a bit more on this bug, I concluded that what Stefan initially
> suggested, to use the string representation of the value, is safer than
> trying to extract the string corresponding to the argument that the user
> typed in from the command line.

Could you expand on when/where it's "unsafe" or what it breaks?


        Stefan

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>> After working a bit more on this bug, I concluded that what Stefan 
>> initially suggested, to use the string representation of the value, is 
>> safer than trying to extract the string corresponding to the argument 
>> that the user typed in from the command line.
>
> Could you expand on when/where it's "unsafe" or what it breaks?
>

I simply spent too much time trying to get the "extract the correct part 
of the command line from the buffer" right, and wasn't able to convince 
myself that the result was correct in all circumstances.  Given that 
returning (format "%S" arg) is what you initially suggested, and that it 
cannot be wrong, I concluded that it was the best/safest thing to do.

The semantics of the "index" argument of the pcomplete-arg function are 
tricky: it can be 0, "the current argument being examined", < 0, "closer 
to the last argument", and > 0, "closer to the first argument".  And then 
you also have the special values 'first and 'last.  And it can also be 
nil, which is equivalent to 0.

There is a pcomplete-actual-arg function, which returns "the actual text 
representation of the last argument" (in fact, "the actual text 
representation of the INDEXth argument and the following ones"), but no 
function which returns the actual text representation of a given argument.

Perhaps we could just use it and assume that all arguments are separated 
by spaces, though, in which case the patch would become:

diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 5bee515246..c829b6c3b7 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -648,10 +648,11 @@ pcomplete-arg
  accessing absolute argument positions.

  When the argument has been transformed into something that is not
-a string by `pcomplete-parse-arguments-function', the text
-representation of the argument, namely what the user actually
-typed in, is returned, and the value of the argument is stored in
-the pcomplete-arg-value text property of that string."
+a string by `pcomplete-parse-arguments-function' and INDEX is not
+`last', the text representation of the argument, namely what the
+user actually typed in, is returned, and the value of the
+argument is stored in the pcomplete-arg-value text property of
+that string."
    (let ((arg
           (nth (+ (pcase index
                    ('first 0)
@@ -659,11 +660,11 @@ pcomplete-arg
                    (_      (- pcomplete-index (or index 0))))
                  (or offset 0))
                pcomplete-args)))
-    (if (stringp arg)
+    (if (or (stringp arg)
+            (eq index 'last))
          arg
        (propertize
-       (buffer-substring (pcomplete-begin index offset)
-                         (pcomplete-begin (1- (or index 0)) offset))
+       (car (split-string (pcomplete-actual-arg index offset)))
         'pcomplete-arg-value arg))))

  (defun pcomplete-begin (&optional index offset)

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>    (let ((arg
>           (nth (+ (pcase index
>                    ('first 0)
> @@ -659,11 +660,11 @@ pcomplete-arg
>                    (_      (- pcomplete-index (or index 0))))
>                  (or offset 0))
>                pcomplete-args)))
> -    (if (stringp arg)
> +    (if (or (stringp arg)
> +            (eq index 'last))
>          arg
>        (propertize
> -       (buffer-substring (pcomplete-begin index offset)
> -                         (pcomplete-begin (1- (or index 0)) offset))
> +       (car (split-string (pcomplete-actual-arg index offset)))
>         'pcomplete-arg-value arg))))

I'm not sure what specific problem this is trying to solve (is it the
choice of the "index" or is it the precise buffer positions of the
bounds)?

For the first, would the patch below help?
[ For the second, I suspect we can't provide 100% reliably correct
  information anyway, so I think we'll need to find concrete usecases
  where it matters before we can judge how much effort is warranted.  ]


        Stefan


diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 2d3730e294a..815ad252fbd 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -1,6 +1,6 @@
 ;;; pcomplete.el --- programmable completion -*- lexical-binding: t -*-
 
-;; Copyright (C) 1999-2022 Free Software Foundation, Inc.
+;; Copyright (C) 1999-2023 Free Software Foundation, Inc.
 
 ;; Author: John Wiegley <johnw@gnu.org>
 ;; Keywords: processes abbrev
@@ -652,13 +652,12 @@ pcomplete-arg
 representation of the argument, namely what the user actually
 typed in, is returned, and the value of the argument is stored in
 the pcomplete-arg-value text property of that string."
-  (let ((arg
-         (nth (+ (pcase index
+  (let* ((index (+ (pcase index
 	           ('first 0)
 	           ('last  pcomplete-last)
 	           (_      (- pcomplete-index (or index 0))))
-	         (or offset 0))
-              pcomplete-args)))
+	         (or offset 0)))
+         (arg (nth index pcomplete-args)))
     (if (stringp arg)
         arg
       (propertize

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> For the first, would the patch below help?

Well, obviously not because I didn't bother to look at the rest of
the code.

Maybe this one would be closer, tho it probably needs some handling for
the boundary case where `index` is the last.


        Stefan


diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 2d3730e294a..36968d3b73c 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -1,6 +1,6 @@
 ;;; pcomplete.el --- programmable completion -*- lexical-binding: t -*-
 
-;; Copyright (C) 1999-2022 Free Software Foundation, Inc.
+;; Copyright (C) 1999-2023 Free Software Foundation, Inc.
 
 ;; Author: John Wiegley <johnw@gnu.org>
 ;; Keywords: processes abbrev
@@ -652,18 +652,17 @@ pcomplete-arg
 representation of the argument, namely what the user actually
 typed in, is returned, and the value of the argument is stored in
 the pcomplete-arg-value text property of that string."
-  (let ((arg
-         (nth (+ (pcase index
+  (let* ((index (+ (pcase index
 	           ('first 0)
 	           ('last  pcomplete-last)
 	           (_      (- pcomplete-index (or index 0))))
-	         (or offset 0))
-              pcomplete-args)))
+	         (or offset 0)))
+         (arg (nth index pcomplete-args)))
     (if (stringp arg)
         arg
       (propertize
-       (buffer-substring (pcomplete-begin index offset)
-                         (pcomplete-begin (1- (or index 0)) offset))
+       (buffer-substring (nth index pcomplete-begins)
+                         (nth (1+ index) pcomplete-begins))
        'pcomplete-arg-value arg))))
 
 (defun pcomplete-begin (&optional index offset)

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>>                    (_      (- pcomplete-index (or index 0))))
>>                  (or offset 0))
>>                pcomplete-args)))
>> -    (if (stringp arg)
>> +    (if (or (stringp arg)
>> +            (eq index 'last))
>>          arg
>>        (propertize
>> -       (buffer-substring (pcomplete-begin index offset)
>> -                         (pcomplete-begin (1- (or index 0)) offset))
>> +       (car (split-string (pcomplete-actual-arg index offset)))
>>         'pcomplete-arg-value arg))))
>
> I'm not sure what specific problem this is trying to solve (is it the 
> choice of the "index" or is it the precise buffer positions of the 
> bounds)?
>

I admit I don't understand your question.  It tries to use 
pcomplete-actual-arg, which uses buffer-substring, to get the text 
representation of the argument.  It's not guaranteed to work in all cases, 
though, because it simply assumes that arguments are separated by spaces. 
But it should be "good enough".

The (eq index 'last) means that when index is 'last we return the 
argument, even when it is not a string (or more precisely: when it is a 
list of strings, and the code assumes that a non-string arg is a list of 
string), because that's in fact what the caller expects (or at least 
that's what Eshell expects) in that case: that list is displayed in 
*Completions*.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Jim Porter]

On 1/1/2023 8:59 AM, Gregory Heytings wrote:
> Jim, can you also try the patch and report if you see regressions?

Thanks. I tried this out for a bit, and I haven't been able to break it 
yet (although I didn't notice the issue that spawned this bug# the last 
time either, so it's possible I'm missing some problem this time around, 
too).

I think it would be super-useful to collect some of these test cases for 
Pcomplete-in-Eshell and write ERT tests for them. Then we can be more 
confident that future changes to Pcomplete won't regress Eshell. I'll 
look into doing that in the next week or two.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

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


>> Jim, can you also try the patch and report if you see regressions?
>
> Thanks. I tried this out for a bit, and I haven't been able to break it 
> yet (although I didn't notice the issue that spawned this bug# the last 
> time either, so it's possible I'm missing some problem this time around, 
> too).
>

Thanks for your confirmation.

Stefan, do you agree with the attached patch?  Or would you like something 
else?

Eli, are you okay with that patch?

The difference between the code before dafa6d6bad and the code with this 
patch applied can be summarized as follows:

  (defun pcomplete-arg (&optional index offset)
    (let ((arg
           (nth (+ (pcase index
  	           ('first 0)
  	           ('last  pcomplete-last)
  	           (_      (- pcomplete-index (or index 0))))
  	         (or offset 0))
                pcomplete-args)))
-    arg
+    (if (or (stringp arg)
+            (eq index 'last))
+        arg
+      (propertize
+       (car (split-string (pcomplete-actual-arg index offset)))
+       'pcomplete-arg-value arg))))

IOW, instead of unconditionally returning arg, we now sometimes (when arg 
is not already a string and when index is not last) return the textual 
representation of that argument (what the user typed in) with the value of 
arg attached to it.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Further-improvement-for-non-string-values-in-pcomple.patch --]
[-- Type: text/x-diff; name=Further-improvement-for-non-string-values-in-pcomple.patch, Size: 3330 bytes --]

From e5e197a68a25315e93586db299757911862e74a8 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 4 Jan 2023 13:36:04 +0000
Subject: [PATCH] Further improvement for non-string values in pcomplete

* lisp/pcomplete.el (pcomplete-arg): Use the string representation
of the argument value instead of the text representation of the
argument.  Return the value, even when it is not a string, when
index is 'last'.  Fixes bug#60464.
(pcomplete-actual-arg): Move it before 'pcomplete-arg'.
---
 lisp/pcomplete.el | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 5bee515246..73e14c562b 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -632,6 +632,13 @@ pcomplete-list
 ;;; Internal Functions:
 
 ;; argument handling
+(defsubst pcomplete-actual-arg (&optional index offset)
+  "Return the actual text representation of the last argument.
+This is different from `pcomplete-arg', which returns the textual value
+that the last argument evaluated to.  This function returns what the
+user actually typed in."
+  (buffer-substring (pcomplete-begin index offset) (point)))
+
 (defun pcomplete-arg (&optional index offset)
   "Return the textual content of the INDEXth argument.
 INDEX is based from the current processing position.  If INDEX is
@@ -648,10 +655,11 @@ pcomplete-arg
 accessing absolute argument positions.
 
 When the argument has been transformed into something that is not
-a string by `pcomplete-parse-arguments-function', the text
-representation of the argument, namely what the user actually
-typed in, is returned, and the value of the argument is stored in
-the pcomplete-arg-value text property of that string."
+a string by `pcomplete-parse-arguments-function' and INDEX is not
+`last', the text representation of the argument, namely what the
+user actually typed in, is returned, and the value of the
+argument is stored in the pcomplete-arg-value text property of
+that string."
   (let ((arg
          (nth (+ (pcase index
 	           ('first 0)
@@ -659,11 +667,11 @@ pcomplete-arg
 	           (_      (- pcomplete-index (or index 0))))
 	         (or offset 0))
               pcomplete-args)))
-    (if (stringp arg)
+    (if (or (stringp arg)
+            (eq index 'last))
         arg
       (propertize
-       (buffer-substring (pcomplete-begin index offset)
-                         (pcomplete-begin (1- (or index 0)) offset))
+       (car (split-string (pcomplete-actual-arg index offset)))
        'pcomplete-arg-value arg))))
 
 (defun pcomplete-begin (&optional index offset)
@@ -679,13 +687,6 @@ pcomplete-begin
       (setq index (+ index offset)))
   (nth index pcomplete-begins))
 
-(defsubst pcomplete-actual-arg (&optional index offset)
-  "Return the actual text representation of the last argument.
-This is different from `pcomplete-arg', which returns the textual value
-that the last argument evaluated to.  This function returns what the
-user actually typed in."
-  (buffer-substring (pcomplete-begin index offset) (point)))
-
 (defsubst pcomplete-next-arg ()
   "Move the various pointers to the next argument."
   (setq pcomplete-index (1+ pcomplete-index)
-- 
2.39.0

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>  (defun pcomplete-arg (&optional index offset)
>    (let ((arg
>           (nth (+ (pcase index
>  	           ('first 0)
>  	           ('last  pcomplete-last)
>  	           (_      (- pcomplete-index (or index 0))))
>  	         (or offset 0))
>                pcomplete-args)))
> -    arg
> +    (if (or (stringp arg)
> +            (eq index 'last))
> +        arg
> +      (propertize
> +       (car (split-string (pcomplete-actual-arg index offset)))
> +       'pcomplete-arg-value arg))))
>
> IOW, instead of unconditionally returning arg, we now sometimes (when arg is
> not already a string and when index is not last) return the textual
> representation of that argument (what the user typed in) with the value of
> arg attached to it.

I don't understand the `last` condition.
Could you explain it (e.g. with a concrete example that breaks if you
don't include it)?


        Stefan

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/4/23 15:25, Stefan Monnier wrote:
>>  (defun pcomplete-arg (&optional index offset)
>>    (let ((arg
>>           (nth (+ (pcase index
>>  	           ('first 0)
>>  	           ('last  pcomplete-last)
>>  	           (_      (- pcomplete-index (or index 0))))
>>  	         (or offset 0))
>>                pcomplete-args)))
>> -    arg
>> +    (if (or (stringp arg)
>> +            (eq index 'last))
>> +        arg
>> +      (propertize
>> +       (car (split-string (pcomplete-actual-arg index offset)))
>> +       'pcomplete-arg-value arg))))
>>
>> IOW, instead of unconditionally returning arg, we now sometimes (when arg is
>> not already a string and when index is not last) return the textual
>> representation of that argument (what the user typed in) with the value of
>> arg attached to it.
> 
> I don't understand the `last` condition.
> Could you explain it (e.g. with a concrete example that breaks if you
> don't include it)?

This seems like its opening the door for yet another bug. Gregory, could
you please take a look at the version of pcomplete-arg that I've sent?

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>
> This seems like its opening the door for yet another bug.
>

That's always possible, of course.  Jim tested it extensively.  Can you do 
the same?

>
> Gregory, could you please take a look at the version of pcomplete-arg 
> that I've sent?
>

I did.  It's not better (and in fact much more bug-prone) than reusing 
pcomplete-actual-arg.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/4/23 15:46, Gregory Heytings wrote:
>> Gregory, could you please take a look at the version of pcomplete-arg 
>> that I've sent?
>
> I did.  It's not better (and in fact much more bug-prone) than reusing 
> pcomplete-actual-arg.

Maybe, but it works well for me. Your implementation with the 'last
condition doesn't make sense. It is not a good idea to treat 'last
specially such that we return a different type in this case. This is
more bug-prone for the caller.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Eli Zaretskii]

> Date: Wed, 4 Jan 2023 16:14:14 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  Jim Porter <jporterbugs@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>  60464@debbugs.gnu.org
> From: Daniel Mendler <mail@daniel-mendler.de>
> 
> Your implementation with the 'last condition doesn't make sense.

"Doesn't make sense" is a harsh judgment of someone's code, and I have
hard time believing Gregory's deserves that.  Please try to be kinder
in how you express your disagreements with someone else's code.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/4/23 16:18, Eli Zaretskii wrote:
>> Date: Wed, 4 Jan 2023 16:14:14 +0100
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>>  Jim Porter <jporterbugs@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>>  60464@debbugs.gnu.org
>> From: Daniel Mendler <mail@daniel-mendler.de>
>>
>> Your implementation with the 'last condition doesn't make sense.
> 
> "Doesn't make sense" is a harsh judgment of someone's code, and I have
> hard time believing Gregory's deserves that.  Please try to be kinder
> in how you express your disagreements with someone else's code.

I try to formulate it more fairly. In my opinion Gregory's patches so
far did not fix the issue in a way which is future-proof. The first
prematurely installed patch was obviously broken. The current proposal
introduces an unexpected special casing, where it would be better if the
function behaves uniformly for all arguments.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>
> Your implementation with the 'last condition doesn't make sense.
>

Can you please try to make it fail?

>
> It is not a good idea to treat 'last specially such that we return a
> different type in this case.
>

I'm not the one who wrote that code.  According to my analysis of the 
code, the only place where pcomplete-arg is called with a 'last' argument 
is pcomplete-parse-arguments, where you will see the following:

(defun pcomplete-parse-arguments (&optional expand-p)
   ...
   (let ((results (funcall pcomplete-parse-arguments-function)))
     (when results
       (setq ...
 	    pcomplete-stub (pcomplete-arg 'last))
       (let ...
 	(if (and (listp pcomplete-stub) ;??
 		 (not pcomplete-expand-only-p))
 	    ;; If `pcomplete-stub' is a list, it means it's a list of
             ;; completions computed during parsing, e.g. Eshell uses
             ;; that to turn globs into lists of completions.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/4/23 16:39, Gregory Heytings wrote:
>> Your implementation with the 'last condition doesn't make sense.
> 
> Can you please try to make it fail?

That was not my point. The way the function behaves now is problematic
on theoretical grounds.

>> It is not a good idea to treat 'last specially such that we return a
>> different type in this case.
>>
> 
> I'm not the one who wrote that code.  According to my analysis of the 
> code, the only place where pcomplete-arg is called with a 'last' argument 
> is pcomplete-parse-arguments, where you will see the following:

I am not blaming you for it. If your analysis is right (and there are
not many or even only a single caller), what about deprecating support
for 'last and 'first then, if these lead to problems? Stefan said before
that Pcomplete needs some refactoring and fixing so one could do it in
small steps. We could then fix the few call sites which are there. But
such changes may be too late for Emacs 29.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Gregory Heytings [2023-01-04 15:39:26] wrote:
>> Your implementation with the 'last condition doesn't make sense.
> Can you please try to make it fail?
>> It is not a good idea to treat 'last specially such that we return a
>> different type in this case.
> I'm not the one who wrote that code.  According to my analysis of the code,
> the only place where pcomplete-arg is called with a 'last' argument is
> pcomplete-parse-arguments, where you will see the following:
>
> (defun pcomplete-parse-arguments (&optional expand-p)
>   ...
>   (let ((results (funcall pcomplete-parse-arguments-function)))
>     (when results
>       (setq ...
> 	    pcomplete-stub (pcomplete-arg 'last))
>       (let ...
> 	(if (and (listp pcomplete-stub) ;??
> 		 (not pcomplete-expand-only-p))
> 	    ;; If `pcomplete-stub' is a list, it means it's a list of
>             ;; completions computed during parsing, e.g. Eshell uses
>             ;; that to turn globs into lists of completions.

That's also my understanding.  So I think The Right Fix (or at least The
Better Fix) is to pay no special attention to `last` in `pcomplete-arg`
and instead in the above code of `pcomplete-parse-arguments` to look for
the `pcomplete-arg-value` property.

Maybe for `emacs-29` we can use your patch (with a comment about why
`last` is handled specially pointing to its handling in
`pcomplete-parse-arguments`) and then in `master` we remove this special
handling of `last`?


        Stefan

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>> I'm not the one who wrote that code.  According to my analysis of the 
>> code, the only place where pcomplete-arg is called with a 'last' 
>> argument is pcomplete-parse-arguments, where you will see the 
>> following:
>>
>> (defun pcomplete-parse-arguments (&optional expand-p)
>>   ...
>>   (let ((results (funcall pcomplete-parse-arguments-function)))
>>     (when results
>>       (setq ...
>> 	    pcomplete-stub (pcomplete-arg 'last))
>>       (let ...
>> 	(if (and (listp pcomplete-stub) ;??
>> 		 (not pcomplete-expand-only-p))
>> 	    ;; If `pcomplete-stub' is a list, it means it's a list of
>>             ;; completions computed during parsing, e.g. Eshell uses
>>             ;; that to turn globs into lists of completions.
>
> That's also my understanding.  So I think The Right Fix (or at least The 
> Better Fix) is to pay no special attention to `last` in `pcomplete-arg` 
> and instead in the above code of `pcomplete-parse-arguments` to look for 
> the `pcomplete-arg-value` property.
>

That would be even better, indeed.  But it would be a larger change.

>
> Maybe for `emacs-29` we can use your patch (with a comment about why 
> `last` is handled specially pointing to its handling in 
> `pcomplete-parse-arguments`) and then in `master` we remove this special 
> handling of `last`?
>

I'm fine with that.

Unless someone objects, I'll push that patch in a day or two, and I'll do 
that change on master afterwards.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>
> Unless someone objects, I'll push that patch in a day or two
>

Now done (72c45fa910).

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Jim Porter]

On 1/4/2023 7:56 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Maybe for `emacs-29` we can use your patch (with a comment about why
> `last` is handled specially pointing to its handling in
> `pcomplete-parse-arguments`) and then in `master` we remove this special
> handling of `last`?

I'd definitely support a minimal fix for the 29 branch, and then we can 
come up with something more thorough for master. It's possible (maybe 
even likely) that we need to make some changes on the Eshell side too.

Once I get some ERT tests written up for Pcomplete-from-Eshell, it 
should hopefully make it easier to try out new implementations and make 
sure it doesn't regress anything.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Jim Porter]

On 1/4/2023 6:46 AM, Gregory Heytings wrote:
> 
>>
>> This seems like its opening the door for yet another bug.
>>
> 
> That's always possible, of course.  Jim tested it extensively.  Can you 
> do the same?

Well, let's not say "extensively". :) I tested the small set of things I 
could think of that would be likely to break here. However, since I know 
very little about Pcomplete, my testing was mostly just "try a few 
different Eshell constructs and make sure no errors get signaled".

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Gregory Heytings]

>> IOW, instead of unconditionally returning arg, we now sometimes (when 
>> arg is not already a string and when index is not last) return the 
>> textual representation of that argument (what the user typed in) with 
>> the value of arg attached to it.
>
> I don't understand the `last` condition.
>
> Could you explain it (e.g. with a concrete example that breaks if you 
> don't include it)?
>

Yes: when index is 'last' returning a non-string (more precisely: a list 
of strings) is allowed (and in fact expected, at least by Eshell).  A 
recipe, in the Emacs repository:

M-x eshell
cd lisp
ls *.el TAB

(Note that there is no SPC before TAB.)

Without the 'last' condition, you don't get what you're supposed to get at 
that point (and what you got with Emacs 27 or 28), namely the list of .el 
files in *Completions*.  Instead you see "Complete, but not unique", and 
you have to press TAB a second time to see the completions.

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Daniel Mendler]

On 1/4/23 15:36, Gregory Heytings wrote:
> 
>>> IOW, instead of unconditionally returning arg, we now sometimes (when 
>>> arg is not already a string and when index is not last) return the 
>>> textual representation of that argument (what the user typed in) with 
>>> the value of arg attached to it.
>>
>> I don't understand the `last` condition.
>>
>> Could you explain it (e.g. with a concrete example that breaks if you 
>> don't include it)?
>>
> 
> Yes: when index is 'last' returning a non-string (more precisely: a list 
> of strings) is allowed (and in fact expected, at least by Eshell).  A 
> recipe, in the Emacs repository:
> 
> M-x eshell
> cd lisp
> ls *.el TAB
> 
> (Note that there is no SPC before TAB.)
> 
> Without the 'last' condition, you don't get what you're supposed to get at 
> that point (and what you got with Emacs 27 or 28), namely the list of .el 
> files in *Completions*.  Instead you see "Complete, but not unique", and 
> you have to press TAB a second time to see the completions.

Don't you think that this is by accident? Shouldn't Eshell fixed instead
to not expect a list in this case? I believe it would be better if
pcomplete-arg behaves uniformly for all arguments.

Daniel

bug#60464: 29.0.60; Regression - pcomplete-arg fails with argument 'last

[Eli Zaretskii]

> Date: Wed, 04 Jan 2023 13:48:25 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Daniel Mendler <mail@daniel-mendler.de>, 
>     Stefan Monnier <monnier@iro.umontreal.ca>, Eli Zaretskii <eliz@gnu.org>, 
>     60464@debbugs.gnu.org
> 
> > Thanks. I tried this out for a bit, and I haven't been able to break it 
> > yet (although I didn't notice the issue that spawned this bug# the last 
> > time either, so it's possible I'm missing some problem this time around, 
> > too).
> >
> 
> Thanks for your confirmation.
> 
> Stefan, do you agree with the attached patch?  Or would you like something 
> else?
> 
> Eli, are you okay with that patch?

Yes, provided that Stefan is okay with it.