bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell

bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell

[Jim Porter]

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

Structured commands are just Eshell flow control commands, like 'if' and 
'while'. These expected you to set 'eshell-test-body' and 
'eshell-command-body' when implementing them, and were very hard to get 
right (see bug#12571 for example).

Instead, let's improve Eshell's iterative command evaluation some more 
so that you can write the implementations for these commands like 
normal. I think this is beneficial for two main reasons: a) it should 
get us closer to replacing Eshell's iterative evaluation with the CPS 
machinery in generator.el (bug#37635)[1], and b) third-parties should 
have an easier time writing their own fancy Eshell commands using the 
command-rewriting hooks.

Given that we have fairly thorough regression tests in Eshell now, I'm 
pretty confident that these patches don't break anything.

[1] I think I mentioned it elsewhere, but I'm hoping to make sure that 
when we do switch to the CPS machinery (or maybe to threads), it's just 
a drop-in replacement. That way, if there are problems, it should be 
easy to revert to the classic iterative evaluation code. Maybe we could 
even provide a defcustom for it just in case.

[-- Attachment #2: 0001-Simplify-iteration-in-Eshell-for-loops.patch --]
[-- Type: text/plain, Size: 2182 bytes --]

From 767839a0b3c03f910387c834f1320d4f9d465201 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 26 Jan 2023 23:18:42 -0800
Subject: [PATCH 1/3] Simplify iteration in Eshell for loops

The previous code fixed an issue in Eshell's iterative evaluation
where deferred commands caused an infinite loop (see bug#12571).
However, with the fix to unwinding let forms in 'eshell-do-eval' (see
bug#59469), we can just write this code as we normally would.

* lisp/eshell/esh-cmd.el (eshell-rewrite-for-command): Simplify.
---
 lisp/eshell/esh-cmd.el | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index d609711402a..2dd8f5d6042 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -533,25 +533,23 @@ eshell-rewrite-for-command
 	   (equal (nth 2 terms) "in"))
       (let ((body (car (last terms))))
 	(setcdr (last terms 2) nil)
-	`(let ((for-items
-		(copy-tree
-		 (append
-		  ,@(mapcar
-		     (lambda (elem)
-		       (if (listp elem)
-			   elem
-			 `(list ,elem)))
-		     (cdr (cddr terms))))))
-	       (eshell-command-body '(nil))
+        `(let ((for-items
+                (append
+                 ,@(mapcar
+                    (lambda (elem)
+                      (if (listp elem)
+                          elem
+                        `(list ,elem)))
+                    (nthcdr 3 terms))))
+               (eshell-command-body '(nil))
                (eshell-test-body '(nil)))
-	   (while (car 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--local-vars)))
 	       (eshell-protect
 	   	,(eshell-invokify-arg body t)))
-	     (setcar for-items (cadr for-items))
-	     (setcdr for-items (cddr for-items)))
+             (setq for-items (cdr for-items)))
            (eshell-close-handles)))))
 
 (defun eshell-structure-basic-command (func names keyword test body
-- 
2.25.1


[-- Attachment #3: 0002-Simplify-how-Eshell-s-iterative-evaluation-handles-i.patch --]
[-- Type: text/plain, Size: 3756 bytes --]

From 0e9d89c2373a3f82385e61330233e88698df0d1a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 28 Jan 2023 15:06:31 -0800
Subject: [PATCH 2/3] Simplify how Eshell's iterative evaluation handles 'if'
 forms

The previous implementation used 'eshell-test-body' and
'eshell-command-body' to track the condition and the then/else forms,
but those special variables are only needed for looping.  'if' only
evaluates each form once (at most).

* lisp/eshell/esh-cmd.el (Command evaluation macros): Remove 'if' from
the notes about 'eshell-test-body' and 'eshell-command-body'.
(eshell-do-eval): Reimplement evaluation of 'if' forms.
(eshell-eval-command): Don't let-bind 'eshell-command-body' and
'eshell-test-body'; they're no longer needed here.
---
 lisp/eshell/esh-cmd.el | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2dd8f5d6042..5dbbd770582 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -745,9 +745,9 @@ eshell-separate-commands
 ;;   `condition-case', `if', `let', `prog1', `progn', `quote', `setq',
 ;;   `unwind-protect', and `while'.
 ;;
-;; @ When using `if' or `while', first let-bind `eshell-test-body' and
+;; @ When using `while', first let-bind `eshell-test-body' and
 ;;   `eshell-command-body' to '(nil).  Eshell uses these variables to
-;;   handle conditional evaluation.
+;;   handle evaluating its subforms multiple times.
 ;;
 ;; @ The two `special' variables are `eshell-current-handles' and
 ;;   `eshell-current-subjob-p'.  Bind them locally with a `let' if you
@@ -1031,9 +1031,7 @@ eshell-eval-command
       ;; We can just stick the new command at the end of the current
       ;; one, and everything will happen as it should.
       (setcdr (last (cdr eshell-current-command))
-              (list `(let ((here (and (eobp) (point)))
-                           (eshell-command-body '(nil))
-                           (eshell-test-body '(nil)))
+              (list `(let ((here (and (eobp) (point))))
                        ,(and input
                              `(insert-and-inherit ,(concat input "\n")))
                        (if here
@@ -1149,23 +1147,17 @@ eshell-do-eval
           (setcar eshell-test-body (copy-tree (car args))))
 	(setcar eshell-command-body nil))
        ((eq (car form) 'if)
-        ;; `copy-tree' is needed here so that the test argument
-	;; doesn't get modified and thus always yield the same result.
-	(if (car eshell-command-body)
-	    (progn
-	      (cl-assert (not synchronous-p))
-	      (eshell-do-eval (car eshell-command-body)))
-	  (unless (car eshell-test-body)
-            (setcar eshell-test-body (copy-tree (car args))))
-	  (setcar eshell-command-body
-                  (copy-tree
-                   (if (cadr (eshell-do-eval (car eshell-test-body)
-                                             synchronous-p))
-                       (cadr args)
-                     (car (cddr args)))))
-	  (eshell-do-eval (car eshell-command-body) synchronous-p))
-	(setcar eshell-command-body nil)
-	(setcar eshell-test-body nil))
+        (eshell-manipulate "evaluating if condition"
+          (setcar args (eshell-do-eval (car args) synchronous-p)))
+        (eshell-do-eval
+         (cond
+          ((eval (car args))            ; COND is non-nil
+           (cadr args))
+          ((cdddr args)                 ; Multiple ELSE forms
+           `(progn ,@(cddr args)))
+          (t                            ; Zero or one ELSE forms
+           (caddr args)))
+         synchronous-p))
        ((eq (car form) 'setcar)
 	(setcar (cdr args) (eshell-do-eval (cadr args) synchronous-p))
 	(eval form))
-- 
2.25.1


[-- Attachment #4: 0003-Simplify-usage-of-while-forms-in-Eshell-s-iterative-.patch --]
[-- Type: text/plain, Size: 5419 bytes --]

From fd4e9a51ac13816fc7e0a0cd8e2e4f47a7b0226a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 28 Jan 2023 17:04:11 -0800
Subject: [PATCH 3/3] Simplify usage of 'while' forms in Eshell's iterative
 evaluation

Now, 'eshell-do-eval' rewrites 'while' forms to let-bind variables for
the command and test bodies.  This means that external code, such as
command rewriting hooks, no longer has to worry about this, making it
easier to pass "normal" Lisp forms to 'eshell-do-eval'.

* lisp/eshell/esh-cmd.el (eshell-command-body, eshell-test-body): No
longer used outside of 'eshell-do-eval', so rename to...
(eshell--command-body, eshell--test-body): ... these.
(Command evaluation macros): Remove obsolete description about 'if'
and 'while' forms.
(eshell-rewrite-for-command, eshell-structure-basic-command): Remove
'eshell-command-body' and 'eshell-test-body'.
(eshell-do-eval): Reimplement handling of 'while' forms.
---
 lisp/eshell/esh-cmd.el | 59 ++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 5dbbd770582..93f2616020c 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -494,8 +494,8 @@ eshell-rewrite-named-command
      (t
       (list sym (car terms))))))
 
-(defvar eshell-command-body)
-(defvar eshell-test-body)
+(defvar eshell--command-body)
+(defvar eshell--test-body)
 
 (defsubst eshell-invokify-arg (arg &optional share-output silent)
   "Change ARG so it can be invoked from a structured command.
@@ -540,9 +540,7 @@ eshell-rewrite-for-command
                       (if (listp elem)
                           elem
                         `(list ,elem)))
-                    (nthcdr 3 terms))))
-               (eshell-command-body '(nil))
-               (eshell-test-body '(nil)))
+                    (nthcdr 3 terms)))))
            (while for-items
              (let ((,(intern (cadr terms)) (car for-items))
 		   (eshell--local-vars (cons ',(intern (cadr terms))
@@ -579,8 +577,7 @@ eshell-structure-basic-command
 
   ;; finally, create the form that represents this structured
   ;; command
-  `(let ((eshell-command-body '(nil))
-         (eshell-test-body '(nil)))
+  `(progn
      (,func ,test ,body ,else)
      (eshell-close-handles)))
 
@@ -745,10 +742,6 @@ eshell-separate-commands
 ;;   `condition-case', `if', `let', `prog1', `progn', `quote', `setq',
 ;;   `unwind-protect', and `while'.
 ;;
-;; @ When using `while', first let-bind `eshell-test-body' and
-;;   `eshell-command-body' to '(nil).  Eshell uses these variables to
-;;   handle evaluating its subforms multiple times.
-;;
 ;; @ 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
@@ -1128,24 +1121,34 @@ eshell-do-eval
     (let ((args (cdr form)))
       (cond
        ((eq (car form) 'while)
+        ;; Wrap the `while' form with let-bindings for the command and
+        ;; test bodies.  This helps us resume evaluation midway
+        ;; through the loop.
+        (let ((new-form (copy-tree `(let ((eshell--command-body nil)
+                                          (eshell--test-body nil))
+                                      (eshell--wrapped-while ,@args)))))
+          (eshell-manipulate "modifying while form"
+            (setcar form (car new-form))
+            (setcdr form (cdr new-form)))
+          (eshell-do-eval form synchronous-p)))
+       ((eq (car form) 'eshell--wrapped-while)
+        (when eshell--command-body
+          (cl-assert (not synchronous-p))
+          (eshell-do-eval eshell--command-body)
+          (setq eshell--command-body nil
+                eshell--test-body nil))
         ;; `copy-tree' is needed here so that the test argument
-	;; doesn't get modified and thus always yield the same result.
-	(when (car eshell-command-body)
-	  (cl-assert (not synchronous-p))
-	  (eshell-do-eval (car eshell-command-body))
-	  (setcar eshell-command-body nil)
-	  (setcar eshell-test-body nil))
-	(unless (car eshell-test-body)
-          (setcar eshell-test-body (copy-tree (car args))))
-	(while (cadr (eshell-do-eval (car eshell-test-body) synchronous-p))
-	  (setcar eshell-command-body
-                  (if (cddr args)
-                      `(progn ,@(copy-tree (cdr args)))
-                    (copy-tree (cadr args))))
-	  (eshell-do-eval (car eshell-command-body) synchronous-p)
-	  (setcar eshell-command-body nil)
-          (setcar eshell-test-body (copy-tree (car args))))
-	(setcar eshell-command-body nil))
+        ;; doesn't get modified and thus always yield the same result.
+        (unless eshell--test-body
+          (setq eshell--test-body (copy-tree (car args))))
+        (while (cadr (eshell-do-eval eshell--test-body synchronous-p))
+          (setq eshell--command-body
+                (if (cddr args)
+                    `(progn ,@(copy-tree (cdr args)))
+                  (copy-tree (cadr args))))
+          (eshell-do-eval eshell--command-body synchronous-p)
+          (setq eshell--command-body nil
+                eshell--test-body (copy-tree (car args)))))
        ((eq (car form) 'if)
         (eshell-manipulate "evaluating if condition"
           (setcar args (eshell-do-eval (car args) synchronous-p)))
-- 
2.25.1

bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell

[Sean Whitton]

Hello,

On Fri 03 Mar 2023 at 11:28PM -08, Jim Porter wrote:

> Structured commands are just Eshell flow control commands, like 'if' and
> 'while'. These expected you to set 'eshell-test-body' and
> 'eshell-command-body' when implementing them, and were very hard to get right
> (see bug#12571 for example).
>
> Instead, let's improve Eshell's iterative command evaluation some more so that
> you can write the implementations for these commands like normal. I think this
> is beneficial for two main reasons: a) it should get us closer to replacing
> Eshell's iterative evaluation with the CPS machinery in generator.el
> (bug#37635)[1], and b) third-parties should have an easier time writing their
> own fancy Eshell commands using the command-rewriting hooks.

I think you have typo'd the bug number?

I would like to read about what the advantages of moving to CPS in
particular will be.  My understanding was that CPS is only rarely a good
idea.

-- 
Sean Whitton

bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell

[Jim Porter]

On 3/5/2023 5:15 PM, Sean Whitton wrote:
> I think you have typo'd the bug number?

Sorry, bug#57635.

> I would like to read about what the advantages of moving to CPS in
> particular will be.  My understanding was that CPS is only rarely a good
> idea.

Well, that's what 'eshell-do-eval' is already; it's just incomplete 
(doesn't understand all of Elisp) and has a number of bugs. Migrating to 
generator.el's CPS machinery would mean we get the benefits of a 
more-complete implementation, with (hopefully) fewer bugs.

Whether CPS is a good idea in general is above my pay grade, but short 
of some major improvements to threads in Emacs, I don't see an 
alternative. Eshell needs *some* mechanism for deferring execution of 
its command forms. (As I recall, threads have issues around prompting 
the user, which is why Tramp doesn't yet use threads, and Eshell would 
have the same issues. Not only that, but if Eshell used threads, it 
would cause problems when Eshell code calls Tramp code.)

bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell

[Jim Porter]

On 3/3/2023 11:28 PM, Jim Porter wrote:
> Structured commands are just Eshell flow control commands, like 'if' and 
> 'while'. These expected you to set 'eshell-test-body' and 
> 'eshell-command-body' when implementing them, and were very hard to get 
> right (see bug#12571 for example).
> 
> Instead, let's improve Eshell's iterative command evaluation some more 
> so that you can write the implementations for these commands like 
> normal.

Since there haven't been any concerns about the patches themselves in 
the last two weeks-ish, merging this now as 1565dbcae3 and closing the bug.