Subject: bug#62452: closed (Re: bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 01 Apr 2023 23:32:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
62452: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62452
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Sat, 1 Apr 2023 16:31:01 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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

Pushed as 97e35b14987. Closing this bug now.

Subject: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Sat, 25 Mar 2023 21:48:13 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

From 7567f2b1c87f9fb8b74808f25dfa6e5f1d9f4ee8 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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