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

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

[Milan Zimmermann]

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

Hi,
I might be doing something wrong, but it appears that calling any non-lisp
command  (example: /usr/bin/tail) causes the variable set in the {} block
of "for var in list {}" to change to nil.

To duplicate, please create a file "bug.txt" in ~tmp,

   echo "line one" > ~/tmp/bug.txt

then run the following code -

In the code below, *please replace my full path
"/home/mzimmermann/tmp/bug.txt" with your full path. There seems to be an
unrelated issue trying to list ~/tmp/bug.txt*

  for file in (list "/home/mzimmermann/tmp/bug.txt") {
                  type-of $file
                  echo "file=$file";
                  # Create a variable. The back and forth between .log and
.txt is
                  # only there to simulate some useful work to set value of
samefile
                  export samefile="${echo
$file(:s/.txt/.log/)}"(:s/.log/.txt/);
                  echo "samefile=$samefile"
                  # Note that on my system, 'which tail' ==> /usr/bin/tail
                  tail $file
                  # Note that on my system, 'which cat' ==> eshell/cat is a
byte-compiled Lisp function in ‘em-unix.el’.
                  # cat $file
                  echo "samefile=$samefile"
                  echo "file=$file";
              }

Actual result:

================
string
file=/home/mzimmermann/tmp/bug.txt
samefile=/home/mzimmermann/tmp/bug.txt
line one
samefile=nil
file=/home/mzimmermann/tmp/bug.txt
================

Please *note how the contents of samefile is nullified. This is the bug I
am reporting here*

Expected result:
================
string
file=/home/mzimmermann/tmp/bug.txt
samefile=/home/mzimmermann/tmp/bug.txt
line one
samefile=/home/mzimmermann/tmp/bug.txt
file=/home/mzimmermann/tmp/bug.txt
================

Notes:

1. I tried this with other non-lisp functions (/usr/bin/gzip) and it caused
the same issue.

2. If we use "cat" which is an elisp function, the loop works as expected.

Thanks,
Milan Zimmermann

[-- Attachment #2: Type: text/html, Size: 2572 bytes --]

bug#59469: Adding a simpler duplication of the issue

[Milan Zimmermann]

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

A simpler duplication shows the issue is below.

It appears the issue is  is not related to the "for loop" - it can be
duplicated just writing code inside the eshell { .. } block.

To duplicate, please create a file ccc.el.gz - something the gzip does not
fail on.

Then, On the eshell prompt, enter the following code:

 $  export aaa="This is contents of aaa"
 {
# create a variable in this block
export bbb=$aaa;
echo "Before gzip: aaa=$aaa"
echo "Before gzip: bbb=$bbb";
gzip --decompress ccc.el.gz;
echo "After gzip: aaa=$aaa"
echo "After gzip: bbb=$bbb";
}
Before gzip: aaa=This is contents of aaa
Before gzip: bbb=This is contents of aaa
After gzip: aaa=This is contents of aaa
After gzip: bbb=nil
 $

Same bug: After the call to non-elisp program, /usr/bin/gzip, a previously
exported variable bbb (exported inside the block) is nullified.

Thanks

[-- Attachment #2: Type: text/html, Size: 1283 bytes --]

bug#59469: Adding a simpler duplication of the issue

[Jim Porter]

On 11/21/2022 6:50 PM, Milan Zimmermann wrote:
> A simpler duplication shows the issue is below.
[snip]
> Same bug: After the call to non-elisp program, /usr/bin/gzip, a 
> previously exported variable bbb (exported inside the block) is nullified.

I'm not entirely sure, but I have a suspicion that this is due to 
Eshell's deferred commands. Deferred commands tell Eshell to stop 
processing synchronously and yield to the rest of Emacs. It's a way of 
keeping long-running commands (e.g. external processes) from hanging the 
rest of Emacs.

Unfortunately, the logic to do this (see 'eshell-do-eval') was written 
before lexical binding was added to Emacs Lisp, and I think this is the 
cause of quite a few subtle bugs with Eshell command evaluation. Fixing 
that is bug#57635, which would leverage the generator.el internals to do 
this.

Of course, I could be wrong. This is reaching well past my comfort zone 
for Emacs Lisp, but this sure seems like an issue with 'eshell-do-eval'.

I'd certainly like to fix this one day, since it's blocking a few other 
things I want to do in Eshell, but I think it'll be a pretty big project.

bug#59469: Adding a simpler duplication of the issue

[Milan Zimmermann]

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

Thanks for the detailed update as always.

This is over my elisp level, I got here mostly as a relapsed eshell user,
trying to use eshell as my primary shell for the third time :)

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.

Not sure what to do next regarding this bug, perhaps we should go ahead and
add your comment to   bug#57635 so these two are linked from both ends? Or
let me know if I can help with something else,

Thanks,
Milan


On Mon, Nov 21, 2022 at 11:56 PM Jim Porter <jporterbugs@gmail.com> wrote:

> On 11/21/2022 6:50 PM, Milan Zimmermann wrote:
> > A simpler duplication shows the issue is below.
> [snip]
> > Same bug: After the call to non-elisp program, /usr/bin/gzip, a
> > previously exported variable bbb (exported inside the block) is
> nullified.
>
> I'm not entirely sure, but I have a suspicion that this is due to
> Eshell's deferred commands. Deferred commands tell Eshell to stop
> processing synchronously and yield to the rest of Emacs. It's a way of
> keeping long-running commands (e.g. external processes) from hanging the
> rest of Emacs.
>
> Unfortunately, the logic to do this (see 'eshell-do-eval') was written
> before lexical binding was added to Emacs Lisp, and I think this is the
> cause of quite a few subtle bugs with Eshell command evaluation. Fixing
> that is bug#57635, which would leverage the generator.el internals to do
> this.
>
> Of course, I could be wrong. This is reaching well past my comfort zone
> for Emacs Lisp, but this sure seems like an issue with 'eshell-do-eval'.
>
> I'd certainly like to fix this one day, since it's blocking a few other
> things I want to do in Eshell, but I think it'll be a pretty big project.
>

[-- Attachment #2: Type: text/html, Size: 2533 bytes --]

bug#59469: Adding a simpler duplication of the issue

[Jim Porter]

[-- 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

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]

[-- 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

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]

On 1/28/2023 10:55 PM, Jim Porter wrote:
> 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!).

Merged as c53255f677. Closing this bug now.