unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell
@ 2023-03-04  7:28 Jim Porter
  2023-03-06  1:15 ` Sean Whitton
  2023-03-17  5:33 ` Jim Porter
  0 siblings, 2 replies; 4+ messages in thread
From: Jim Porter @ 2023-03-04  7:28 UTC (permalink / raw)
  To: 61954

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-17  5:33 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-04  7:28 bug#61954: 30.0.50; [PATCH] Simplify structured commands in Eshell Jim Porter
2023-03-06  1:15 ` Sean Whitton
2023-03-06  2:11   ` Jim Porter
2023-03-17  5:33 ` Jim Porter

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).