From: Jim Porter <jporterbugs@gmail.com>
To: 62452@debbugs.gnu.org
Subject: bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms
Date: Sat, 25 Mar 2023 21:48:13 -0700 [thread overview]
Message-ID: <81f70b2b-5ab9-ba7d-1e9f-551e47d72812@gmail.com> (raw)
[-- Attachment #1: Type: text/plain, Size: 390 bytes --]
To see this in action, start from "emacs -Q -f eshell", then:
~ $ setq value "hi there"
hi there
~ $ echo $value
hi there
~ $ echo ${echo $value}
eshell-temp
The last command should *also* print "hi there", but the variable
'value' gets shadowed by some internal Eshell code. (A similar problem
occurs for 'for-items' when using a for loop in Eshell.)
Patch attached.
[-- Attachment #2: 0001-Avoid-shadowing-variables-in-some-Eshell-command-for.patch --]
[-- Type: text/plain, Size: 6303 bytes --]
From 7567f2b1c87f9fb8b74808f25dfa6e5f1d9f4ee8 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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
next reply other threads:[~2023-03-26 4:48 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-26 4:48 Jim Porter [this message]
2023-04-01 23:31 ` bug#62452: 30.0.50; [PATCH] Avoid shadowing variables in some Eshell command forms Jim Porter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=81f70b2b-5ab9-ba7d-1e9f-551e47d72812@gmail.com \
--to=jporterbugs@gmail.com \
--cc=62452@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).