bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

1. Start emacs -Q
2. M-x eshell
3. ls > #<*scratch*> TAB

The following error is observed:

Debugger entered--Lisp error: (wrong-type-argument stringp #<buffer *scratch*>)
  pcomplete-match("\\`--[^=]+=\\(.*\\)" 0)
  pcomplete-here-using-help("ls --help")
  pcomplete/ls()
  pcomplete-completions()
  pcomplete-completions-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-11 built on projects
Repository revision: f221105723dc8d3ee9f3c8d1c2717058afbc6666
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Configured using:
 'configure --prefix=/home/user/emacs/install --with-json
 --with-native-compilation --with-x-toolkit=athena --without-dbus
 --without-selinux --without-threads --without-gsettings --with-cairo'

Configured features:
CAIRO FREETYPE GIF GLIB GMP GNUTLS HARFBUZZ JPEG JSON LIBXML2 MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 TIFF
TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Debugger

Minor modes in effect:
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils help-fns
radix-tree cl-print byte-opt debug backtrace find-func pcmpl-unix
em-unix em-term term disp-table shell ehelp em-script em-prompt em-ls
em-hist em-pred em-glob em-extpipe em-cmpl em-dirs esh-var pcomplete
comint ansi-osc ansi-color ring em-basic em-banner em-alias esh-mode
eshell esh-cmd generator esh-ext esh-opt esh-proc esh-io esh-arg
esh-module esh-groups esh-util cus-edit pp cus-start cus-load wid-edit
cl-loaddefs comp comp-cstr warnings icons subr-x rx cl-seq cl-macs gv
cl-extra help-mode bytecomp byte-compile cl-lib files-x rmc iso-transl
tooltip cconv eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote inotify
dynamic-setting font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 134689 10796)
 (symbols 48 11140 0)
 (strings 32 32175 1832)
 (string-bytes 1 996479)
 (vectors 16 21968)
 (vector-slots 8 436129 17427)
 (floats 8 49 31)
 (intervals 56 529 0)
 (buffers 976 14))

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

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


>
> 1. Start emacs -Q
> 2. M-x eshell
> 3. ls > #<*scratch*> TAB
>

Thanks for your bug report.  Can you check that the attached patch fixes 
that bug, without introducing other obvious regressions?

[-- 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#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Augusto Stoffel]

This is essentially the same as bug#59956.  Note also that it's not
limited to completion rules defined by pcomplete-here-using-help.

On Mon, 12 Dec 2022 at 22:23, Daniel Mendler wrote:

> 1. Start emacs -Q
> 2. M-x eshell
> 3. ls > #<*scratch*> TAB
>
> The following error is observed:
>
> Debugger entered--Lisp error: (wrong-type-argument stringp #<buffer *scratch*>)
>   pcomplete-match("\\`--[^=]+=\\(.*\\)" 0)
>   pcomplete-here-using-help("ls --help")
>   pcomplete/ls()
>   pcomplete-completions()
>   pcomplete-completions-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-11 built on projects
> Repository revision: f221105723dc8d3ee9f3c8d1c2717058afbc6666
> Repository branch: emacs-29
> Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
> System Description: Debian GNU/Linux 10 (buster)
>
> Configured using:
>  'configure --prefix=/home/user/emacs/install --with-json
>  --with-native-compilation --with-x-toolkit=athena --without-dbus
>  --without-selinux --without-threads --without-gsettings --with-cairo'
>
> Configured features:
> CAIRO FREETYPE GIF GLIB GMP GNUTLS HARFBUZZ JPEG JSON LIBXML2 MODULES
> NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 TIFF
> TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM LUCID ZLIB
>
> Important settings:
>   value of $LANG: en_US.UTF-8
>   locale-coding-system: utf-8-unix
>
> Major mode: Debugger
>
> Minor modes in effect:
>   shell-dirtrack-mode: t
>   tooltip-mode: t
>   global-eldoc-mode: t
>   show-paren-mode: t
>   electric-indent-mode: t
>   mouse-wheel-mode: t
>   tool-bar-mode: t
>   menu-bar-mode: t
>   file-name-shadow-mode: t
>   global-font-lock-mode: t
>   font-lock-mode: t
>   blink-cursor-mode: t
>   buffer-read-only: t
>   line-number-mode: t
>   indent-tabs-mode: t
>   transient-mark-mode: t
>   auto-composition-mode: t
>   auto-encryption-mode: t
>   auto-compression-mode: t
>
> Load-path shadows:
> None found.
>
> Features:
> (shadow sort mail-extr emacsbug message mailcap yank-media puny dired
> dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
> epg-config gnus-util text-property-search time-date mm-decode mm-bodies
> mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
> rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils help-fns
> radix-tree cl-print byte-opt debug backtrace find-func pcmpl-unix
> em-unix em-term term disp-table shell ehelp em-script em-prompt em-ls
> em-hist em-pred em-glob em-extpipe em-cmpl em-dirs esh-var pcomplete
> comint ansi-osc ansi-color ring em-basic em-banner em-alias esh-mode
> eshell esh-cmd generator esh-ext esh-opt esh-proc esh-io esh-arg
> esh-module esh-groups esh-util cus-edit pp cus-start cus-load wid-edit
> cl-loaddefs comp comp-cstr warnings icons subr-x rx cl-seq cl-macs gv
> cl-extra help-mode bytecomp byte-compile cl-lib files-x rmc iso-transl
> tooltip cconv eldoc paren electric uniquify ediff-hook vc-hooks
> lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
> tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
> newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
> rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
> font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
> simple cl-generic indonesian philippine cham georgian utf-8-lang
> misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
> cp51932 hebrew greek romanian slovak czech european ethiopic indian
> cyrillic chinese composite emoji-zwj charscript charprop case-table
> epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
> loaddefs theme-loaddefs faces cus-face macroexp files window
> text-properties overlay sha1 md5 base64 format env code-pages mule
> custom widget keymap hashtable-print-readable backquote inotify
> dynamic-setting font-render-setting cairo x-toolkit xinput2 x multi-tty
> make-network-process native-compile emacs)
>
> Memory information:
> ((conses 16 134689 10796)
>  (symbols 48 11140 0)
>  (strings 32 32175 1832)
>  (string-bytes 1 996479)
>  (vectors 16 21968)
>  (vector-slots 8 436129 17427)
>  (floats 8 49 31)
>  (intervals 56 529 0)
>  (buffers 976 14))

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/12/22 23:41, Augusto Stoffel wrote:
> This is essentially the same as bug#59956.  Note also that it's not
> limited to completion rules defined by pcomplete-here-using-help.

Yes, but the last time we discussed you said that Pcomplete arguments
should only be strings, which seems clearly not to be the case. In
#59956 the argument is a list and in #60021 it is a buffer. But I agree
that it would have been better to attach this to the older report.

Daniel

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/12/22 23:24, Gregory Heytings wrote:
> Thanks for your bug report.  Can you check that the attached patch fixes 
> that bug, without introducing other obvious regressions?

Yes, in my quick tests it fixes both issues. But I am not familiar with
the code in question. Hard to tell if it introduces other regressions.
Maybe Augusto (cc'ed) could also take a look please?

Daniel

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

>> Thanks for your bug report.  Can you check that the attached patch 
>> fixes that bug, without introducing other obvious regressions?
>
> Yes, in my quick tests it fixes both issues. But I am not familiar with 
> the code in question. Hard to tell if it introduces other regressions. 
> Maybe Augusto (cc'ed) could also take a look please?
>

He already did, and although it fixes the two reported recipes, it's alas 
not a proper fix for that bug.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/13/22 00:34, Gregory Heytings wrote:
> 
>>> Thanks for your bug report.  Can you check that the attached patch 
>>> fixes that bug, without introducing other obvious regressions?
>>
>> Yes, in my quick tests it fixes both issues. But I am not familiar with 
>> the code in question. Hard to tell if it introduces other regressions. 
>> Maybe Augusto (cc'ed) could also take a look please?
>>
> 
> He already did, and although it fixes the two reported recipes, it's alas 
> not a proper fix for that bug.

Why is this not a proper fix? Is there some deep issue here? Is there a
chance that this could be fixed in a better way? I have never looked
deeply into the inner workings of Pcomplete, but from my experience the
Pcomplete Capf unfortunately is one of the buggiest Capfs provided by
Emacs. Nevertheless, I am a happy Eshell user. Completion in combination
with my Corfu UI works quite well, but I also employ some advices which
ignore errors in Pcomplete and prevent the Capf from modifying the buffer.

Daniel

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

>> He already did, and although it fixes the two reported recipes, it's 
>> alas not a proper fix for that bug.
>
> Why is this not a proper fix?
>

Try cvs $exec-path SPC TAB.

>
> Is there some deep issue here?
>

AFAIU, yes.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/13/22 14:50, Gregory Heytings wrote:
> 
>>> He already did, and although it fixes the two reported recipes, it's 
>>> alas not a proper fix for that bug.
>>
>> Why is this not a proper fix?
>>
> 
> Try cvs $exec-path SPC TAB.

I don't see what you mean. $exec-path is also expanded to a list, which
then again breaks pcomplete-here-using-help. But this just seems to be
the same issue as #59956.

Daniel

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

>> Try cvs $exec-path SPC TAB.
>
> I don't see what you mean. $exec-path is also expanded to a list, which 
> then again breaks pcomplete-here-using-help. But this just seems to be 
> the same issue as #59956.
>

I mean: try cvs $exec-path SPC TAB with the patch applied.  You'll see 
that the problem is still there.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

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


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

[-- Attachment #2: Handle-non-string-values-in-pcomplete.patch --]
[-- Type: text/x-diff, 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#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/19/22 23:24, Gregory Heytings wrote:
> Daniel, can you please try the attached patch and tell us if it fixes the 
> issue without introducing regressions?

Thanks for looking into this!

I inspected the patch and it seems like a reasonable fix for the
problem. I wonder about the introduction of the `pcomplete-arg-value'
string property. Is the introduction of this property necessary? Do you
have a use case for this right now?

If the recipes of #59956 and #60021 are fixed by this on your setup, I
trust it will work properly on my setup. No reason for additional
testing on my side.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

>> Daniel, can you please try the attached patch and tell us if it fixes 
>> the issue without introducing regressions?
>
> Thanks for looking into this!
>

Thanks for your reply!

>
> I inspected the patch and it seems like a reasonable fix for the 
> problem. I wonder about the introduction of the `pcomplete-arg-value' 
> string property. Is the introduction of this property necessary? Do you 
> have a use case for this right now?
>

Not right now, no.  It's a kind of compromise.  Previously pcomplete-args 
(and pcomplete.el in general) assumed that 
pcomplete-parse-argument-function would return strings, and only strings. 
When that's not the case, such as in eshell, it fails.  For example, with 
your recipe, when '#<*scratch*>' is parsed, a buffer object is returned. 
That value could have been transformed into a string with 'format', but 
that would mean that the original value has been lost (from the point of 
view of pcomplete).  The solution we found is to wrap that value in a 
string, the textual representation of the argument, namely the string 
"#<*scratch*>" in your recipe.  With that solution it becomes possible to 
design other pcomplete functions that would use that value.

>
> If the recipes of #59956 and #60021 are fixed by this on your setup, I 
> trust it will work properly on my setup. No reason for additional 
> testing on my side.
>

They are fixed, but I don't use eshell (and neither does Stefan M IIUC), 
so I may very well have missed some important other cases in which that 
fix would introduce a regression.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/20/22 09:55, Gregory Heytings wrote:
>> If the recipes of #59956 and #60021 are fixed by this on your setup, I 
>> trust it will work properly on my setup. No reason for additional 
>> testing on my side.
>>
> 
> They are fixed, but I don't use eshell (and neither does Stefan M IIUC), 
> so I may very well have missed some important other cases in which that 
> fix would introduce a regression.

I installed the patch and I will see if something comes up during my
normal Emacs usage.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

>>> If the recipes of #59956 and #60021 are fixed by this on your setup, I 
>>> trust it will work properly on my setup. No reason for additional 
>>> testing on my side.
>>
>> They are fixed, but I don't use eshell (and neither does Stefan M 
>> IIUC), so I may very well have missed some important other cases in 
>> which that fix would introduce a regression.
>
> I installed the patch and I will see if something comes up during my 
> normal Emacs usage.
>

Thanks.

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Gregory Heytings]

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

bug#60021: 29.0.60; Eshell completion errors in the presence of buffer redirects

[Daniel Mendler]

On 12/29/22 23:03, Gregory Heytings wrote:
> 
> No further comments in a week, pushed (dafa6d6bad) and closing.

Hello Gregory,

I am afraid but closing the bug was premature. Your patch dafa6d6bad is
broken, since it doesn't cater for the INDEX values 'last and 'first,
when you extract the buffer substring.

Daniel