master 97e35b14987: Avoid shadowing variables in some Eshell command forms

[Jim Porter]

branch: master
commit 97e35b149874a105a9975853a7fcd6f0034ddeab
Author: Jim Porter <jporterbugs@gmail.com>
Commit: Jim Porter <jporterbugs@gmail.com>

    Avoid shadowing variables in some Eshell command forms
    
    * lisp/eshell/esh-cmd.el (eshell-rewrite-for-command): Make
    'for-items' an uninterned symbol.
    (eshell-as-subcommand): Correct docstring.
    (eshell-do-command-to-value): Mark obsolete.
    (eshell-command-to-value): Move binding of 'value' outside of the
    macro's result, and remove call to 'eshell-do-command-to-value'.
    
    * test/lisp/eshell/esh-cmd-tests.el
    (esh-cmd-test/subcommand-shadow-value)
    (esh-cmd-test/for-loop-for-items-shadow): New tests.
    (esh-cmd-test/for-name-loop, esh-cmd-test/for-name-shadow-loop):
    Rename to...
    (esh-cmd-test/for-loop-name, esh-cmd-test/for-loop-name-shadow):
    ... these.
---
 lisp/eshell/esh-cmd.el            | 36 +++++++++++++++++++++---------------
 test/lisp/eshell/esh-cmd-tests.el | 18 ++++++++++++++++--
 2 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index d5237ee1f04..f0c6a146dfd 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -542,9 +542,10 @@ of its argument (i.e., use of a Lisp special form), it 
must be
 implemented via rewriting, rather than as a function."
   (if (and (equal (car terms) "for")
           (equal (nth 2 terms) "in"))
-      (let ((body (car (last terms))))
+      (let ((for-items (make-symbol "for-items"))
+            (body (car (last terms))))
        (setcdr (last terms 2) nil)
-        `(let ((for-items
+        `(let ((,for-items
                 (append
                  ,@(mapcar
                     (lambda (elem)
@@ -552,13 +553,13 @@ implemented via rewriting, rather than as a function."
                           elem
                         `(list ,elem)))
                     (nthcdr 3 terms)))))
-           (while for-items
-             (let ((,(intern (cadr terms)) (car for-items))
+           (while ,for-items
+             (let ((,(intern (cadr terms)) (car ,for-items))
                   (eshell--local-vars (cons ',(intern (cadr terms))
                                              eshell--local-vars)))
               (eshell-protect
                ,(eshell-invokify-arg body t)))
-             (setq for-items (cdr for-items)))
+             (setq ,for-items (cdr ,for-items)))
            (eshell-close-handles)))))
 
 (defun eshell-structure-basic-command (func names keyword test body
@@ -901,28 +902,33 @@ This is used on systems where async subprocesses are not 
supported."
                                       (symbol-value tailproc))))))
 
 (defmacro eshell-as-subcommand (command)
-  "Execute COMMAND using a temp buffer.
-This is used so that certain Lisp commands, such as `cd', when
-executed in a subshell, do not disturb the environment of the main
-Eshell buffer."
+  "Execute COMMAND as a subcommand.
+A subcommand creates a local environment so that any changes to
+the environment don't propagate outside of the subcommand's
+scope.  This lets you use commands like `cd' within a subcommand
+without changing the current directory of the main Eshell
+buffer."
   `(let ,eshell-subcommand-bindings
      ,command))
 
 (defmacro eshell-do-command-to-value (object)
   "Run a subcommand prepared by `eshell-command-to-value'.
 This avoids the need to use `let*'."
+  (declare (obsolete nil "30.1"))
   `(let ((eshell-current-handles
          (eshell-create-handles value 'overwrite)))
      (progn
        ,object
        (symbol-value value))))
 
-(defmacro eshell-command-to-value (object)
-  "Run OBJECT synchronously, returning its result as a string.
-Returns a string comprising the output from the command."
-  `(let ((value (make-symbol "eshell-temp"))
-         (eshell-in-pipeline-p nil))
-     (eshell-do-command-to-value ,object)))
+(defmacro eshell-command-to-value (command)
+  "Run an Eshell COMMAND synchronously, returning its output."
+  (let ((value (make-symbol "eshell-temp")))
+    `(let ((eshell-in-pipeline-p nil)
+           (eshell-current-handles
+           (eshell-create-handles ',value 'overwrite)))
+       ,command
+       ,value)))
 
 ;;;_* Iterative evaluation
 ;;
diff --git a/test/lisp/eshell/esh-cmd-tests.el 
b/test/lisp/eshell/esh-cmd-tests.el
index 94763954622..a7208eb3a0b 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -73,6 +73,13 @@ Test that trailing arguments outside the subcommand are 
ignored.
 e.g. \"{(+ 1 2)} 3\" => 3"
   (eshell-command-result-equal "{(+ 1 2)} 3" 3))
 
+(ert-deftest esh-cmd-test/subcommand-shadow-value ()
+  "Test that the variable `value' isn't shadowed inside subcommands."
+  (with-temp-eshell
+   (with-no-warnings (setq-local value "hello"))
+   (eshell-match-command-output "echo ${echo $value}"
+                                "hello\n")))
+
 (ert-deftest esh-cmd-test/let-rebinds-after-defer ()
   "Test that let-bound values are properly updated after `eshell-defer'.
 When inside a `let' block in an Eshell command form, we need to
@@ -151,13 +158,13 @@ bug#59469."
    (eshell-match-command-output "for i in 1 2 (list 3 4) { echo $i }"
                                 "1\n2\n3\n4\n")))
 
-(ert-deftest esh-cmd-test/for-name-loop () ; bug#15231
+(ert-deftest esh-cmd-test/for-loop-name () ; bug#15231
   "Test invocation of a for loop using `name'."
   (let ((process-environment (cons "name" process-environment)))
     (eshell-command-result-equal "for name in 3 { echo $name }"
                                  3)))
 
-(ert-deftest esh-cmd-test/for-name-shadow-loop () ; bug#15372
+(ert-deftest esh-cmd-test/for-loop-name-shadow () ; bug#15372
   "Test invocation of a for loop using an env-var."
   (let ((process-environment (cons "name=env-value" process-environment)))
     (with-temp-eshell
@@ -165,6 +172,13 @@ bug#59469."
       "echo $name; for name in 3 { echo $name }; echo $name"
       "env-value\n3\nenv-value\n"))))
 
+(ert-deftest esh-cmd-test/for-loop-for-items-shadow ()
+  "Test that the variable `for-items' isn't shadowed inside for loops."
+  (with-temp-eshell
+   (with-no-warnings (setq-local for-items "hello"))
+   (eshell-match-command-output "for i in 1 { echo $for-items }"
+                                "hello\n")))
+
 (ert-deftest esh-cmd-test/for-loop-pipe ()
   "Test invocation of a for loop piped to another command."
   (skip-unless (executable-find "rev"))