From 67a2b320f61642d0cbbce31ac34d4e1ce77c9230 Mon Sep 17 00:00:00 2001 From: Jim Porter 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 (bug#61954). * 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 @@ implemented via rewriting, rather than as a function." (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 From e01660ca50ad360db78e5a0206ed824465c2aada Mon Sep 17 00:00:00 2001 From: Jim Porter 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 (bug#61954). * 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 @@ if none)." ;; `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 @@ produced by `eshell-parse-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 @@ have been replaced by constants." (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)) From 1565dbcae35c1e42b22066fde226e3b063614a9e Mon Sep 17 00:00:00 2001 From: Jim Porter 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' (bug#61954). * 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 @@ hooks should be run before and after the 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 @@ implemented via rewriting, rather than as a function." (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 @@ function." ;; 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 @@ if none)." ;; `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 @@ have been replaced by constants." (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)))