Improve correctness of Eshell sub-forms

This makes sure that we treat Eshell sub-forms (whether Lisp or command
forms) as values when appropriate, or as regular invocations.  This
requires a bit more explicit work, but helps to resolve some of the
surprising differences between Lisp and command forms in complex Eshell
statements.

* lisp/eshell/esh-cmd.el (eshell-subcommand-arg-values): Make obsolete.
(eshell-parse-lisp-argument): Don't add 'eshell-command-to-value' here.
(eshell-rewrite-sexp-command): Don't check for 'eshell-command-to-value
here'; instead check for 'eshell-lisp-command'.
(eshell-structure-basic-command): Check for 'eshell-lisp-command'.
(eshell-term-as-value): New function...
(eshell-rewrite-named-command, eshell-rewrite-for-command): ... call it.

* lisp/eshell/esh-arg.el (eshell-parse-special-reference):
* lisp/eshell/esh-io.el (eshell-strip-redirections):
* lisp/eshell/esh-var.el (eshell-prepare-indices): Call
'eshell-term-as-value'.

* test/lisp/eshell/esh-arg-tests.el
(esh-arg-test/special-reference/command-form):
* test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/for-loop-lisp-body)
(esh-cmd-test/while-loop-lisp-body)
(esh-cmd-test/if-else-statement-lisp-body): New tests.

* test/lisp/eshell/esh-var-tests.el
(esh-var-test/interp-var-indices-subcommand): Add another command to
test.

* doc/misc/eshell.texi (Control Flow): Update documentation.

4 files changed, 43 insertions, 25 deletions

diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index 6fc700cce89..b441cbfc274 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -35,6 +35,8 @@
 (eval-when-compile
   (require 'cl-lib))
 
+(declare-function eshell-term-as-value "esh-cmd" (term))
+
 (defgroup eshell-arg nil
   "Argument parsing involves transforming the arguments passed on the
 command line into equivalent Lisp forms that, when evaluated, will
@@ -626,7 +628,8 @@ If the form has no `type', the syntax is parsed as if `type' were
             (prog1
                 (cons creation-fun
                       (let ((eshell-current-argument-plain t))
-                        (eshell-parse-arguments (point) end)))
+                        (mapcar #'eshell-term-as-value
+                                (eshell-parse-arguments (point) end))))
               (goto-char (1+ end)))
           (ignore (goto-char here)))))))
 
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2a299125f22..65f997e5b88 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -454,6 +454,7 @@ command hooks should be run before and after the command."
 
 (defun eshell-subcommand-arg-values (terms)
   "Convert subcommand arguments {x} to ${x}, in order to take their values."
+  (declare (obsolete nil "31.1"))
   (setq terms (cdr terms))		; skip command argument
   (while terms
     (if (and (listp (car terms))
@@ -465,9 +466,9 @@ command hooks should be run before and after the command."
 (defun eshell-rewrite-sexp-command (terms)
   "Rewrite a sexp in initial position, such as `(+ 1 2)'."
   ;; this occurs when a Lisp expression is in first position
-  (if (and (listp (car terms))
-	   (eq (caar terms) 'eshell-command-to-value))
-      (car (cdar terms))))
+  (when (and (listp (car terms))
+             (eq (caar terms) 'eshell-lisp-command))
+    (car terms)))
 
 (defun eshell-rewrite-initial-subcommand (terms)
   "Rewrite a subcommand in initial position, such as `{+ 1 2}'."
@@ -477,20 +478,23 @@ command hooks should be run before and after the command."
 
 (defun eshell-rewrite-named-command (terms)
   "If no other rewriting rule transforms TERMS, assume a named command."
-  (eshell-subcommand-arg-values terms)
-  (let ((sym (if eshell-in-pipeline-p
-		 'eshell-named-command*
-	       'eshell-named-command))
-        (grouped-terms (eshell-prepare-splice terms)))
-    (cond
-     (grouped-terms
-      `(let ((terms (nconc ,@grouped-terms)))
-         (,sym (car terms) (cdr terms))))
-     ;; If no terms are spliced, use a simpler command form.
-     ((cdr terms)
-      (list sym (car terms) `(list ,@(cdr terms))))
-     (t
-      (list sym (car terms))))))
+  (when terms
+    (setq terms (cons (car terms)
+                      ;; Convert arguments to take their values.
+                      (mapcar #'eshell-term-as-value (cdr terms))))
+    (let ((sym (if eshell-in-pipeline-p
+		   'eshell-named-command*
+	         'eshell-named-command))
+          (grouped-terms (eshell-prepare-splice terms)))
+      (cond
+       (grouped-terms
+        `(let ((new-terms (nconc ,@grouped-terms)))
+           (,sym (car new-terms) (cdr new-terms))))
+       ;; If no terms are spliced, use a simpler command form.
+       ((cdr terms)
+        (list sym (car terms) `(list ,@(cdr terms))))
+       (t
+        (list sym (car terms)))))))
 
 (defvar eshell--command-body)
 (defvar eshell--test-body)
@@ -537,7 +541,7 @@ implemented via rewriting, rather than as a function."
                  ,@(mapcar
                     (lambda (elem)
                       (if (listp elem)
-                          elem
+                          (eshell-term-as-value elem)
                         `(list ,elem)))
                     (nthcdr 3 terms)))))
            (while ,for-items
@@ -555,7 +559,7 @@ negative.  It's not likely that users should ever need to call this
 function."
   ;; If the test form is a subcommand, wrap it in `eshell-commands' to
   ;; silence the output.
-  (when (eq (car test) 'eshell-as-subcommand)
+  (when (memq (car test) '(eshell-as-subcommand eshell-lisp-command))
     (setq test `(eshell-commands ,test t)))
 
   ;; If the test form begins with `eshell-convert' or
@@ -686,8 +690,7 @@ This means an exit code of 0."
 		(end-of-file
                  (throw 'eshell-incomplete "(")))))
 	(if (eshell-arg-delimiter)
-	    `(eshell-command-to-value
-              (eshell-lisp-command (quote ,obj)))
+	    `(eshell-lisp-command (quote ,obj))
 	  (ignore (goto-char here))))))
 
 (defun eshell-split-commands (terms separator &optional
@@ -912,6 +915,15 @@ This avoids the need to use `let*'."
          ,command
          ,value))))
 
+(defun eshell-term-as-value (term)
+  "Convert an Eshell TERM to take its value."
+  (cond
+   ((eq (car-safe term) 'eshell-as-subcommand) ; {x} -> ${x}
+    `(eshell-convert (eshell-command-to-value ,term)))
+   ((eq (car-safe term) 'eshell-lisp-command)  ; (x) -> $(x)
+    `(eshell-command-to-value ,term))
+   (t term)))
+
 ;;;_* Iterative evaluation
 ;;
 ;; Eshell runs all of its external commands asynchronously, so that
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index feb4bf8959f..443c39ff0d1 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -75,6 +75,7 @@
   (require 'cl-lib))
 
 (declare-function eshell-interactive-print "esh-mode" (string))
+(declare-function eshell-term-as-value "esh-cmd" (term))
 
 (defgroup eshell-io nil
   "Eshell's I/O management code provides a scheme for treating many
@@ -301,8 +302,8 @@ describing the mode, e.g. for using with `eshell-get-target'.")
         (unless (cdr tt)
           (error "Missing redirection target"))
         (nconc eshell-current-redirections
-               (list (list 'ignore
-                           (append (car tt) (list (cadr tt))))))
+               `((ignore ,(append (car tt)
+                                  (list (eshell-term-as-value (cadr tt)))))))
         (setcdr tl (cddr tt))
         (setq tt (cddr tt)))
        (t
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index d53ae997cdf..059bba03ee4 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -670,7 +670,9 @@ the original value of INDEX."
 (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)))
+  `(list ,@(mapcar (lambda (idx-list)
+                     (cons 'list (mapcar #'eshell-term-as-value idx-list)))
+                   indices)))
 
 (defun eshell-get-variable (name &optional indices quoted)
   "Get the value for the variable NAME.