bug#59469: 29.0.50; Eshell "for" loop: Calling a non-lisp command (example: /usr/bin/tail) sets the variable exported in the {} block of "for var in list {}" to nil (was: Adding a simpler duplication of the issue)

[Jim Porter <jporterbugs@gmail.com>]

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

(Restoring the original subject so this issue is hopefully easier to track.)

On 1/24/2023 5:39 PM, Jim Porter wrote:
> The fix here is that when we're inside a 'let' and see 'eshell-defer' 
> get thrown, update the let-bindings in place. So now the updated form 
> would look like:
> 
>    (let ((process-environment (cons "var=value" process-environment)))
>      '"value"   ; Not really necessary, but it doesn't hurt anything.
>      (eshell-external-command "grep")
>      (eshell/echo (getenv "var")))
> 
> And so with this, it all works.

Here's an updated patch with an improved comment introducing Eshell's 
iterative evaluation. Hopefully the added detail will help anyone else 
who looks into this code (assuming I haven't replaced it with threads or 
generator.el's CPS machinery by then, of course!).

Note: I have a few other patches to apply after this that should let 
'eshell-do-eval' handle "normal" Emacs Lisp for the most part. This will 
make it easier to maintain all the code that generates Eshell's internal 
forms, as well as simplifying the migration to threads and/or 
generator.el. I'll file those separately though, since this is tricky 
code, and I want to be sure each change gets a reasonable amount of 
attention.

[-- Attachment #2: 0001-Ensure-that-deferred-commands-don-t-make-Eshell-forg.patch --]
[-- Type: text/plain, Size: 7383 bytes --]

From f127c735c54919f933d39ef2b625080c0b756a04 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 24 Jan 2023 17:14:54 -0800
Subject: [PATCH 1/4] Ensure that deferred commands don't make Eshell forget
 let-bound values

* lisp/eshell/esh-cmd.el (Command evaluation macros): Expand this
documentation to list allowed special forms and caveats for working
with 'if' and 'while'.
(eshell-do-eval): Provide more detail in docstring.  Handle
'eshell-defer' inside 'let' forms.

* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/let-rebinds-after-defer): New test (bug#59469).
---
 lisp/eshell/esh-cmd.el            | 79 +++++++++++++++++++++++--------
 test/lisp/eshell/esh-cmd-tests.el | 17 +++++++
 2 files changed, 77 insertions(+), 19 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 99c3d7f627d..96519724dd5 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -741,18 +741,24 @@ eshell-separate-commands
 ;; The structure of the following macros is very important to
 ;; `eshell-do-eval' [Iterative evaluation]:
 ;;
-;; @ Don't use forms that conditionally evaluate their arguments, such
-;;   as `setq', `if', `while', `let*', etc.  The only special forms
-;;   that can be used are `let', `condition-case' and
-;;   `unwind-protect'.
+;; @ Don't use special forms that conditionally evaluate their
+;;   arguments, such as `let*', unless Eshell explicitly supports
+;;   them.  Eshell supports the following special forms: `catch',
+;;   `condition-case', `if', `let', `prog1', `progn', `quote', `setq',
+;;   `unwind-protect', and `while'.
 ;;
-;; @ The main body of a `let' can contain only one form.  Use `progn'
-;;   if necessary.
+;; @ When using `if' or `while', first let-bind `eshell-test-body' and
+;;   `eshell-command-body' to '(nil).  Eshell uses these variables to
+;;   handle conditional evaluation.
 ;;
 ;; @ The two `special' variables are `eshell-current-handles' and
 ;;   `eshell-current-subjob-p'.  Bind them locally with a `let' if you
 ;;   need to change them.  Change them directly only if your intention
 ;;   is to change the calling environment.
+;;
+;; These rules likewise apply to any other code that generates forms
+;; that `eshell-do-eval' will evaluated, such as command rewriting
+;; hooks (see `eshell-rewrite-command-hook' and friends).
 
 (defmacro eshell-do-subjob (object)
   "Evaluate a command OBJECT as a subjob.
@@ -1095,9 +1101,17 @@ eshell-manipulate
        (eshell-debug-command ,(concat "done " (eval tag)) form))))
 
 (defun eshell-do-eval (form &optional synchronous-p)
-  "Evaluate form, simplifying it as we go.
+  "Evaluate FORM, simplifying it as we go.
 Unless SYNCHRONOUS-P is non-nil, throws `eshell-defer' if it needs to
-be finished later after the completion of an asynchronous subprocess."
+be finished later after the completion of an asynchronous subprocess.
+
+As this function evaluates FORM, it will gradually replace
+subforms with the (quoted) result of evaluating them.  For
+example, a function call is replaced with the result of the call.
+This allows us to resume evaluation of FORM after something
+inside throws `eshell-defer' simply by calling this function
+again.  Any forms preceding one that throw `eshell-defer' will
+have been replaced by constants."
   (cond
    ((not (listp form))
     (list 'quote (eval form)))
@@ -1161,21 +1175,48 @@ eshell-do-eval
 	(setcar (cdr args) (eshell-do-eval (cadr args) synchronous-p))
 	(eval form))
        ((eq (car form) 'let)
-	(if (not (eq (car (cadr args)) 'eshell-do-eval))
-	    (eshell-manipulate "evaluating let args"
-	      (dolist (letarg (car args))
-		(if (and (listp letarg)
-			 (not (eq (cadr letarg) 'quote)))
-		    (setcdr letarg
-			    (list (eshell-do-eval
-				   (cadr letarg) synchronous-p)))))))
+        (when (not (eq (car (cadr args)) 'eshell-do-eval))
+          (eshell-manipulate "evaluating let args"
+            (dolist (letarg (car args))
+              (when (and (listp letarg)
+                         (not (eq (cadr letarg) 'quote)))
+                (setcdr letarg
+                        (list (eshell-do-eval
+                               (cadr letarg) synchronous-p)))))))
         (cl-progv
-            (mapcar (lambda (binding) (if (consp binding) (car binding) binding))
+            (mapcar (lambda (binding)
+                      (if (consp binding) (car binding) binding))
                     (car args))
             ;; These expressions should all be constants now.
-            (mapcar (lambda (binding) (if (consp binding) (eval (cadr binding))))
+            (mapcar (lambda (binding)
+                      (when (consp binding) (eval (cadr binding))))
                     (car args))
-	  (eshell-do-eval (macroexp-progn (cdr args)) synchronous-p)))
+          (let (deferred result)
+            ;; Evaluate the `let' body, catching `eshell-defer' so we
+            ;; can handle it below.
+            (setq deferred
+                  (catch 'eshell-defer
+                    (ignore (setq result (eshell-do-eval
+                                          (macroexp-progn (cdr args))
+                                          synchronous-p)))))
+            ;; If something threw `eshell-defer', we need to update
+            ;; the let-bindings' values so that those values are
+            ;; correct when we resume evaluation of this form.
+            (when deferred
+              (eshell-manipulate "rebinding let args after `eshell-defer'"
+                (let ((bindings (car args)))
+                  (while bindings
+                    (let ((binding (if (consp (car bindings))
+                                       (caar bindings)
+                                     (car bindings))))
+                      (setcar bindings
+                              (list binding
+                                    (list 'quote (symbol-value binding)))))
+                    (pop bindings))))
+              (throw 'eshell-defer deferred))
+            ;; If we get here, there was no `eshell-defer' thrown, so
+            ;; just return the `let' body's result.
+            result)))
        ((memq (car form) '(catch condition-case unwind-protect))
 	;; `condition-case' and `unwind-protect' have to be
 	;; handled specially, because we only want to call
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index bcecc9a531f..94763954622 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -73,6 +73,23 @@ esh-cmd-test/subcommand-lisp
 e.g. \"{(+ 1 2)} 3\" => 3"
   (eshell-command-result-equal "{(+ 1 2)} 3" 3))
 
+(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
+ensure that deferred commands update any let-bound variables so
+they have the correct values when resuming evaluation.  See
+bug#59469."
+  (skip-unless (executable-find "echo"))
+  (with-temp-eshell
+   (eshell-match-command-output
+    (concat "{"
+            "  export LOCAL=value; "
+            "  echo \"$LOCAL\"; "
+            "  *echo external; "        ; This will throw `eshell-defer'.
+            "  echo \"$LOCAL\"; "
+            "}")
+    "value\nexternal\nvalue\n")))
+
 \f
 ;; Lisp forms
 
-- 
2.25.1