From a78204c24c7c0db442735480bd06bceca0a342c2 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Fri, 4 Oct 2024 21:45:04 -0700 Subject: [PATCH] Improve correctness of Eshell sub-forms This makes sure that we treat Eshell sub-forms (whether Lisp or command forms) as values when appropriate, or as regular invocations. This requires a bit more explicit work, but helps to resolve some of the surprising differences between Lisp and command forms in complex Eshell statements. * lisp/eshell/esh-cmd.el (eshell-subcommand-arg-values): Make obsolete. (eshell-parse-lisp-argument): Don't add 'eshell-command-to-value' here. (eshell-rewrite-sexp-command): Don't check for 'eshell-command-to-value here'; instead check for 'eshell-lisp-command'. (eshell-structure-basic-command): Check for 'eshell-lisp-command'. (eshell-term-as-value): New function... (eshell-rewrite-named-command, eshell-rewrite-for-command): ... call it. * lisp/eshell/esh-arg.el (eshell-parse-special-reference): * lisp/eshell/esh-io.el (eshell-strip-redirections): * lisp/eshell/esh-var.el (eshell-prepare-indices): Call 'eshell-term-as-value'. * test/lisp/eshell/esh-arg-tests.el (esh-arg-test/special-reference/command-form): * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/for-loop-lisp-body) (esh-cmd-test/while-loop-lisp-body) (esh-cmd-test/if-else-statement-lisp-body): New tests. * test/lisp/eshell/esh-var-tests.el (esh-var-test/interp-var-indices-subcommand): Add another command to test. * doc/misc/eshell.texi (Control Flow): Update documentation. (cherry picked from commit 40ffacb34b194aa82273539ab7a5be2f485a706f) --- doc/misc/eshell.texi | 43 ++++++++++++------------ lisp/eshell/esh-arg.el | 5 ++- lisp/eshell/esh-cmd.el | 54 +++++++++++++++++++------------ lisp/eshell/esh-io.el | 5 +-- lisp/eshell/esh-var.el | 4 ++- test/lisp/eshell/esh-arg-tests.el | 13 ++++++++ test/lisp/eshell/esh-cmd-tests.el | 26 +++++++++++++++ test/lisp/eshell/esh-var-tests.el | 3 ++ 8 files changed, 108 insertions(+), 45 deletions(-) diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi index a28ef26169b..8aa103741bc 100644 --- a/doc/misc/eshell.texi +++ b/doc/misc/eshell.texi @@ -1689,36 +1689,39 @@ convenience. Most of Eshell's control flow statements accept a @var{conditional}. This can take a few different forms. If @var{conditional} is a dollar -expansion, the condition is satisfied if the result is a -non-@code{nil} value. If @var{conditional} is a @samp{@{ -@var{subcommand} @}} or @samp{(@var{lisp form})}, the condition is -satisfied if the command's exit status is 0. +expansion, the condition is satisfied if the result is a non-@code{nil} +value. Alternately, @var{conditional} may be a subcommand, either in +command form, e.g.@: @samp{@{@var{subcommand}@}}; or in Lisp form, +e.g.@: @samp{(@var{lisp form})}. In that case, the condition is +satisfied if the subcommand's exit status is 0. @table @code -@item if @var{conditional} @{ @var{true-commands} @} -@itemx if @var{conditional} @{ @var{true-commands} @} @{ @var{false-commands} @} -Evaluate @var{true-commands} if @var{conditional} is satisfied; -otherwise, evaluate @var{false-commands}. +@item if @var{conditional} @var{true-subcommand} +@itemx if @var{conditional} @var{true-subcommand} @var{false-subcommand} +Evaluate @var{true-subcommand} if @var{conditional} is satisfied; +otherwise, evaluate @var{false-subcommand}. Both @var{true-subcommand} +and @var{false-subcommand} should be subcommands, as with +@var{conditional}. -@item unless @var{conditional} @{ @var{false-commands} @} -@itemx unless @var{conditional} @{ @var{false-commands} @} @{ @var{true-commands} @} -Evaluate @var{false-commands} if @var{conditional} is not satisfied; -otherwise, evaluate @var{true-commands}. +@item unless @var{conditional} @var{false-subcommand} +@itemx unless @var{conditional} @var{false-subcommand} @var{true-subcommand} +Evaluate @var{false-subcommand} if @var{conditional} is not satisfied; +otherwise, evaluate @var{true-subcommand}. -@item while @var{conditional} @{ @var{commands} @} -Repeatedly evaluate @var{commands} so long as @var{conditional} is +@item while @var{conditional} @var{subcommand} +Repeatedly evaluate @var{subcommand} so long as @var{conditional} is satisfied. -@item until @var{conditional} @{ @var{commands} @} -Repeatedly evaluate @var{commands} until @var{conditional} is +@item until @var{conditional} @var{subcommand} +Repeatedly evaluate @var{subcommand} until @var{conditional} is satisfied. -@item for @var{var} in @var{list}@dots{} @{ @var{commands} @} +@item for @var{var} in @var{list}@dots{} @var{subcommand} Iterate over each element of @var{list}, storing the element in -@var{var} and evaluating @var{commands}. If @var{list} is not a list, -treat it as a list of one element. If you specify multiple -@var{lists}, this will iterate over each of them in turn. +@var{var} and evaluating @var{subcommand}. If @var{list} is not a list, +treat it as a list of one element. If you specify multiple @var{lists}, +this will iterate over each of them in turn. @end table diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el index 6fc700cce89..b441cbfc274 100644 --- a/lisp/eshell/esh-arg.el +++ b/lisp/eshell/esh-arg.el @@ -35,6 +35,8 @@ (eval-when-compile (require 'cl-lib)) +(declare-function eshell-term-as-value "esh-cmd" (term)) + (defgroup eshell-arg nil "Argument parsing involves transforming the arguments passed on the command line into equivalent Lisp forms that, when evaluated, will @@ -626,7 +628,8 @@ If the form has no `type', the syntax is parsed as if `type' were (prog1 (cons creation-fun (let ((eshell-current-argument-plain t)) - (eshell-parse-arguments (point) end))) + (mapcar #'eshell-term-as-value + (eshell-parse-arguments (point) end)))) (goto-char (1+ end))) (ignore (goto-char here))))))) diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 2a299125f22..65f997e5b88 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -454,6 +454,7 @@ command hooks should be run before and after the command." (defun eshell-subcommand-arg-values (terms) "Convert subcommand arguments {x} to ${x}, in order to take their values." + (declare (obsolete nil "31.1")) (setq terms (cdr terms)) ; skip command argument (while terms (if (and (listp (car terms)) @@ -465,9 +466,9 @@ command hooks should be run before and after the command." (defun eshell-rewrite-sexp-command (terms) "Rewrite a sexp in initial position, such as `(+ 1 2)'." ;; this occurs when a Lisp expression is in first position - (if (and (listp (car terms)) - (eq (caar terms) 'eshell-command-to-value)) - (car (cdar terms)))) + (when (and (listp (car terms)) + (eq (caar terms) 'eshell-lisp-command)) + (car terms))) (defun eshell-rewrite-initial-subcommand (terms) "Rewrite a subcommand in initial position, such as `{+ 1 2}'." @@ -477,20 +478,23 @@ command hooks should be run before and after the command." (defun eshell-rewrite-named-command (terms) "If no other rewriting rule transforms TERMS, assume a named command." - (eshell-subcommand-arg-values terms) - (let ((sym (if eshell-in-pipeline-p - 'eshell-named-command* - 'eshell-named-command)) - (grouped-terms (eshell-prepare-splice terms))) - (cond - (grouped-terms - `(let ((terms (nconc ,@grouped-terms))) - (,sym (car terms) (cdr terms)))) - ;; If no terms are spliced, use a simpler command form. - ((cdr terms) - (list sym (car terms) `(list ,@(cdr terms)))) - (t - (list sym (car terms)))))) + (when terms + (setq terms (cons (car terms) + ;; Convert arguments to take their values. + (mapcar #'eshell-term-as-value (cdr terms)))) + (let ((sym (if eshell-in-pipeline-p + 'eshell-named-command* + 'eshell-named-command)) + (grouped-terms (eshell-prepare-splice terms))) + (cond + (grouped-terms + `(let ((new-terms (nconc ,@grouped-terms))) + (,sym (car new-terms) (cdr new-terms)))) + ;; If no terms are spliced, use a simpler command form. + ((cdr terms) + (list sym (car terms) `(list ,@(cdr terms)))) + (t + (list sym (car terms))))))) (defvar eshell--command-body) (defvar eshell--test-body) @@ -537,7 +541,7 @@ implemented via rewriting, rather than as a function." ,@(mapcar (lambda (elem) (if (listp elem) - elem + (eshell-term-as-value elem) `(list ,elem))) (nthcdr 3 terms))))) (while ,for-items @@ -555,7 +559,7 @@ negative. It's not likely that users should ever need to call this function." ;; If the test form is a subcommand, wrap it in `eshell-commands' to ;; silence the output. - (when (eq (car test) 'eshell-as-subcommand) + (when (memq (car test) '(eshell-as-subcommand eshell-lisp-command)) (setq test `(eshell-commands ,test t))) ;; If the test form begins with `eshell-convert' or @@ -686,8 +690,7 @@ This means an exit code of 0." (end-of-file (throw 'eshell-incomplete "("))))) (if (eshell-arg-delimiter) - `(eshell-command-to-value - (eshell-lisp-command (quote ,obj))) + `(eshell-lisp-command (quote ,obj)) (ignore (goto-char here)))))) (defun eshell-split-commands (terms separator &optional @@ -912,6 +915,15 @@ This avoids the need to use `let*'." ,command ,value)))) +(defun eshell-term-as-value (term) + "Convert an Eshell TERM to take its value." + (cond + ((eq (car-safe term) 'eshell-as-subcommand) ; {x} -> ${x} + `(eshell-convert (eshell-command-to-value ,term))) + ((eq (car-safe term) 'eshell-lisp-command) ; (x) -> $(x) + `(eshell-command-to-value ,term)) + (t term))) + ;;;_* Iterative evaluation ;; ;; Eshell runs all of its external commands asynchronously, so that diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el index feb4bf8959f..443c39ff0d1 100644 --- a/lisp/eshell/esh-io.el +++ b/lisp/eshell/esh-io.el @@ -75,6 +75,7 @@ (require 'cl-lib)) (declare-function eshell-interactive-print "esh-mode" (string)) +(declare-function eshell-term-as-value "esh-cmd" (term)) (defgroup eshell-io nil "Eshell's I/O management code provides a scheme for treating many @@ -301,8 +302,8 @@ describing the mode, e.g. for using with `eshell-get-target'.") (unless (cdr tt) (error "Missing redirection target")) (nconc eshell-current-redirections - (list (list 'ignore - (append (car tt) (list (cadr tt)))))) + `((ignore ,(append (car tt) + (list (eshell-term-as-value (cadr tt))))))) (setcdr tl (cddr tt)) (setq tt (cddr tt))) (t diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el index d53ae997cdf..059bba03ee4 100644 --- a/lisp/eshell/esh-var.el +++ b/lisp/eshell/esh-var.el @@ -670,7 +670,9 @@ the original value of INDEX." (defun eshell-prepare-indices (indices) "Prepare INDICES to be evaluated by Eshell. INDICES is a list of index-lists generated by `eshell-parse-indices'." - `(list ,@(mapcar (lambda (idx-list) (cons 'list idx-list)) indices))) + `(list ,@(mapcar (lambda (idx-list) + (cons 'list (mapcar #'eshell-term-as-value idx-list))) + indices))) (defun eshell-get-variable (name &optional indices quoted) "Get the value for the variable NAME. diff --git a/test/lisp/eshell/esh-arg-tests.el b/test/lisp/eshell/esh-arg-tests.el index b748c5ab4c0..209c4fa8ea9 100644 --- a/test/lisp/eshell/esh-arg-tests.el +++ b/test/lisp/eshell/esh-arg-tests.el @@ -181,6 +181,19 @@ chars." "setq eshell-test-value #>") (should (equal eshell-test-value marker))))) +(ert-deftest esh-arg-test/special-reference/command-form () + "Test that command forms inside special references work." + (with-temp-eshell + (let ((marker (make-marker)) + eshell-test-value) + (set-marker marker 1 (current-buffer)) + (eshell-insert-command + "setq eshell-test-value #") + (should (equal eshell-test-value marker)) + (eshell-insert-command + "setq eshell-test-value #>") + (should (equal eshell-test-value marker))))) + (ert-deftest esh-arg-test/special-reference/special-characters () "Test that \"#<...>\" works correctly when escaping special characters." (with-temp-buffer diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el index cac349a2616..9e4cbc58201 100644 --- a/test/lisp/eshell/esh-cmd-tests.el +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -325,6 +325,12 @@ processes correctly." (eshell-match-command-output "for i in 1 { echo $for-items }" "hello\n"))) +(ert-deftest esh-cmd-test/for-loop-lisp-body () + "Test invocation of a for loop with a Lisp body form." + (with-temp-eshell + (eshell-match-command-output "for i in 1 2 3 (format \"%s\" i)" + "1\n2\n3\n"))) + (ert-deftest esh-cmd-test/for-loop-pipe () "Test invocation of a for loop piped to another command." (skip-unless (executable-find "rev")) @@ -350,6 +356,15 @@ processes correctly." "{ setq eshell-test-value (1+ eshell-test-value) }") "1\n2\n3\n")))) +(ert-deftest esh-cmd-test/while-loop-lisp-body () + "Test invocation of a while loop using a Lisp form for the body." + (with-temp-eshell + (let ((eshell-test-value 0)) + (eshell-match-command-output + (concat "while (/= eshell-test-value 3) " + "(setq eshell-test-value (1+ eshell-test-value))") + "1\n2\n3\n")))) + (ert-deftest esh-cmd-test/while-loop-ext-cmd () "Test invocation of a while loop using an external command." (skip-unless (executable-find "[")) @@ -440,6 +455,17 @@ This tests when `eshell-lisp-form-nil-is-failure' is nil." (eshell-command-result-equal "if (zerop \"foo\") {echo yes} {echo no}" "no")))) +(ert-deftest esh-cmd-test/if-else-statement-lisp-body () + "Test invocation of an if/else statement using Lisp forms for the bodies." + (eshell-command-result-equal "if (zerop 0) (format \"yes\") (format \"no\")" + "yes") + (eshell-command-result-equal "if (zerop 1) (format \"yes\") (format \"no\")" + "no") + (let ((debug-on-error nil)) + (eshell-command-result-equal + "if (zerop \"foo\") (format \"yes\") (format \"no\")" + "no"))) + (ert-deftest esh-cmd-test/if-else-statement-ext-cmd () "Test invocation of an if/else statement using an external command." (skip-unless (executable-find "[")) diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el index 7b29e4a21db..7ac9807a1a7 100644 --- a/test/lisp/eshell/esh-var-tests.el +++ b/test/lisp/eshell/esh-var-tests.el @@ -190,6 +190,9 @@ nil, use FUNCTION instead." "zero") (eshell-command-result-equal "echo $eshell-test-value[${*echo 0} ${*echo 2}]" + '("zero" "two")) + (eshell-command-result-equal + "echo $eshell-test-value[{*echo 0} {*echo 2}]" '("zero" "two")))) (ert-deftest esh-var-test/interp-var-length-list () -- 2.39.5