bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms

bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms

[Jim Porter]

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

To see this in action, start from "emacs -Q -f eshell", then:

   ~ $ setq value "hi there"
   hi there
   ~ $ echo $value
   hi there
   ~ $ echo ${echo $value}
   eshell-temp

The last command should *also* print "hi there", but the variable 
'value' gets shadowed by some internal Eshell code. (A similar problem 
occurs for 'for-items' when using a for loop in Eshell.)

Patch attached.

[-- Attachment #2: 0001-Avoid-shadowing-variables-in-some-Eshell-command-for.patch --]
[-- Type: text/plain, Size: 6303 bytes --]

From 7567f2b1c87f9fb8b74808f25dfa6e5f1d9f4ee8 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 18 Mar 2023 19:18:28 -0700
Subject: [PATCH] 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 93f2616020c..c3e91e5ca70 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -531,9 +531,10 @@ eshell-rewrite-for-command
 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)
@@ -541,13 +542,13 @@ eshell-rewrite-for-command
                           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
@@ -890,28 +891,33 @@ eshell-execute-pipeline
                                       (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 @@ esh-cmd-test/subcommand-lisp
 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 @@ esh-cmd-test/for-loop-multiple-args
    (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 @@ esh-cmd-test/for-name-shadow-loop
       "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"))
-- 
2.25.1

bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms

[Jim Porter]

On 3/25/2023 9:48 PM, Jim Porter wrote:
> Patch attached.

Pushed as 97e35b14987. Closing this bug now.