From 7567f2b1c87f9fb8b74808f25dfa6e5f1d9f4ee8 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Sat, 18 Mar 2023 19:18:28 -0700 Subject: [PATCH] Avoid shadowing variables in some Eshell command forms * lisp/eshell/esh-cmd.el (eshell-rewrite-for-command): Make 'for-items' an uninterned symbol. (eshell-as-subcommand): Correct docstring. (eshell-do-command-to-value): Mark obsolete. (eshell-command-to-value): Move binding of 'value' outside of the macro's result, and remove call to 'eshell-do-command-to-value'. * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/subcommand-shadow-value) (esh-cmd-test/for-loop-for-items-shadow): New tests. (esh-cmd-test/for-name-loop, esh-cmd-test/for-name-shadow-loop): Rename to... (esh-cmd-test/for-loop-name, esh-cmd-test/for-loop-name-shadow): ... these. --- lisp/eshell/esh-cmd.el | 36 ++++++++++++++++++------------- test/lisp/eshell/esh-cmd-tests.el | 18 ++++++++++++++-- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 93f2616020c..c3e91e5ca70 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -531,9 +531,10 @@ eshell-rewrite-for-command implemented via rewriting, rather than as a function." (if (and (equal (car terms) "for") (equal (nth 2 terms) "in")) - (let ((body (car (last terms)))) + (let ((for-items (make-symbol "for-items")) + (body (car (last terms)))) (setcdr (last terms 2) nil) - `(let ((for-items + `(let ((,for-items (append ,@(mapcar (lambda (elem) @@ -541,13 +542,13 @@ eshell-rewrite-for-command elem `(list ,elem))) (nthcdr 3 terms))))) - (while 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-protect ,(eshell-invokify-arg body t))) - (setq for-items (cdr for-items))) + (setq ,for-items (cdr ,for-items))) (eshell-close-handles))))) (defun eshell-structure-basic-command (func names keyword test body @@ -890,28 +891,33 @@ eshell-execute-pipeline (symbol-value tailproc)))))) (defmacro eshell-as-subcommand (command) - "Execute COMMAND using a temp buffer. -This is used so that certain Lisp commands, such as `cd', when -executed in a subshell, do not disturb the environment of the main -Eshell buffer." + "Execute COMMAND as a subcommand. +A subcommand creates a local environment so that any changes to +the environment don't propagate outside of the subcommand's +scope. This lets you use commands like `cd' within a subcommand +without changing the current directory of the main Eshell +buffer." `(let ,eshell-subcommand-bindings ,command)) (defmacro eshell-do-command-to-value (object) "Run a subcommand prepared by `eshell-command-to-value'. This avoids the need to use `let*'." + (declare (obsolete nil "30.1")) `(let ((eshell-current-handles (eshell-create-handles value 'overwrite))) (progn ,object (symbol-value value)))) -(defmacro eshell-command-to-value (object) - "Run OBJECT synchronously, returning its result as a string. -Returns a string comprising the output from the command." - `(let ((value (make-symbol "eshell-temp")) - (eshell-in-pipeline-p nil)) - (eshell-do-command-to-value ,object))) +(defmacro eshell-command-to-value (command) + "Run an Eshell COMMAND synchronously, returning its output." + (let ((value (make-symbol "eshell-temp"))) + `(let ((eshell-in-pipeline-p nil) + (eshell-current-handles + (eshell-create-handles ',value 'overwrite))) + ,command + ,value))) ;;;_* Iterative evaluation ;; diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el index 94763954622..a7208eb3a0b 100644 --- a/test/lisp/eshell/esh-cmd-tests.el +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -73,6 +73,13 @@ esh-cmd-test/subcommand-lisp e.g. \"{(+ 1 2)} 3\" => 3" (eshell-command-result-equal "{(+ 1 2)} 3" 3)) +(ert-deftest esh-cmd-test/subcommand-shadow-value () + "Test that the variable `value' isn't shadowed inside subcommands." + (with-temp-eshell + (with-no-warnings (setq-local value "hello")) + (eshell-match-command-output "echo ${echo $value}" + "hello\n"))) + (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 @@ -151,13 +158,13 @@ esh-cmd-test/for-loop-multiple-args (eshell-match-command-output "for i in 1 2 (list 3 4) { echo $i }" "1\n2\n3\n4\n"))) -(ert-deftest esh-cmd-test/for-name-loop () ; bug#15231 +(ert-deftest esh-cmd-test/for-loop-name () ; bug#15231 "Test invocation of a for loop using `name'." (let ((process-environment (cons "name" process-environment))) (eshell-command-result-equal "for name in 3 { echo $name }" 3))) -(ert-deftest esh-cmd-test/for-name-shadow-loop () ; bug#15372 +(ert-deftest esh-cmd-test/for-loop-name-shadow () ; bug#15372 "Test invocation of a for loop using an env-var." (let ((process-environment (cons "name=env-value" process-environment))) (with-temp-eshell @@ -165,6 +172,13 @@ esh-cmd-test/for-name-shadow-loop "echo $name; for name in 3 { echo $name }; echo $name" "env-value\n3\nenv-value\n")))) +(ert-deftest esh-cmd-test/for-loop-for-items-shadow () + "Test that the variable `for-items' isn't shadowed inside for loops." + (with-temp-eshell + (with-no-warnings (setq-local for-items "hello")) + (eshell-match-command-output "for i in 1 { echo $for-items }" + "hello\n"))) + (ert-deftest esh-cmd-test/for-loop-pipe () "Test invocation of a for loop piped to another command." (skip-unless (executable-find "rev")) -- 2.25.1