bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Jim Porter]

X-Debbugs-CC: arstoffel@gmail.com

Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note the 
trailing space), and then hit TAB. The result is this error:

   pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)

This is a regression from Emacs 28, and it looks like it's due to 
'pcomplete-here-using-help' assuming that all the pcomplete args are 
strings. However, 'exec-path' is a list (and Eshell reports it this way 
to pcomplete), so the completion fails. I think all that's necessary is 
checking that the pcomplete args are strings in 
'pcomplete-here-using-help', but I know next to nothing about pcomplete...

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Eli Zaretskii]

> Cc: arstoffel@gmail.com
> Date: Sat, 10 Dec 2022 17:25:53 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> 
> X-Debbugs-CC: arstoffel@gmail.com
> 
> Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note the 
> trailing space), and then hit TAB. The result is this error:
> 
>    pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)
> 
> This is a regression from Emacs 28, and it looks like it's due to 
> 'pcomplete-here-using-help' assuming that all the pcomplete args are 
> strings. However, 'exec-path' is a list (and Eshell reports it this way 
> to pcomplete), so the completion fails. I think all that's necessary is 
> checking that the pcomplete args are strings in 
> 'pcomplete-here-using-help', but I know next to nothing about pcomplete...

Adding Stefan, who should know more about pcomplete.

Stefan, any suggestions?

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Augusto Stoffel]

On Sun, 11 Dec 2022 at 09:44, Eli Zaretskii wrote:

>> Cc: arstoffel@gmail.com
>> Date: Sat, 10 Dec 2022 17:25:53 -0800
>> From: Jim Porter <jporterbugs@gmail.com>
>> 
>> X-Debbugs-CC: arstoffel@gmail.com
>> 
>> Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note the 
>> trailing space), and then hit TAB. The result is this error:
>> 
>>    pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)
>> 
>> This is a regression from Emacs 28, and it looks like it's due to 
>> 'pcomplete-here-using-help' assuming that all the pcomplete args are 
>> strings. However, 'exec-path' is a list (and Eshell reports it this way 
>> to pcomplete), so the completion fails. I think all that's necessary is 
>> checking that the pcomplete args are strings in 
>> 'pcomplete-here-using-help', but I know next to nothing about pcomplete...
>
> Adding Stefan, who should know more about pcomplete.
>
> Stefan, any suggestions?

For the record, this problem afflicts just about every pcomplete rule
(except probably those meant specifically for eshell).  I just tried
`cvs $echo-path TAB` and I get the same error.

The issue here is what eshell returns via (pcomplete-arg ...) etc.
Currently it can be a list.  How about changing it to the string
"$echo-path", with the list sneaked in as a text property?

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

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

Hi John,

As the original designer of both Eshell and Pcomplete, what's your
opinion on the below:

>>> Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note the 
>>> trailing space), and then hit TAB. The result is this error:
>>> 
>>>    pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)
>>> 
>>> This is a regression from Emacs 28, and it looks like it's due to 
>>> 'pcomplete-here-using-help' assuming that all the pcomplete args are 
>>> strings. However, 'exec-path' is a list (and Eshell reports it this way 
>>> to pcomplete), so the completion fails. I think all that's necessary is 
>>> checking that the pcomplete args are strings in 
>>> 'pcomplete-here-using-help', but I know next to nothing about pcomplete...
>>
>> Adding Stefan, who should know more about pcomplete.
>> Stefan, any suggestions?
>
> For the record, this problem afflicts just about every pcomplete rule
> (except probably those meant specifically for eshell).  I just tried
> `cvs $echo-path TAB` and I get the same error.

Indeed, it seems to be a pervasive problem.

> The issue here is what eshell returns via (pcomplete-arg ...) etc.
> Currently it can be a list.  How about changing it to the string
> "$echo-path", with the list sneaked in as a text property?

From the point of view of Pcomplete, `pcomplete-arg` should indeed
always return a string, I think.  But should it be the string as it
stands in the buffer or the string *represented* by what's in
the buffer (i.e. after $ expansion and such)?
Should `pcomplete-arg` return nil in a case like `echo $nil`?

For "normal" shells, this is not an issue since they don't really
manipulate anything else than strings (or so we can pretend), but
obviously things aren't that simple for Eshell.


        Stefan

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

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


>> Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note the 
>> trailing space), and then hit TAB. The result is this error:
>>
>>    pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)
>>
>> This is a regression from Emacs 28, and it looks like it's due to 
>> 'pcomplete-here-using-help' assuming that all the pcomplete args are 
>> strings. However, 'exec-path' is a list (and Eshell reports it this way 
>> to pcomplete), so the completion fails. I think all that's necessary is 
>> checking that the pcomplete args are strings in 
>> 'pcomplete-here-using-help', but I know next to nothing about 
>> pcomplete...
>
> Adding Stefan, who should know more about pcomplete.
>
> Stefan, any suggestions?
>

I'm still not Stefan, but this bug is fixed by the attached patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Check-that-arguments-are-strings-in-pcomplete-here-u.patch --]
[-- Type: text/x-diff; name=Check-that-arguments-are-strings-in-pcomplete-here-u.patch, Size: 1661 bytes --]

From 60416d39cda3ebf336bdb77487d3b7d8d429f1b6 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 12 Dec 2022 22:16:18 +0000
Subject: [PATCH] Check that arguments are strings in pcomplete-here-using-help

* lisp/pcomplete.el (pcomplete-here-using-help): Check that
pcomplete-args are strings before applying string operations on
them.  Fixes bug#59956 and bug#60021.
---
 lisp/pcomplete.el | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 4e3a88bbda8..c63c490bcaa 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -1449,12 +1449,18 @@ pcomplete-here-using-help
 The switches are obtained by calling `pcomplete-from-help' with
 COMMAND and ARGS as arguments."
   (while (cond
-          ((string= "--" (pcomplete-arg 1))
+          ((let ((arg (pcomplete-arg 1)))
+             (if (stringp arg)
+                 (string= "--" arg)))
            (while (pcomplete-here (pcomplete-entries))))
-          ((pcomplete-match "\\`--[^=]+=\\(.*\\)" 0)
+          ((let ((arg (pcomplete-arg 0)))
+             (if (stringp arg)
+                 (pcomplete-match "\\`--[^=]+=\\(.*\\)" 0)))
            (pcomplete-here (pcomplete-entries)
                            (pcomplete-match-string 1 0)))
-          ((string-prefix-p "-" (pcomplete-arg 0))
+          ((let ((arg (pcomplete-arg 0)))
+             (if (stringp arg)
+                 (string-prefix-p "-" arg)))
            (pcomplete-here (apply #'pcomplete-from-help command args)))
           (t (pcomplete-here* (pcomplete-entries))))))
 
-- 
2.35.1

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Augusto Stoffel]

On Mon, 12 Dec 2022 at 22:21, Gregory Heytings wrote:

>>> Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note
>>> the trailing space), and then hit TAB. The result is this error:
>>>
>>>    pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)
>>>
>>> This is a regression from Emacs 28, and it looks like it's due to
>>> 'pcomplete-here-using-help' assuming that all the pcomplete args
>>> are strings. However, 'exec-path' is a list (and Eshell reports it
>>> this way to pcomplete), so the completion fails. I think all that's
>>> necessary is checking that the pcomplete args are strings in
>>> 'pcomplete-here-using-help', but I know next to nothing about
>>> pcomplete...
>>
>> Adding Stefan, who should know more about pcomplete.
>>
>> Stefan, any suggestions?
>>
>
> I'm still not Stefan, but this bug is fixed by the attached patch.

This is not a fix to the problem, in my opinion.  Just take any other
command with a pcomplete rule, say "cvs $exec-path SPC TAB" and you will
see the same problem.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

>>>> Starting from "emacs -Q -f -eshell", type "echo $exec-path " (note 
>>>> the trailing space), and then hit TAB. The result is this error:
>>>>
>>>>    pcomplete-match: Wrong type argument: stringp, ("/usr/bin" ...)
>>>>
>>>> This is a regression from Emacs 28, and it looks like it's due to 
>>>> 'pcomplete-here-using-help' assuming that all the pcomplete args are 
>>>> strings. However, 'exec-path' is a list (and Eshell reports it this 
>>>> way to pcomplete), so the completion fails. I think all that's 
>>>> necessary is checking that the pcomplete args are strings in 
>>>> 'pcomplete-here-using-help', but I know next to nothing about 
>>>> pcomplete...
>>>
>>> Adding Stefan, who should know more about pcomplete.
>>>
>>> Stefan, any suggestions?
>>
>> I'm still not Stefan, but this bug is fixed by the attached patch.
>
> This is not a fix to the problem, in my opinion.  Just take any other 
> command with a pcomplete rule, say "cvs $exec-path SPC TAB" and you will 
> see the same problem.
>

Agreed, it's not a proper fix for that bug.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Jim Porter]

On 12/12/2022 2:21 PM, Gregory Heytings wrote:
> I'm still not Stefan, but this bug is fixed by the attached patch.

Thanks. This is pretty close to what I was thinking (I tried almost the 
exact same patch locally, but wasn't sure if there was a better way). 
Personally, I think something like your patch would probably be the 
safest bet for the 29 branch.

Still, like Stefan and Augusto mentioned, there's probably a larger 
issue here: should Eshell be allowed to feed Pcomplete non-strings? 
Since Pcomplete was written for Eshell initially, there's some basis for 
why it *might* support non-string values, but actually requiring that is 
an awful lot to ask of every programmer who ever wants to write a 
pcomplete function.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Augusto Stoffel]

On Mon, 12 Dec 2022 at 23:27, Gregory Heytings wrote:

> Agreed, it's not a proper fix for that bug.

There are only 6 pcomplete/eshell/* functions in my Emacs.  It doesn't
look like the proper fix, which is to adjust eshell so that the
22-year-old docstring of `pcomplete-match' is not lying anymore, is a
lot of work.

But I'm not an eshell user and don't know its internals at all.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

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

> Still, like Stefan and Augusto mentioned, there's probably a larger issue
> here: should Eshell be allowed to feed Pcomplete non-strings? Since
> Pcomplete was written for Eshell initially, there's some basis for why it
> *might* support non-string values, but actually requiring that is an awful
> lot to ask of every programmer who ever wants to write a pcomplete function.

A "general" solution might be the one below, tho it looks more like
a general workaround, I think.

John?


        Stefan


diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 4e3a88bbda8..6a4d754b8c0 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -646,12 +646,14 @@ pcomplete-arg
 The OFFSET argument is added to/taken away from the index that will be
 used.  This is really only useful with `first' and `last', for
 accessing absolute argument positions."
-  (nth (+ (pcase index
-	   ('first 0)
-	   ('last  pcomplete-last)
-	   (_      (- pcomplete-index (or index 0))))
-	  (or offset 0))
-       pcomplete-args))
+  (let ((arg (nth (+ (pcase index
+	               ('first 0)
+	               ('last  pcomplete-last)
+	               (_      (- pcomplete-index (or index 0))))
+	             (or offset 0))
+	          pcomplete-args)))
+    (when arg
+      (if (stringp arg) arg (format "%S" arg)))))
 
 (defun pcomplete-begin (&optional index offset)
   "Return the beginning position of the INDEXth argument.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

>
> A "general" solution might be the one below
>

Why not (format "%s" (nth (+ (pcase ...))))?

>
> tho it looks more like a general workaround, I think.
>

Indeed, I think it would be better if pcomplete could see the unexpanded 
argument instead of its expansion.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

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

>> A "general" solution might be the one below
> Why not (format "%s" (nth (+ (pcase ...))))?

Because I prefer to confine the workaround to those rare cases where we
actually need it.

>> tho it looks more like a general workaround, I think.
> Indeed, I think it would be better if pcomplete could see the
> unexpanded argument instead of its expansion.

There's a tension here: we want the completion to operate on the actual
buffer text obviously, so in some places we definitely want to see the
"unexpanded argument" [1], but when it comes to looking at other arguments
to decide which completion table to use at point, it's often more useful
to see the expanded arguments (i.e. the thing that the command will
actually see).  E.g. if the previous arg is `$foo` which expands to `-u`
we'd probably prefer to see `-u` in order to know we should complete
against user names.


        Stefan


[1] And we have an "API bug" in this area where Pcomplete can't reliably
    figure out what the to-be-completed text actually is, BTW.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

>>> A "general" solution might be the one below
>>
>> Why not (format "%s" (nth (+ (pcase ...))))?
>
> Because I prefer to confine the workaround to those rare cases where we 
> actually need it.
>

I see, thanks!  And I agree that it's better.

>>> tho it looks more like a general workaround, I think.
>>
>> Indeed, I think it would be better if pcomplete could see the 
>> unexpanded argument instead of its expansion.
>
> There's a tension here: we want the completion to operate on the actual 
> buffer text obviously, so in some places we definitely want to see the 
> "unexpanded argument" [1], but when it comes to looking at other 
> arguments to decide which completion table to use at point, it's often 
> more useful to see the expanded arguments (i.e. the thing that the 
> command will actually see).  E.g. if the previous arg is `$foo` which 
> expands to `-u` we'd probably prefer to see `-u` in order to know we 
> should complete against user names.
>

Hmmm...  So I guess it would be better, instead of replacing lists (and in 
general non-strings) by their string representation, and instead of always 
looking at an unexpanded argument, to look at the unexpanded argument only 
when the expanded argument is not a string?  E.g. if $foo is "-u" we would 
use "-u", but if $foo is (b a r) we would use "$foo".

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Jim Porter]

On 12/18/2022 5:19 PM, Gregory Heytings wrote:
> Hmmm...  So I guess it would be better, instead of replacing lists (and 
> in general non-strings) by their string representation, and instead of 
> always looking at an unexpanded argument, to look at the unexpanded 
> argument only when the expanded argument is not a string?  E.g. if $foo 
> is "-u" we would use "-u", but if $foo is (b a r) we would use "$foo".

For external commands called from Eshell, the most-correct thing to do 
would be to take the list of all arguments, flatten it, and convert 
every element in the flattened list to a string. That's how Eshell will 
ultimately call the program.

The tricky bit in my mind is how to do that automatically for external 
commands, but to still allow a Pcomplete function for an Eshell command 
(i.e. a function named 'eshell/FOO') to have access to the "raw"[1] 
expanded form, even if it contains sublists, numbers, other objects, 
etc. Maybe we could just do the first thing above, and then think about 
adding a new API to Pcomplete that gets the "raw" expanded form for later...

[1] Is it really "raw" if variables have been expanded? Probably not. I 
can't think of a better name at the moment though.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

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

> Hmmm...  So I guess it would be better, instead of replacing lists (and in
> general non-strings) by their string representation, and instead of always
> looking at an unexpanded argument, to look at the unexpanded argument only
> when the expanded argument is not a string?  E.g. if $foo is "-u" we would
> use "-u", but if $foo is (b a r) we would use "$foo".

Sounds like just another way to work around the problem.
I think the only "real" fix is to change the API so the callers can say
what kind of data they're looking for.

The more I think about it, the more my workaround sounds appealing.
My main remaining question is whether there exist code out there that
relies on the current behavior.


        Stefan

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Augusto Stoffel]

On Sun, 18 Dec 2022 at 19:54, Stefan Monnier wrote:

> There's a tension here: we want the completion to operate on the actual
> buffer text obviously, so in some places we definitely want to see the
> "unexpanded argument" [1], but when it comes to looking at other arguments
> to decide which completion table to use at point, it's often more useful
> to see the expanded arguments (i.e. the thing that the command will
> actually see).  E.g. if the previous arg is `$foo` which expands to `-u`
> we'd probably prefer to see `-u` in order to know we should complete
> against user names.

If $foo is "-u -v"`, your patch will make pcomplete see this as a single
argument, no?

I think it makes sense to send the string "$foo" with the list '("-u"
"-v") embedded as text property, and add some convenience functions to
fetch the list for the pcomplete functions that can handle the more
refined information.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

>
> The more I think about it, the more my workaround sounds appealing.
>

What do you think of this (based on Augusto's suggestion)?  It seems to me 
that it's a more elegant workaround, with which further improvements 
become possible, which isn't the case if we squeeze the value into a 
string.


diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 4e3a88bbda8..2a2bbf114e0 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -645,13 +645,25 @@ pcomplete-arg

  The OFFSET argument is added to/taken away from the index that will be
  used.  This is really only useful with `first' and `last', for
-accessing absolute argument positions."
-  (nth (+ (pcase index
-          ('first 0)
-          ('last  pcomplete-last)
-          (_      (- pcomplete-index (or index 0))))
-         (or offset 0))
-       pcomplete-args))
+accessing absolute argument positions.
+
+When the INDEXth argument has been transformed into something
+that is not a string by `pcomplete-parse-arguments-function', the
+text representation of the argument is returned, and its value is
+stored in its pcomplete-arg-value text property."
+  (let ((arg
+         (nth (+ (pcase index
+                  ('first 0)
+                  ('last  pcomplete-last)
+                  (_      (- pcomplete-index (or index 0))))
+                (or offset 0))
+              pcomplete-args)))
+    (if (stringp arg)
+        arg
+      (propertize
+       (buffer-substring (pcomplete-begin index offset)
+                         (pcomplete-begin (1- (or index 0)) offset))
+       'pcomplete-arg-value arg))))

  (defun pcomplete-begin (&optional index offset)
    "Return the beginning position of the INDEXth argument.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

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

>> There's a tension here: we want the completion to operate on the actual
>> buffer text obviously, so in some places we definitely want to see the
>> "unexpanded argument" [1], but when it comes to looking at other arguments
>> to decide which completion table to use at point, it's often more useful
>> to see the expanded arguments (i.e. the thing that the command will
>> actually see).  E.g. if the previous arg is `$foo` which expands to `-u`
>> we'd probably prefer to see `-u` in order to know we should complete
>> against user names.
>
> If $foo is "-u -v"`, your patch will make pcomplete see this as a single
> argument, no?

I must admit that this gets into detailed semantics of Eshell with which
I'm not familiar, but AFAIK if $foo contains a string such as "-u -v",
then my patch won't have any effect at all.

More generally my patch doesn't change the number of arguments as seen
by `pcomplete-arg`.  IOW if $foo is turned into several arguments, it's
presumably done before those are turned into what Pcomplete sees as "the
list of arguments".

> I think it makes sense to send the string "$foo" with the list '("-u"
> "-v") embedded as text property, and add some convenience functions to
> fetch the list for the pcomplete functions that can handle the more
> refined information.

Could make sense, indeed.  I'd be interested to see what a patch for
that could look like, tho.


        Stefan

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

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

> What do you think of this (based on Augusto's suggestion)?  It seems to me
> that it's a more elegant workaround, with which further improvements become
> possible, which isn't the case if we squeeze the value into a string.

Sounds good to me,


        Stefan

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Jim Porter]

On 12/19/2022 2:31 AM, Augusto Stoffel wrote:
> If $foo is "-u -v"`, your patch will make pcomplete see this as a single
> argument, no?

For Eshell at least, that's what it should do:

   ~ $ setq foo "-AlF ."
   -AlF .
   ~/src/emacs/build $ ls $foo
   /usr/bin/ls: invalid option -- ' '
   Try '/usr/bin/ls --help' for more information.

Variable expansions in Eshell work more like they do in Zsh, where they 
*aren't* split into multiple arguments on word boundaries.

> I think it makes sense to send the string "$foo" with the list '("-u"
> "-v") embedded as text property, and add some convenience functions to
> fetch the list for the pcomplete functions that can handle the more
> refined information.

I think the best would be if code that invokes Pcomplete would tell 
Pcomplete the expanded forms of its arguments like it does now, but to 
additionally make sure that the Pcomplete function sees those arguments 
the way the actual command would. If Eshell provided a flattened list of 
strings to Pcomplete, then everything should Just Work, right?

However, I'm not sure how to do that *and* be able to provide the 
un-stringified version so that Pcomplete functions for built-in Eshell 
commands can access those arguments in their original (but still 
expanded) forms. Can Eshell know ahead of time what form to provide its 
arguments in? Should it just provide both and let the Pcomplete function 
pick its poison?

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

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


>
> Sounds good to me,
>

Thanks.

Augusto and Jim, can you please try the attached patch and tell us if it 
fixes the issue without introducing regressions?

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

From 5bee9d8bec42ddf38bb7e8d8f2a1b36adb96a506 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 19 Dec 2022 22:18:22 +0000
Subject: [PATCH] Handle non-string values in pcomplete

* lisp/pcomplete.el (pcomplete-arg): When
pcomplete-parse-arguments-function returns a non-string value,
return the string the user typed in, and attach the value as a
text property to that string.  Fixes bug#59956 and bug#60021.
---
 lisp/pcomplete.el | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 4e3a88bbda8..b1fbd88a377 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -645,13 +645,26 @@ pcomplete-arg
 
 The OFFSET argument is added to/taken away from the index that will be
 used.  This is really only useful with `first' and `last', for
-accessing absolute argument positions."
-  (nth (+ (pcase index
-	   ('first 0)
-	   ('last  pcomplete-last)
-	   (_      (- pcomplete-index (or index 0))))
-	  (or offset 0))
-       pcomplete-args))
+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."
+  (let ((arg
+         (nth (+ (pcase index
+	           ('first 0)
+	           ('last  pcomplete-last)
+	           (_      (- pcomplete-index (or index 0))))
+	         (or offset 0))
+              pcomplete-args)))
+    (if (stringp arg)
+        arg
+      (propertize
+       (buffer-substring (pcomplete-begin index offset)
+                         (pcomplete-begin (1- (or index 0)) offset))
+       'pcomplete-arg-value arg))))
 
 (defun pcomplete-begin (&optional index offset)
   "Return the beginning position of the INDEXth argument.
-- 
2.35.1

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Jim Porter]

On 12/19/2022 2:22 PM, Gregory Heytings wrote:
> Augusto and Jim, can you please try the attached patch and tell us if it 
> fixes the issue without introducing regressions?

It works for me after a bit of testing, thanks. I don't see any 
regressions on the Eshell side of things, at least.

While it would be nice to have a way for Eshell to provide multiple 
forms of its arguments for completion (one for external commands that 
expect a flat list of strings, and another for Eshell commands), that 
seems like a lot more work that would probably be better spent 
elsewhere. (And a change like that would be too big for the release 
branch anyway.)

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

>> Augusto and Jim, can you please try the attached patch and tell us if 
>> it fixes the issue without introducing regressions?
>
> It works for me after a bit of testing, thanks. I don't see any 
> regressions on the Eshell side of things, at least.
>

Thanks for your feedback!

>
> While it would be nice to have a way for Eshell to provide multiple 
> forms of its arguments for completion (one for external commands that 
> expect a flat list of strings, and another for Eshell commands), that 
> seems like a lot more work that would probably be better spent 
> elsewhere. (And a change like that would be too big for the release 
> branch anyway.)
>

Indeed.  There are two levels involved here: the pcomplete one and the 
eshell one.  The patch makes the pcomplete level more robust by ensuring 
that, even if pcomplete-parse-arguments-function does not return strings 
(which was an implicit assumption), pcomplete will continue to work.  On 
the eshell level, it would probably be better add a companion function to 
eshell-complete-parse-arguments (which is used for 
pcomplete-parse-arguments-function) that would return a proper string 
representation of the expanded arguments, and to use it for 
pcomplete-parse-arguments-function, instead of 
eshell-complete-parse-arguments.

bug#59956: 29.0.60: Failure when completing arguments in Eshell after variable interpolation

[Gregory Heytings]

No further comments in a week, pushed (dafa6d6bad) and closing.