Bug report

Bug report

[mvxxc]

If the following script is run in an empty directory, zsh executes the defined
function only once.

  #!/usr/bin/zsh
  
  set -e
  setopt NULL_GLOB
  
  func () {for i in _*; do echo $i; done; echo _;} 
  
  func
  if false; then
    :
  else
    func
  fi  

After adding one line, the function is executed twice.

  [...]
  else
    :
    func
  fi

"set -e" and "NULL_GLOB" seem to affect both function calls in a different way.
This could be a bug, as the answer to my question on Stack Overflow suggests.

  https://stackoverflow.com/questions/27648773.

MVXXC

Re: Bug report

[Bart Schaefer]

On Dec 26,  5:53pm, mvxxc@gmx.de wrote:
} Subject: Bug report
}
} If the following script is run in an empty directory, zsh executes the
} defined function only once.

Changing this line:

}   set -e

To "set -exv" shows that func is in fact called twice but exits without
doing anything on the second call.

I therefore suspect one of workers/32568 or workers/32569.

This is very tricky because we have to propagate the value of $? from
before the "for" keyword (including before the start of the function)
into the "do" body, but must *not* *use* the value of $? for deciding
whether to exit-on-error, because the nonzero value came from an "if"
test.  There is a "noerrexit" flag that is supposed to cover this case,
but it's not set when the very first "sublist" in a function body is
a for-loop (there are likely to be other similar cases).

[PATCH] ERR_EXIT with "for" loops and shell functions (Re: Bug report)

[Bart Schaefer]

On Dec 26,  6:35pm, Bart Schaefer wrote:
}
} This is very tricky because we have to propagate the value of $? from
} before the "for" keyword (including before the start of the function)
} into the "do" body, but must *not* *use* the value of $? for deciding
} whether to exit-on-error, because the nonzero value came from an "if"
} test.  There is a "noerrexit" flag that is supposed to cover this case,
} but it's not set when the very first "sublist" in a function body is
} a for-loop (there are likely to be other similar cases).

There's a further complication here, which is this:

    zsh -fc 'false; for x; do :; done; echo $?'
    1

A "for" loop which executes zero times is supposed to set $? == 0, or
at least it does so in bash.  Zsh 5.0.7 passes through $? unchanged when
the "for" does not loop.

Oddly, the current version from git behaves differently:

    Src/zsh -fc 'false; for x in; do false; done; echo $?' 
    0

But:

    Src/zsh -o null_glob -fc 'false; for x in _*; do false; done; echo $?'   
    1

I haven't traced through why a missing "in" list resets $? to zero,
whereas an empty list created by globbing passes through the outer $?.

Here's another difference from bash:

    bash -c 'false; select x in; do false; done; echo $?'
    bash: -c: line 0: syntax error near unexpected token `;'
    bash: -c: line 0: `false; select x in; do false; done; echo $?'

whereas zsh accepts a variety of syntax with inconsistent return: 

    zsh -fc 'true; select x; do :; done; echo $?'
    1
    zsh -fc 'true; select x in; do :; done; echo $?'
    0
    zsh -fc 'true; select x in $empty; do :; done; echo $?'
    1

The first and last of those three also pass syntax in bash and give a
loop that executes zero times and sets $? to zero rather than one.

The following patch attempts to fix all of this weirdness.  I'm a bit
concerned that there may still be cases where prefork substitutions
are performed when they actually should not be because the errexit
[not noerrexit] condition has not been tested soon enough -- it seems
as if zsh is checking errexit at the start of the next command rather
than immediately following the command that returned nonzero, in at
least some circumstances -- but I can't come up with a test case.

Possibly this whole thing needs to be rethought.

Anyway, the most controversial thing below is probably the change in the
return status of "select", which is now always zero unless something in
the loop body returns nonzero.  If we want to disallow empty-"in" syntax
we should do it elsewhere.


diff --git a/Src/exec.c b/Src/exec.c
index 6a7dbb1..eaf73df 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2632,6 +2632,10 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	}
     }
 
+    /* if we get this far, it is OK to pay attention to lastval again */
+    if (noerrexit == 2 && !is_shfunc)
+	noerrexit = 0;
+
     /* Do prefork substitutions */
     esprefork = (assign || isset(MAGICEQUALSUBST)) ? PREFORK_TYPESET : 0;
     if (args && htok)
diff --git a/Src/loop.c b/Src/loop.c
index 8bb1ec9..7b3bdd2 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -102,7 +102,10 @@ execfor(Estate state, int do_exec)
 		addlinknode(args, dupstring(*x));
 	}
     }
-    /* lastval = 0; */
+
+    if (!args || empty(args))
+	lastval = 0;
+
     loops++;
     pushheap();
     cmdpush(CS_FOR);
@@ -238,10 +241,10 @@ execselect(Estate state, UNUSED(int do_exec))
     }
     if (!args || empty(args)) {
 	state->pc = end;
-	return 1;
+	return 0;
     }
     loops++;
-    /* lastval = 0; */
+
     pushheap();
     cmdpush(CS_SELECT);
     usezle = interact && SHTTY != -1 && isset(USEZLE);
@@ -519,14 +522,17 @@ execif(Estate state, int do_exec)
 	s = 1;
 	state->pc = next;
     }
-    noerrexit = olderrexit;
 
     if (run) {
+	/* we need to ignore lastval until we reach execcmd() */
+	noerrexit = olderrexit ? olderrexit : lastval ? 2 : 0;
 	cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
 	execlist(state, 1, do_exec);
 	cmdpop();
-    } else
+    } else {
+	noerrexit = olderrexit;
 	lastval = 0;
+    }
     state->pc = end;
 
     return lastval;

-- 
Barton E. Schaefer

Re: [PATCH] ERR_EXIT with "for" loops and shell functions (Re: Bug report)

[Peter Stephenson]

On Sat, 27 Dec 2014 16:32:20 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Dec 26,  6:35pm, Bart Schaefer wrote:
> }
> } This is very tricky because we have to propagate the value of $? from
> } before the "for" keyword (including before the start of the function)
> } into the "do" body, but must *not* *use* the value of $? for deciding
> } whether to exit-on-error, because the nonzero value came from an "if"
> } test.  There is a "noerrexit" flag that is supposed to cover this case,
> } but it's not set when the very first "sublist" in a function body is
> } a for-loop (there are likely to be other similar cases).
> 
> The following patch attempts to fix all of this weirdness.

I had a look at this myself while in 2Gnetworkland, and sent a couple of
messages which didn't get through quite possibly because the gmail app
sent them to zsh-hackers@zsh.org for undetermined reasons.

However, you must have found everything I did since the tests I added
now pass.  They're all just for status, nothing specific to ERR_EXIT or
ERR_RETURN; I didn't find I needed to change anything there in the main
code, just for the status value itself.  They overlap with yours but that's no big deal.  Here they are.

diff --git a/Test/A07control.ztst b/Test/A07control.ztst
index 397a821..b1a2487 100644
--- a/Test/A07control.ztst
+++ b/Test/A07control.ztst
@@ -110,3 +110,56 @@
   done
 1:break error case -1
 ?(eval):break:2: argument is not positive: -1
+
+  false
+  for x in; do
+    print nothing executed
+  done
+0:Status 0 from for with explicit empty list
+
+  set --
+  false
+  for x; do
+    print nothing executed
+  done
+0:Status 0 from for with implicit empty list
+
+  (exit 2)
+  for x in 1 2; do
+    print $?
+  done
+0:Status from previous command propagated into for loop
+>2
+>0
+
+  false
+  for x in $(echo 1 2; (exit 3)); do
+    print $?
+  done
+0:Status from expansion propagated into for loop
+>3
+>0
+
+  false
+  for x in $(exit 4); do
+    print not executed
+  done
+0:Status from expansion not propagated after unexecuted for loop
+  
+  false
+  for x in NonExistentFilePrefix*(N); do
+    print not executed, either
+  done
+0:Status from before for loop not propagated if empty after expansion
+
+  for x in $(echo 1; false); do
+  done
+0:Status reset by empty list in for loop
+
+  false
+  for x in $(echo 1; false); do
+    echo $?
+    (exit 4)  
+  done
+4:Last status from loop body is kept even with other funny business going on
+>1

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: [PATCH] ERR_EXIT with "for" loops and shell functions (Re: Bug report)

[Bart Schaefer]

On Jan 2,  4:56pm, Peter Stephenson wrote:
}
} On Sat, 27 Dec 2014 16:32:20 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > The following patch attempts to fix all of this weirdness.
} 
} However, you must have found everything I did since the tests I added
} now pass. They're all just for status, nothing specific to ERR_EXIT
} or ERR_RETURN; I didn't find I needed to change anything there in the
} main code, just for the status value itself. They overlap with yours
} but that's no big deal. Here they are.

They don't really overlap with mine much at all, because they examine
the normal cases whereas I looked at the ERR_EXIT case.

This one:

} +  set --
} +  false
} +  for x; do
} +    print nothing executed
} +  done
} +0:Status 0 from for with implicit empty list

Makes me wonder if my similar test of "select" needs an explicit "set --"
as well?

Re: [PATCH] ERR_EXIT with "for" loops and shell functions (Re: Bug report)

[Peter Stephenson]

On Fri, 02 Jan 2015 10:56:13 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This one:
> 
> } +  set --
> } +  false
> } +  for x; do
> } +    print nothing executed
> } +  done
> } +0:Status 0 from for with implicit empty list
> 
> Makes me wonder if my similar test of "select" needs an explicit "set --"
> as well?

Obviously wouldn't hurt, but the chances of extra arguments creeping
into the function that's executing the test is probably fairly low.

pws