GNU bug report logs - #59469
29.0.50; Eshell "for" loop: Calling a non-lisp command (example: /usr/bin/tail) sets the variable exported in the {} block of "for var in list {}" to nil

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 59469 in the body.
You can then email your comments to 59469 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Milan Zimmermann <milan.zimmermann <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Eshell "for" loop: Calling a non-lisp command (example:
 /usr/bin/tail) sets the variable exported in the {} block of "for var in list
 {}" to nil
Date: Mon, 21 Nov 2022 21:04:51 -0500

[Message part 1 (text/plain, inline)]

Hi,
I might be doing something wrong, but it appears that calling any non-lisp
command  (example: /usr/bin/tail) causes the variable set in the {} block
of "for var in list {}" to change to nil.

To duplicate, please create a file "bug.txt" in ~tmp,

   echo "line one" > ~/tmp/bug.txt

then run the following code -

In the code below, *please replace my full path
"/home/mzimmermann/tmp/bug.txt" with your full path. There seems to be an
unrelated issue trying to list ~/tmp/bug.txt*

  for file in (list "/home/mzimmermann/tmp/bug.txt") {
                  type-of $file
                  echo "file=$file";
                  # Create a variable. The back and forth between .log and
.txt is
                  # only there to simulate some useful work to set value of
samefile
                  export samefile="${echo
$file(:s/.txt/.log/)}"(:s/.log/.txt/);
                  echo "samefile=$samefile"
                  # Note that on my system, 'which tail' ==> /usr/bin/tail
                  tail $file
                  # Note that on my system, 'which cat' ==> eshell/cat is a
byte-compiled Lisp function in ‘em-unix.el’.
                  # cat $file
                  echo "samefile=$samefile"
                  echo "file=$file";
              }

Actual result:

================
string
file=/home/mzimmermann/tmp/bug.txt
samefile=/home/mzimmermann/tmp/bug.txt
line one
samefile=nil
file=/home/mzimmermann/tmp/bug.txt
================

Please *note how the contents of samefile is nullified. This is the bug I
am reporting here*

Expected result:
================
string
file=/home/mzimmermann/tmp/bug.txt
samefile=/home/mzimmermann/tmp/bug.txt
line one
samefile=/home/mzimmermann/tmp/bug.txt
file=/home/mzimmermann/tmp/bug.txt
================

Notes:

1. I tried this with other non-lisp functions (/usr/bin/gzip) and it caused
the same issue.

2. If we use "cat" which is an elisp function, the loop works as expected.

Thanks,
Milan Zimmermann

[Message part 2 (text/html, inline)]

Message #8 received at 59469 <at> debbugs.gnu.org (full text, mbox):

From: Milan Zimmermann <milan.zimmermann <at> gmail.com>
To: 59469 <at> debbugs.gnu.org
Subject: Adding a simpler duplication of the issue
Date: Mon, 21 Nov 2022 21:50:03 -0500

[Message part 1 (text/plain, inline)]

A simpler duplication shows the issue is below.

It appears the issue is  is not related to the "for loop" - it can be
duplicated just writing code inside the eshell { .. } block.

To duplicate, please create a file ccc.el.gz - something the gzip does not
fail on.

Then, On the eshell prompt, enter the following code:

 $  export aaa="This is contents of aaa"
 {
# create a variable in this block
export bbb=$aaa;
echo "Before gzip: aaa=$aaa"
echo "Before gzip: bbb=$bbb";
gzip --decompress ccc.el.gz;
echo "After gzip: aaa=$aaa"
echo "After gzip: bbb=$bbb";
}
Before gzip: aaa=This is contents of aaa
Before gzip: bbb=This is contents of aaa
After gzip: aaa=This is contents of aaa
After gzip: bbb=nil
 $

Same bug: After the call to non-elisp program, /usr/bin/gzip, a previously
exported variable bbb (exported inside the block) is nullified.

Thanks

[Message part 2 (text/html, inline)]

Message #11 received at 59469 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Milan Zimmermann <milan.zimmermann <at> gmail.com>, 59469 <at> debbugs.gnu.org
Subject: Re: bug#59469: Adding a simpler duplication of the issue
Date: Mon, 21 Nov 2022 20:56:16 -0800

On 11/21/2022 6:50 PM, Milan Zimmermann wrote:
> A simpler duplication shows the issue is below.
[snip]
> Same bug: After the call to non-elisp program, /usr/bin/gzip, a 
> previously exported variable bbb (exported inside the block) is nullified.

I'm not entirely sure, but I have a suspicion that this is due to 
Eshell's deferred commands. Deferred commands tell Eshell to stop 
processing synchronously and yield to the rest of Emacs. It's a way of 
keeping long-running commands (e.g. external processes) from hanging the 
rest of Emacs.

Unfortunately, the logic to do this (see 'eshell-do-eval') was written 
before lexical binding was added to Emacs Lisp, and I think this is the 
cause of quite a few subtle bugs with Eshell command evaluation. Fixing 
that is bug#57635, which would leverage the generator.el internals to do 
this.

Of course, I could be wrong. This is reaching well past my comfort zone 
for Emacs Lisp, but this sure seems like an issue with 'eshell-do-eval'.

I'd certainly like to fix this one day, since it's blocking a few other 
things I want to do in Eshell, but I think it'll be a pretty big project.

Message #14 received at 59469 <at> debbugs.gnu.org (full text, mbox):

From: Milan Zimmermann <milan.zimmermann <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 59469 <at> debbugs.gnu.org
Subject: Re: bug#59469: Adding a simpler duplication of the issue
Date: Tue, 22 Nov 2022 02:18:00 -0500

[Message part 1 (text/plain, inline)]

Thanks for the detailed update as always.

This is over my elisp level, I got here mostly as a relapsed eshell user,
trying to use eshell as my primary shell for the third time :)

But it sounds to me like your intuition about this could be fixed by
rewriting the core 'eshell-do-eval' loop in bug#57635 can be correct.  I
would enjoy helping with  it, but at the moment it is above my time and
elisp abilities.

Not sure what to do next regarding this bug, perhaps we should go ahead and
add your comment to   bug#57635 so these two are linked from both ends? Or
let me know if I can help with something else,

Thanks,
Milan


On Mon, Nov 21, 2022 at 11:56 PM Jim Porter <jporterbugs <at> gmail.com> wrote:

> On 11/21/2022 6:50 PM, Milan Zimmermann wrote:
> > A simpler duplication shows the issue is below.
> [snip]
> > Same bug: After the call to non-elisp program, /usr/bin/gzip, a
> > previously exported variable bbb (exported inside the block) is
> nullified.
>
> I'm not entirely sure, but I have a suspicion that this is due to
> Eshell's deferred commands. Deferred commands tell Eshell to stop
> processing synchronously and yield to the rest of Emacs. It's a way of
> keeping long-running commands (e.g. external processes) from hanging the
> rest of Emacs.
>
> Unfortunately, the logic to do this (see 'eshell-do-eval') was written
> before lexical binding was added to Emacs Lisp, and I think this is the
> cause of quite a few subtle bugs with Eshell command evaluation. Fixing
> that is bug#57635, which would leverage the generator.el internals to do
> this.
>
> Of course, I could be wrong. This is reaching well past my comfort zone
> for Emacs Lisp, but this sure seems like an issue with 'eshell-do-eval'.
>
> I'd certainly like to fix this one day, since it's blocking a few other
> things I want to do in Eshell, but I think it'll be a pretty big project.
>

[Message part 2 (text/html, inline)]

Message #17 received at 59469 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Milan Zimmermann <milan.zimmermann <at> gmail.com>
Cc: 59469 <at> debbugs.gnu.org
Subject: Re: bug#59469: Adding a simpler duplication of the issue
Date: Tue, 24 Jan 2023 17:39:26 -0800

[Message part 1 (text/plain, inline)]

On 11/21/2022 11:18 PM, Milan Zimmermann wrote:
> But it sounds to me like your intuition about this could be fixed by 
> rewriting the core 'eshell-do-eval' loop in bug#57635 can be correct.  I 
> would enjoy helping with  it, but at the moment it is above my time and 
> elisp abilities.

After digging through 'eshell-do-eval' for another issue, I think I 
mostly understand it now. I still think bug#57635 is the way to go 
long-term (either that or use real threads), but since that's a big 
change, it might be best to give users some time where they could opt 
out of some new Eshell evaluation backend, just in case we break 
something with it. As such, I want to make sure the existing backend is 
as bug-free as possible so that (ideally) switching between the two has 
no behavioral difference.

With that said, here's a patch that fixes this, plus a regression test. 
One question for anyone reviewing the patch: is there a better way to do 
the "catch and rethrow" dance I do in 'eshell-do-eval'? It seems kind of 
clumsy...

----------------------------------------

The patch has a description of the problem, but I'll describe it in more 
detail here for anyone curious. Eshell turns the command in the original 
message into a form like this (note: this is very approximate):

  (let ((process-environment process-environment))
    (setenv "var" "value")
    (eshell-external-command "grep") ; This throws 'eshell-defer'
    (eshell/echo (getenv "var")))

'eshell-do-eval' gradually steps through this form, evaluating subforms 
and replacing them with their (quoted) results. This way, when a command 
throws 'eshell-defer', you can resume this form simply by calling 
'eshell-do-eval' again. So at the point 'eshell-defer' gets thrown, the 
form has been updated to:

  (let ((process-environment process-environment))
    '"value"   ; The quoted result of 'setenv' above.
    (eshell-external-command "grep")
    (eshell/echo (getenv "var")))

But since 'process-environment' is let-bound, when we resume evaluation, 
it's as though 'setenv' had never been called at all!

The fix here is that when we're inside a 'let' and see 'eshell-defer' 
get thrown, update the let-bindings in place. So now the updated form 
would look like:

  (let ((process-environment (cons "var=value" process-environment)))
    '"value"   ; Not really necessary, but it doesn't hurt anything.
    (eshell-external-command "grep")
    (eshell/echo (getenv "var")))

And so with this, it all works.

[0001-Ensure-that-deferred-commands-don-t-make-Eshell-forg.patch (text/plain, attachment)]

Message #20 received at 59469 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Milan Zimmermann <milan.zimmermann <at> gmail.com>
Cc: 59469 <at> debbugs.gnu.org
Subject: Re: bug#59469: 29.0.50; Eshell "for" loop: Calling a non-lisp command
 (example: /usr/bin/tail) sets the variable exported in the {} block
 of "for
 var in list {}" to nil (was: Adding a simpler duplication of the issue)
Date: Sat, 28 Jan 2023 22:55:37 -0800

[Message part 1 (text/plain, inline)]

(Restoring the original subject so this issue is hopefully easier to track.)

On 1/24/2023 5:39 PM, Jim Porter wrote:
> The fix here is that when we're inside a 'let' and see 'eshell-defer' 
> get thrown, update the let-bindings in place. So now the updated form 
> would look like:
> 
>    (let ((process-environment (cons "var=value" process-environment)))
>      '"value"   ; Not really necessary, but it doesn't hurt anything.
>      (eshell-external-command "grep")
>      (eshell/echo (getenv "var")))
> 
> And so with this, it all works.

Here's an updated patch with an improved comment introducing Eshell's 
iterative evaluation. Hopefully the added detail will help anyone else 
who looks into this code (assuming I haven't replaced it with threads or 
generator.el's CPS machinery by then, of course!).

Note: I have a few other patches to apply after this that should let 
'eshell-do-eval' handle "normal" Emacs Lisp for the most part. This will 
make it easier to maintain all the code that generates Eshell's internal 
forms, as well as simplifying the migration to threads and/or 
generator.el. I'll file those separately though, since this is tricky 
code, and I want to be sure each change gets a reasonable amount of 
attention.

[0001-Ensure-that-deferred-commands-don-t-make-Eshell-forg.patch (text/plain, attachment)]

Message #25 received at 59469-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Milan Zimmermann <milan.zimmermann <at> gmail.com>
Cc: 59469-done <at> debbugs.gnu.org
Subject: Re: bug#59469: 29.0.50; Eshell "for" loop: Calling a non-lisp command
 (example: /usr/bin/tail) sets the variable exported in the {} block
 of "for
 var in list {}" to nil (was: Adding a simpler duplication of the issue)
Date: Thu, 9 Feb 2023 21:44:15 -0800

On 1/28/2023 10:55 PM, Jim Porter wrote:
> Here's an updated patch with an improved comment introducing Eshell's 
> iterative evaluation. Hopefully the added detail will help anyone else 
> who looks into this code (assuming I haven't replaced it with threads or 
> generator.el's CPS machinery by then, of course!).

Merged as c53255f677. Closing this bug now.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.