From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 25 Jan 2023 01:14:54 +0000 (-0800)
Subject: Ensure that deferred commands don't make Eshell forget let-bound values
X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=c53255f67758cbd528c3422e248c0cb979a9a676;p=emacs.git
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).
---
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index b5f1d60ff18..efc46f10c96 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -741,18 +741,24 @@ if none)."
;; 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 @@ produced by `eshell-parse-command'."
(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 @@ be finished later after the completion of an asynchronous subprocess."
(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 @@ Test that trailing arguments outside the subcommand are ignored.
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")))
+
;; Lisp forms