bug#54227: 29.0.50; [PATCH] Inconsistencies with Eshell variable interpolation

[Jim Porter <jporterbugs@gmail.com>]

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

On 3/7/2022 4:52 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: 54227@debbugs.gnu.org
>> Date: Sat, 5 Mar 2022 13:44:24 -0800
>>
>> +If @var{i} is a non-numeric value, expand to the value associated with
>> +the key @code{"i"}. For example, if @var{var} is @samp{(("dog"
> 
> Why did you need quotes in "i"?

That was my attempt to indicate that this:

   $foo[bar]

means (cdr (assoc "bar" foo)), not (cdr (assoc 'bar foo)) or (cdr (assoc 
bar foo)). Hopefully that's an ok way to express that. That said, this 
means the same thing too:

   $foo["bar"]

so it's not quite as simple as wrapping quotes around the subscript. 
This seemed like the simplest way to express the general idea, but it 
might be more technically correct to say something like, "... expand to 
the value associated with @var{i}'s value as a string." That wording is 
pretty confusing to me though...
> Also, it's suppoed to be @var{i}, as in the other alternatives.

Thanks, fixed.

>> +@item anything else
>> +Raises an error.
> 
> We say "Signals an error".

Fixed there, and in the entry below about the '$#foo' syntax.

[-- Attachment #2: 0001-Improve-wording-of-Eshell-variable-interpolation-cod.patch --]
[-- Type: text/plain, Size: 8517 bytes --]

From 48dbb3d9e1f8283440bf4695708a880a372f06d2 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 3 Mar 2022 09:37:25 -0800
Subject: [PATCH 1/2] Improve wording of Eshell variable interpolation
 code/documentation

* lisp/eshell/esh-arg.el (eshell-unescape-inner-double-quote): Rename
from 'eshell-parse-inner-double-quote'.

* lisp/eshell/esh-cmd.el (eshell-with-temp-command): Improve
docstring.

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Use
'eshell-unescape-inner-double-quote' and improve robustness of quoted
variable name matching.
(eshell-parse-indices): Use 'eshell-unescape-inner-double-quote'.

* doc/misc/eshell.texi (Dollars Expansion): Improve wording of
subscript notation.
---
 doc/misc/eshell.texi   | 43 ++++++++++++++++++++++++++++++------------
 lisp/eshell/esh-arg.el |  4 ++--
 lisp/eshell/esh-cmd.el | 25 +++++++++++++-----------
 lisp/eshell/esh-var.el | 14 ++++++++------
 4 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 5581e5cd9e..372e4c3ffb 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1040,18 +1040,37 @@ Dollars Expansion
 Expands to the @var{i}th element of the result of @var{expr}, an
 expression in one of the above forms listed here.  If multiple indices
 are supplied, this will return a list containing the elements for each
-index.  If @var{expr}'s value is a string, it will first be split at
-whitespace to make it a list.  If @var{expr}'s value is an alist
-(@pxref{Association List Type, Association Lists, , elisp, The Emacs
-Lisp Reference Manual}), this will call @code{assoc} on the result of
-@var{expr}, returning the @code{cdr} of the element of the result
-whose car is equal to @code{"i"}.  Raises an error if the value is not
-a sequence (@pxref{Sequences Arrays Vectors, Sequences, , elisp, The
+index.  The exact behavior depends on the type of @var{expr}'s value:
+
+@table @asis
+
+@item a sequence
+Expands to the element at the (zero-based) index @var{i} of the
+sequence (@pxref{Sequences Arrays Vectors, Sequences, , elisp, The
 Emacs Lisp Reference Manual}).
 
-Multiple sets of indices can also be specified. For example, if
-@var{var} is a list of lists, @samp{$@var{var}[0][0]} is equivalent to
-@samp{(caar @var{var})}.
+@item a string
+Split the string at whitespace, and then expand to the @var{i}th
+element of the resulting sequence.
+
+@item an alist
+If @var{i} is a non-numeric value, expand to the value associated with
+the key @code{"@var{i}"} in the alist.  For example, if @var{var} is
+@samp{(("dog" . "fido") ("cat" . "felix"))}, then
+@samp{$@var{var}[dog]} expands to @code{"fido"}.  Otherwise, this
+behaves as with sequences; e.g., @samp{$@var{var}[0]} expands to
+@code{("dog" . "fido")}.  @xref{Association List Type, Association
+Lists, , elisp, The Emacs Lisp Reference Manual}.
+
+@item anything else
+Signals an error.
+
+@end table
+
+Multiple sets of indices can also be specified.  For example, if
+@var{var} is @samp{((1 2) (3 4))}, then @samp{$@var{var}[0][1]} will
+expand to @code{2}, i.e.@: the second element of the first list member
+(all indices are zero-based).
 
 @item $@var{expr}[@var{regexp} @var{i...}]
 As above (when @var{expr} expands to a string), but use @var{regexp}
@@ -1063,8 +1082,8 @@ Dollars Expansion
 Expands to the length of the result of @var{expr}, an expression in
 one of the above forms.  For example, @samp{$#@var{var}} returns the
 length of the variable @var{var} and @samp{$#@var{var}[0]} returns the
-length of the first element of @var{var}.  Again, raises an error if
-the result of @var{expr} is not a sequence.
+length of the first element of @var{var}.  Again, signals an error if
+the result of @var{expr} is not a string or a sequence.
 
 @end table
 
diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index e19481c4ba..ee3f907f85 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -354,8 +354,8 @@ eshell-parse-double-quote
 		  (list 'eshell-escape-arg arg))))
 	  (goto-char (1+ end)))))))
 
-(defun eshell-parse-inner-double-quote (bound)
-  "Parse the inner part of a double quoted string.
+(defun eshell-unescape-inner-double-quote (bound)
+  "Unescape escaped characters inside a double-quoted string.
 The string to parse starts at point and ends at BOUND.
 
 If Eshell is currently parsing a quoted string and there are any
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 04b54d9d79..8be1136e31 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -350,24 +350,27 @@ eshell-complete-lisp-symbols
 
 (defvar eshell--sep-terms)
 
-(defmacro eshell-with-temp-command (command &rest body)
-  "Narrow the buffer to COMMAND and execute the forms in BODY.
-COMMAND can either be a string, or a cons cell demarcating a
-buffer region.  If COMMAND is a string, temporarily insert it
-into the buffer before narrowing.  Point will be set to the
-beginning of the narrowed region.
+(defmacro eshell-with-temp-command (region &rest body)
+  "Narrow the buffer to REGION and execute the forms in BODY.
+
+REGION is a cons cell (START . END) that specifies the region to
+which to narrow the buffer.  REGION can also be a string, in
+which case the macro temporarily inserts it into the buffer at
+point, and narrows the buffer to the inserted string.  Before
+executing BODY, point is set to the beginning of the narrowed
+REGION.
 
 The value returned is the last form in BODY."
   (declare (indent 1))
-  `(let ((cmd ,command))
-     (if (stringp cmd)
+  `(let ((reg ,region))
+     (if (stringp reg)
          ;; Since parsing relies partly on buffer-local state
          ;; (e.g. that of `eshell-parse-argument-hook'), we need to
          ;; perform the parsing in the Eshell buffer.
          (let ((begin (point)) end
 	       (inhibit-point-motion-hooks t))
            (with-silent-modifications
-             (insert cmd)
+             (insert reg)
              (setq end (point))
              (unwind-protect
                  (save-restriction
@@ -376,8 +379,8 @@ eshell-with-temp-command
                    ,@body)
                (delete-region begin end))))
        (save-restriction
-         (narrow-to-region (car cmd) (cdr cmd))
-         (goto-char (car cmd))
+         (narrow-to-region (car reg) (cdr reg))
+         (goto-char (car reg))
          ,@body))))
 
 (defun eshell-parse-command (command &optional args toplevel)
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index af89e35f55..8746f2bb93 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -437,7 +437,7 @@ eshell-parse-variable-ref
             `(eshell-convert
               (eshell-command-to-value
                (eshell-as-subcommand
-                ,(let ((subcmd (or (eshell-parse-inner-double-quote end)
+                ,(let ((subcmd (or (eshell-unescape-inner-double-quote end)
                                    (cons (point) end)))
                        (eshell-current-quoted nil))
                    (eshell-parse-command subcmd)))))
@@ -470,13 +470,15 @@ eshell-parse-variable-ref
     (condition-case nil
         `(eshell-command-to-value
           (eshell-lisp-command
-           ',(read (or (eshell-parse-inner-double-quote (point-max))
+           ',(read (or (eshell-unescape-inner-double-quote (point-max))
                        (current-buffer)))))
       (end-of-file
        (throw 'eshell-incomplete ?\())))
-   ((looking-at (rx (or "'" "\"" "\\\"")))
-    (eshell-with-temp-command (or (eshell-parse-inner-double-quote (point-max))
-                                  (cons (point) (point-max)))
+   ((looking-at (rx-to-string
+                 `(or "'" ,(if eshell-current-quoted "\\\"" "\""))))
+    (eshell-with-temp-command
+        (or (eshell-unescape-inner-double-quote (point-max))
+            (cons (point) (point-max)))
       (let ((name (if (eq (char-after) ?\')
                       (eshell-parse-literal-quote)
                     (eshell-parse-double-quote))))
@@ -506,7 +508,7 @@ eshell-parse-indices
 	(if (not end)
 	    (throw 'eshell-incomplete ?\[)
 	  (forward-char)
-          (eshell-with-temp-command (or (eshell-parse-inner-double-quote end)
+          (eshell-with-temp-command (or (eshell-unescape-inner-double-quote end)
                                         (cons (point) end))
 	    (let (eshell-glob-function (eshell-current-quoted nil))
 	      (setq indices (cons (eshell-parse-arguments
-- 
2.25.1


[-- Attachment #3: 0002-Support-applying-indices-to-more-Eshell-dollar-expan.patch --]
[-- Type: text/plain, Size: 5505 bytes --]

From 1f5704b24a3073f9ac1d273ff7a6b19760071711 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 5 Mar 2022 11:45:49 -0800
Subject: [PATCH 2/2] Support applying indices to more Eshell dollar expansions

For example, '${echo -e "hi\nbye"}[1]' should expand to "bye".

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Support applying
indices to '${}', '$()', and '$<>' forms.

* lisp/eshell/esh-var-tests.el (esh-var-test/interp-lisp-indices)
(esh-var-test/interp-cmd-indices)
(esh-var-test/interp-cmd-external-indices)
(esh-var-test/quoted-interp-lisp-indices)
(esh-var-test/quoted-interp-cmd-indices): New tests.
---
 lisp/eshell/esh-var.el            | 28 ++++++++++++++++------------
 test/lisp/eshell/esh-var-tests.el | 23 +++++++++++++++++++++++
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 8746f2bb93..ca4cbd744c 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -434,13 +434,15 @@ eshell-parse-variable-ref
           (throw 'eshell-incomplete ?\{)
         (forward-char)
         (prog1
-            `(eshell-convert
-              (eshell-command-to-value
-               (eshell-as-subcommand
-                ,(let ((subcmd (or (eshell-unescape-inner-double-quote end)
-                                   (cons (point) end)))
-                       (eshell-current-quoted nil))
-                   (eshell-parse-command subcmd)))))
+            `(eshell-apply-indices
+              (eshell-convert
+               (eshell-command-to-value
+                (eshell-as-subcommand
+                 ,(let ((subcmd (or (eshell-unescape-inner-double-quote end)
+                                    (cons (point) end)))
+                        (eshell-current-quoted nil))
+                    (eshell-parse-command subcmd)))))
+              indices)
           (goto-char (1+ end))))))
    ((eq (char-after) ?\<)
     (let ((end (eshell-find-delimiter ?\< ?\>)))
@@ -464,14 +466,16 @@ eshell-parse-variable-ref
                            ;; properly.  See bug#54190.
                            (list (function (lambda ()
                                    (delete-file ,temp))))))
-                   (quote ,temp)))
+                   (eshell-apply-indices ,temp indices)))
             (goto-char (1+ end)))))))
    ((eq (char-after) ?\()
     (condition-case nil
-        `(eshell-command-to-value
-          (eshell-lisp-command
-           ',(read (or (eshell-unescape-inner-double-quote (point-max))
-                       (current-buffer)))))
+        `(eshell-apply-indices
+          (eshell-command-to-value
+           (eshell-lisp-command
+            ',(read (or (eshell-unescape-inner-double-quote (point-max))
+                        (current-buffer)))))
+          indices)
       (end-of-file
        (throw 'eshell-incomplete ?\())))
    ((looking-at (rx-to-string
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index d09dd614de..1d051d681a 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -137,10 +137,18 @@ esh-var-test/interp-lisp
   "Interpolate Lisp form evaluation"
   (should (equal (eshell-test-command-result "+ $(+ 1 2) 3") 6)))
 
+(ert-deftest esh-var-test/interp-lisp-indices ()
+  "Interpolate Lisp form evaluation with index"
+  (should (equal (eshell-test-command-result "+ $(list 1 2)[1] 3") 5)))
+
 (ert-deftest esh-var-test/interp-cmd ()
   "Interpolate command result"
   (should (equal (eshell-test-command-result "+ ${+ 1 2} 3") 6)))
 
+(ert-deftest esh-var-test/interp-cmd-indices ()
+  "Interpolate command result with index"
+  (should (equal (eshell-test-command-result "+ ${list 1 2}[1] 3") 5)))
+
 (ert-deftest esh-var-test/interp-cmd-external ()
   "Interpolate command result from external command"
   (skip-unless (executable-find "echo"))
@@ -148,6 +156,13 @@ esh-var-test/interp-cmd-external
    (eshell-command-result-p "echo ${*echo hi}"
                             "hi\n")))
 
+(ert-deftest esh-var-test/interp-cmd-external-indices ()
+  "Interpolate command result from external command with index"
+  (skip-unless (executable-find "echo"))
+  (with-temp-eshell
+   (eshell-command-result-p "echo ${*echo \"hi\nbye\"}[1]"
+                            "bye\n")))
+
 (ert-deftest esh-var-test/interp-temp-cmd ()
   "Interpolate command result redirected to temp file"
   (should (equal (eshell-test-command-result "cat $<echo hi>") "hi")))
@@ -282,12 +297,20 @@ esh-var-test/quoted-interp-lisp
                   "echo \"hi $(concat \\\"the\\\" \\\"re\\\")\"")
                  "hi there")))
 
+(ert-deftest esh-var-test/quoted-interp-lisp-indices ()
+  "Interpolate Lisp form evaluation with index"
+  (should (equal (eshell-test-command-result "+ \"$(list 1 2)[1]\" 3") 5)))
+
 (ert-deftest esh-var-test/quoted-interp-cmd ()
   "Interpolate command result inside double-quotes"
   (should (equal (eshell-test-command-result
                   "echo \"hi ${echo \\\"there\\\"}\"")
                  "hi there")))
 
+(ert-deftest esh-var-test/quoted-interp-cmd-indices ()
+  "Interpolate command result with index inside double-quotes"
+  (should (equal (eshell-test-command-result "+ \"${list 1 2}[1]\" 3") 5)))
+
 (ert-deftest esh-var-test/quoted-interp-temp-cmd ()
   "Interpolate command result redirected to temp file inside double-quotes"
   (should (equal (eshell-test-command-result "cat \"$<echo hi>\"") "hi")))
-- 
2.25.1