bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Jim Porter]

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

Starting from "emacs -Q -f eshell":

   ~ $ echo $exec-path[0]
   /usr/local/sbin
   ~ $ echo $exec-path[${echo 0}]
   /usr/local/sbin
   ~ $ echo $exec-path[${*echo 0}]
   ;; no output

This is because 'eshell-eval-indices' gets an S-expr describing code to 
evaluate for the indices, and it just passes that to 'eval'. That's not 
the right way to do things for Eshell: instead, we should rely on 
'eshell-do-eval', which properly handles asynchronous evaluation. That's 
required for working with external commands like "*echo" (which calls 
the real /bin/echo).

The attached patch fixes this by changing 'eshell-eval-indices' to 
'eshell-indices', which does some minimal transformations on the S-expr 
for the indices, and then uses it to build the final S-expr to pass to 
'eshell-do-eval'.

This could possibly go in Emacs 29, since it's a bugfix to add onto a 
previous bugfix (see commit 990f36fa10). However, I'd lean towards just 
merging to master; this is a fairly obscure issue, and we can't just fix 
*every* bug we find on the release branch, or the branch will never 
stabilize. If someone else thinks it's important enough to go on the 
release branch though, I won't argue.

[-- Attachment #2: 0001-Fix-evaluation-of-asynchronous-expansions-in-Eshell-.patch --]
[-- Type: text/plain, Size: 4989 bytes --]

From 1bbaf547d8d6668bca732e14dc190416c5b52671 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 18 Jan 2023 19:15:38 -0800
Subject: [PATCH] Fix evaluation of asynchronous expansions in Eshell indices

Previously, this code passed the indices to a separate function, which
called 'eval' on them, but it should instead make an S-expr that
'eshell-do-eval' can evaluate.

* lisp/eshell/esh-var.el (eshell-eval-indices): Rename to...
(eshell-indices): ... this, and adjust implementation to return a form
to evaluate via 'eshell-do-eval'.
(eshell-parse-variable): Use 'eshell-indices'.  Also, remove
irrelevant comment.
(eshell-parse-variable-ref): Fix quoting in docstring.
(eshell-parse-indices): Fix typo in docstring.

* test/lisp/eshell/esh-var-tests.el
(esh-var-test/interp-var-indices-subcommand)
(esh-var-test/quoted-interp-var-indices-subcommand): New tests.
---
 lisp/eshell/esh-var.el            | 15 +++++++--------
 test/lisp/eshell/esh-var-tests.el | 25 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index fd76a2c6f09..2da35222044 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -467,9 +467,7 @@ eshell-parse-variable
 	  indices (and (not (eobp))
 		       (eq (char-after) ?\[)
 		       (eshell-parse-indices))
-          ;; This is an expression that will be evaluated by `eshell-do-eval',
-          ;; which only support let-binding of dynamically-scoped vars
-	  value `(let ((indices (eshell-eval-indices ',indices))) ,value))
+          value `(let ((indices ,(eshell-indices indices))) ,value))
     (when get-len
       (setq value `(length ,value)))
     (when eshell-current-quoted
@@ -496,7 +494,7 @@ eshell-parse-variable-ref
 
   NAME          an environment or Lisp variable value
   \"LONG-NAME\"   disambiguates the length of the name
-  `LONG-NAME'   as above
+  \\='LONG-NAME\\='   as above
   {COMMAND}     result of command is variable's value
   (LISP-FORM)   result of Lisp form is variable's value
   <COMMAND>     write the output of command to a temporary file;
@@ -591,7 +589,7 @@ eshell-parse-indices
   "Parse and return a list of index-lists.
 
 For example, \"[0 1][2]\" becomes:
-  ((\"0\" \"1\") (\"2\")."
+  ((\"0\" \"1\") (\"2\"))."
   (let (indices)
     (while (eq (char-after) ?\[)
       (let ((end (eshell-find-delimiter ?\[ ?\])))
@@ -607,9 +605,10 @@ eshell-parse-indices
 	  (goto-char (1+ end)))))
     (nreverse indices)))
 
-(defun eshell-eval-indices (indices)
-  "Evaluate INDICES, a list of index-lists generated by `eshell-parse-indices'."
-  (mapcar (lambda (i) (mapcar #'eval i)) indices))
+(defun eshell-indices (indices)
+  "Prepare INDICES to be evaluated by Eshell.
+INDICES is a list of index-lists generated by `eshell-parse-indices'."
+  `(list ,@(mapcar (lambda (idx-list) (cons 'list idx-list)) indices)))
 
 (defun eshell-get-variable (name &optional indices quoted)
   "Get the value for the variable NAME.
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 0cc1b92266f..82324d72163 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -82,6 +82,17 @@ esh-var-test/interp-var-indices
     (eshell-command-result-equal "echo $eshell-test-value[0 2 4]"
                                  '("zero" "two" "four"))))
 
+(ert-deftest esh-var-test/interp-var-indices-subcommand ()
+  "Interpolate list variable with subcommand expansion for indices"
+  (skip-unless (executable-find "echo"))
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (eshell-command-result-equal
+     "echo $eshell-test-value[${*echo 0}]"
+     "zero")
+    (eshell-command-result-equal
+     "echo $eshell-test-value[${*echo 0} ${*echo 2}]"
+     '("zero" "two"))))
+
 (ert-deftest esh-var-test/interp-var-split-indices ()
   "Interpolate string variable with indices"
   (let ((eshell-test-value "zero one two three four"))
@@ -271,6 +282,20 @@ esh-var-test/quoted-interp-var-indices
     (eshell-command-result-equal "echo \"$eshell-test-value[1 2 4]\""
                                  "(\"one\" \"two\" \"four\")")))
 
+(ert-deftest esh-var-test/quote-interp-var-indices-subcommand ()
+  "Interpolate list variable with subcommand expansion for indices
+inside double-quotes"
+  (skip-unless (executable-find "echo"))
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (eshell-command-result-equal
+     "echo \"$eshell-test-value[${*echo 0}]\""
+     "zero")
+    ;; FIXME: These tests would use the 0th index like the other tests
+    ;; here, but see above.
+    (eshell-command-result-equal
+     "echo \"$eshell-test-value[${*echo 1} ${*echo 2}]\""
+     "(\"one\" \"two\")")))
+
 (ert-deftest esh-var-test/quoted-interp-var-split-indices ()
   "Interpolate string variable with indices inside double-quotes"
   (let ((eshell-test-value "zero one two three four"))
-- 
2.25.1

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Eli Zaretskii]

> Date: Wed, 18 Jan 2023 19:36:41 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> 
> Starting from "emacs -Q -f eshell":
> 
>    ~ $ echo $exec-path[0]
>    /usr/local/sbin
>    ~ $ echo $exec-path[${echo 0}]
>    /usr/local/sbin
>    ~ $ echo $exec-path[${*echo 0}]
>    ;; no output
> 
> This is because 'eshell-eval-indices' gets an S-expr describing code to 
> evaluate for the indices, and it just passes that to 'eval'. That's not 
> the right way to do things for Eshell: instead, we should rely on 
> 'eshell-do-eval', which properly handles asynchronous evaluation. That's 
> required for working with external commands like "*echo" (which calls 
> the real /bin/echo).
> 
> The attached patch fixes this by changing 'eshell-eval-indices' to 
> 'eshell-indices', which does some minimal transformations on the S-expr 
> for the indices, and then uses it to build the final S-expr to pass to 
> 'eshell-do-eval'.
> 
> This could possibly go in Emacs 29, since it's a bugfix to add onto a 
> previous bugfix (see commit 990f36fa10). However, I'd lean towards just 
> merging to master; this is a fairly obscure issue, and we can't just fix 
> *every* bug we find on the release branch, or the branch will never 
> stabilize. If someone else thinks it's important enough to go on the 
> release branch though, I won't argue.

Why do you remove a non-internal function?  We cannot possibly do that
if this is going to be installed on the emacs-29 branch.  But even if
you are going to install on master, why not leave that function alone?
Some code somewhere could be using it, and we don't usually remove
functions before a period of deprecation.

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Jim Porter]

On 1/18/2023 10:49 PM, Eli Zaretskii wrote:
> Why do you remove a non-internal function?  We cannot possibly do that
> if this is going to be installed on the emacs-29 branch.  But even if
> you are going to install on master, why not leave that function alone?
> Some code somewhere could be using it, and we don't usually remove
> functions before a period of deprecation.

I can keep 'eshell-eval-indices' around and mark it obsolete if you 
prefer; it wouldn't hurt anything. I could also fix this bug in that 
function, though it would be an inferior fix compared to the new 
'eshell-indices' function, so we definitely want that new function (or 
something very similar). I'm not sure fixing 'eshell-eval-indices' is 
worth it though, since I'd be very surprised if anyone called that 
function directly.

For context, 'eshell-eval-indices' is a function I added in Emacs 29 to 
fix some related issues with indices[1], but at the time I didn't fully 
understand Eshell's internals and so implemented the fix incorrectly 
(though it works for most common cases). It probably could have been 
marked as internal at the time, but Eshell doesn't seem to do that 
regularly, even for functions that external code would be very unlikely 
to find useful.


[1] In particular, the second case in my original message fails even 
more severely in Emacs 28:

   ~/config $ echo $exec-path[${echo 0}]
   Wrong type argument: number-or-marker-p, (eshell-escape-arg (let 
((indices 'nil)) (eshell-convert (eshell-command-to-value 
(eshell-as-subcommand (eshell-trap-errors (eshell-named-command "echo" 
(list #("0" 0 1 (number t))))))))))

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Jim Porter]

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

On 1/18/2023 11:37 PM, Jim Porter wrote:
> On 1/18/2023 10:49 PM, Eli Zaretskii wrote:
>> Why do you remove a non-internal function?  We cannot possibly do that
>> if this is going to be installed on the emacs-29 branch.  But even if
>> you are going to install on master, why not leave that function alone?
>> Some code somewhere could be using it, and we don't usually remove
>> functions before a period of deprecation.
> 
> I can keep 'eshell-eval-indices' around and mark it obsolete if you 
> prefer; it wouldn't hurt anything.

Here's a patch that does this. It doesn't try to fix 
'eshell-eval-indices', since people shouldn't use that anyway. (I also 
renamed the new 'eshell-indices' to 'eshell-prepare-indices' to be clearer.)

[-- Attachment #2: 0001-Fix-evaluation-of-asynchronous-expansions-in-Eshell-.patch --]
[-- Type: text/plain, Size: 4999 bytes --]

From 4cc14646a50b237d38cfb6643858cb379366141a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 18 Jan 2023 19:15:38 -0800
Subject: [PATCH] Fix evaluation of asynchronous expansions in Eshell indices

Previously, this code passed the indices to a separate function, which
called 'eval' on them, but it should instead make an S-expr that
'eshell-do-eval' can evaluate.

* lisp/eshell/esh-var.el (eshell-eval-indices): Mark obsolete.
(eshell-prepare-indices): New function...
(eshell-parse-variable): ... use it.  Also, remove irrelevant comment.
(eshell-parse-variable-ref): Fix quoting in docstring.
(eshell-parse-indices): Fix typo in docstring.

* test/lisp/eshell/esh-var-tests.el
(esh-var-test/interp-var-indices-subcommand)
(esh-var-test/quoted-interp-var-indices-subcommand): New tests.
---
 lisp/eshell/esh-var.el            | 14 +++++++++-----
 test/lisp/eshell/esh-var-tests.el | 25 +++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index fd76a2c6f09..0998fc6b86a 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -467,9 +467,7 @@ eshell-parse-variable
 	  indices (and (not (eobp))
 		       (eq (char-after) ?\[)
 		       (eshell-parse-indices))
-          ;; This is an expression that will be evaluated by `eshell-do-eval',
-          ;; which only support let-binding of dynamically-scoped vars
-	  value `(let ((indices (eshell-eval-indices ',indices))) ,value))
+          value `(let ((indices ,(eshell-prepare-indices indices))) ,value))
     (when get-len
       (setq value `(length ,value)))
     (when eshell-current-quoted
@@ -496,7 +494,7 @@ eshell-parse-variable-ref
 
   NAME          an environment or Lisp variable value
   \"LONG-NAME\"   disambiguates the length of the name
-  `LONG-NAME'   as above
+  \\='LONG-NAME\\='   as above
   {COMMAND}     result of command is variable's value
   (LISP-FORM)   result of Lisp form is variable's value
   <COMMAND>     write the output of command to a temporary file;
@@ -591,7 +589,7 @@ eshell-parse-indices
   "Parse and return a list of index-lists.
 
 For example, \"[0 1][2]\" becomes:
-  ((\"0\" \"1\") (\"2\")."
+  ((\"0\" \"1\") (\"2\"))."
   (let (indices)
     (while (eq (char-after) ?\[)
       (let ((end (eshell-find-delimiter ?\[ ?\])))
@@ -609,8 +607,14 @@ eshell-parse-indices
 
 (defun eshell-eval-indices (indices)
   "Evaluate INDICES, a list of index-lists generated by `eshell-parse-indices'."
+  (declare (obsolete eshell-prepare-indices "30.1"))
   (mapcar (lambda (i) (mapcar #'eval i)) indices))
 
+(defun eshell-prepare-indices (indices)
+  "Prepare INDICES to be evaluated by Eshell.
+INDICES is a list of index-lists generated by `eshell-parse-indices'."
+  `(list ,@(mapcar (lambda (idx-list) (cons 'list idx-list)) indices)))
+
 (defun eshell-get-variable (name &optional indices quoted)
   "Get the value for the variable NAME.
 INDICES is a list of index-lists (see `eshell-parse-indices').
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 0cc1b92266f..82324d72163 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -82,6 +82,17 @@ esh-var-test/interp-var-indices
     (eshell-command-result-equal "echo $eshell-test-value[0 2 4]"
                                  '("zero" "two" "four"))))
 
+(ert-deftest esh-var-test/interp-var-indices-subcommand ()
+  "Interpolate list variable with subcommand expansion for indices"
+  (skip-unless (executable-find "echo"))
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (eshell-command-result-equal
+     "echo $eshell-test-value[${*echo 0}]"
+     "zero")
+    (eshell-command-result-equal
+     "echo $eshell-test-value[${*echo 0} ${*echo 2}]"
+     '("zero" "two"))))
+
 (ert-deftest esh-var-test/interp-var-split-indices ()
   "Interpolate string variable with indices"
   (let ((eshell-test-value "zero one two three four"))
@@ -271,6 +282,20 @@ esh-var-test/quoted-interp-var-indices
     (eshell-command-result-equal "echo \"$eshell-test-value[1 2 4]\""
                                  "(\"one\" \"two\" \"four\")")))
 
+(ert-deftest esh-var-test/quote-interp-var-indices-subcommand ()
+  "Interpolate list variable with subcommand expansion for indices
+inside double-quotes"
+  (skip-unless (executable-find "echo"))
+  (let ((eshell-test-value '("zero" "one" "two" "three" "four")))
+    (eshell-command-result-equal
+     "echo \"$eshell-test-value[${*echo 0}]\""
+     "zero")
+    ;; FIXME: These tests would use the 0th index like the other tests
+    ;; here, but see above.
+    (eshell-command-result-equal
+     "echo \"$eshell-test-value[${*echo 1} ${*echo 2}]\""
+     "(\"one\" \"two\")")))
+
 (ert-deftest esh-var-test/quoted-interp-var-split-indices ()
   "Interpolate string variable with indices inside double-quotes"
   (let ((eshell-test-value "zero one two three four"))
-- 
2.25.1

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Eli Zaretskii]

> Date: Thu, 19 Jan 2023 11:31:24 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 60942@debbugs.gnu.org
> 
> On 1/18/2023 11:37 PM, Jim Porter wrote:
> > On 1/18/2023 10:49 PM, Eli Zaretskii wrote:
> >> Why do you remove a non-internal function?  We cannot possibly do that
> >> if this is going to be installed on the emacs-29 branch.  But even if
> >> you are going to install on master, why not leave that function alone?
> >> Some code somewhere could be using it, and we don't usually remove
> >> functions before a period of deprecation.
> > 
> > I can keep 'eshell-eval-indices' around and mark it obsolete if you 
> > prefer; it wouldn't hurt anything.
> 
> Here's a patch that does this. It doesn't try to fix 
> 'eshell-eval-indices', since people shouldn't use that anyway. (I also 
> renamed the new 'eshell-indices' to 'eshell-prepare-indices' to be clearer.)

Is this for master?  If so, okay.  Otherwise, you'll need to adjust
the version in the obsolescence declaration.

Thanks.

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Jim Porter]

On 1/19/2023 11:41 AM, Eli Zaretskii wrote:
> Is this for master?  If so, okay.  Otherwise, you'll need to adjust
> the version in the obsolescence declaration.

Personally, I think just for master. The change isn't entirely 
risk-free. This code is a bit tricky, and I'd want a fair amount of time 
for people to identify bugs, just in case I regressed something.

(If this were for the release branch, we could probably remove 
'eshell-eval-indices', since it was introduced in Emacs 29 anyway.)

bug#60942: 30.0.50; [PATCH] Indices in Eshell variable interpolation don't work with async subcommands

[Jim Porter]

On 1/19/2023 12:20 PM, Jim Porter wrote:
> On 1/19/2023 11:41 AM, Eli Zaretskii wrote:
>> Is this for master?  If so, okay.  Otherwise, you'll need to adjust
>> the version in the obsolescence declaration.
> 
> Personally, I think just for master. The change isn't entirely 
> risk-free. This code is a bit tricky, and I'd want a fair amount of time 
> for people to identify bugs, just in case I regressed something.

And now merged to master as 54d5ea66c9. Closing this bug.