bug#59469: Adding a simpler duplication of the issue

[Jim Porter <jporterbugs@gmail.com>]

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

On 11/21/2022 11:18 PM, Milan Zimmermann wrote:
> But it sounds to me like your intuition about this could be fixed by 
> rewriting the core 'eshell-do-eval' loop in bug#57635 can be correct.  I 
> would enjoy helping with  it, but at the moment it is above my time and 
> elisp abilities.

After digging through 'eshell-do-eval' for another issue, I think I 
mostly understand it now. I still think bug#57635 is the way to go 
long-term (either that or use real threads), but since that's a big 
change, it might be best to give users some time where they could opt 
out of some new Eshell evaluation backend, just in case we break 
something with it. As such, I want to make sure the existing backend is 
as bug-free as possible so that (ideally) switching between the two has 
no behavioral difference.

With that said, here's a patch that fixes this, plus a regression test. 
One question for anyone reviewing the patch: is there a better way to do 
the "catch and rethrow" dance I do in 'eshell-do-eval'? It seems kind of 
clumsy...

----------------------------------------

The patch has a description of the problem, but I'll describe it in more 
detail here for anyone curious. Eshell turns the command in the original 
message into a form like this (note: this is very approximate):

   (let ((process-environment process-environment))
     (setenv "var" "value")
     (eshell-external-command "grep") ; This throws 'eshell-defer'
     (eshell/echo (getenv "var")))

'eshell-do-eval' gradually steps through this form, evaluating subforms 
and replacing them with their (quoted) results. This way, when a command 
throws 'eshell-defer', you can resume this form simply by calling 
'eshell-do-eval' again. So at the point 'eshell-defer' gets thrown, the 
form has been updated to:

   (let ((process-environment process-environment))
     '"value"   ; The quoted result of 'setenv' above.
     (eshell-external-command "grep")
     (eshell/echo (getenv "var")))

But since 'process-environment' is let-bound, when we resume evaluation, 
it's as though 'setenv' had never been called at all!

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.

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

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

* lisp/eshell/esh-cmd.el (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            | 61 ++++++++++++++++++++++++-------
 test/lisp/eshell/esh-cmd-tests.el | 17 +++++++++
 2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 99c3d7f627d..414934081b0 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1095,9 +1095,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 +1169,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