bug: $? after empty command

bug: $? after empty command

[Eric Blake]

POSIX requires that the execution of an empty command change $? to 0, as shown
here with bash:

$ zsh -c 'foo=; false; $foo; echo $?'
1
$ bash -c 'foo=; false; $foo; echo $?'
0

-- 
Eric Blake

Re: bug: $? after empty command

[Peter Stephenson]

On Thu, 9 Jul 2009 13:12:20 +0000 (UTC)
Eric Blake <ebb9@byu.net> wrote:
> POSIX requires that the execution of an empty command change $? to 0, as shown
> here with bash:
> 
> $ zsh -c 'foo=; false; $foo; echo $?'
> 1

I'm not aware that that's deliberate shell behaviour, and indeed I
wouldn't have expected it.  It *is* deliberate behaviour that the status is
not reset simply by hitting return at the interactive prompt, but that's
actually a different issue, not affected by the following.

The fix for this is quite subtle because of the case

  zsh -c 'false; $(exit 3); echo $?'

which should print 3, but I think I've finally found a way that handles
both.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.165
diff -u -r1.165 exec.c
--- Src/exec.c	15 Mar 2009 01:04:50 -0000	1.165
+++ Src/exec.c	9 Jul 2009 14:38:41 -0000
@@ -151,6 +151,15 @@
 /**/
 int cmdoutval;
 
+/*
+ * This is set by an exiting $(...) substitution to indicate we need
+ * to retain the status.  We initialize it to zero if we think we need
+ * to reset the status for a command.
+ */
+
+/**/
+int use_cmdoutval;
+
 /* The context in which a shell function is called, see SFC_* in zsh.h. */ 
 
 /**/
@@ -2262,6 +2271,14 @@
      */
     if (!args && varspc)
 	lastval = errflag ? errflag : cmdoutval;
+    /*
+     * If there are arguments, we should reset the status for the
+     * command before execution---unless we are using the result of a
+     * command substitution, which will be indicated by setting
+     * use_cmdoutval to 1.  We haven't kicked those off yet, so
+     * there's no race.
+     */
+    use_cmdoutval = !args;
 
     for (i = 0; i < 10; i++) {
 	save[i] = -2;
@@ -2478,7 +2495,12 @@
 		    lastval = 0;
 		    return;
 		} else {
-		    cmdoutval = lastval;
+		    /*
+		     * No arguments.  Reset the status if there were
+		     * arguments before and no command substitution
+		     * has provided a status.
+		     */
+		    cmdoutval = use_cmdoutval ? lastval : 0;
 		    if (varspc)
 			addvars(state, varspc, 0);
 		    if (errflag)
@@ -4674,6 +4696,7 @@
     es->badcshglob = badcshglob;
     es->cmdoutpid = cmdoutpid;
     es->cmdoutval = cmdoutval;
+    es->use_cmdoutval = use_cmdoutval;
     es->trap_return = trap_return;
     es->trap_state = trap_state;
     es->trapisfunc = trapisfunc;
@@ -4704,6 +4727,7 @@
     badcshglob = exstack->badcshglob;
     cmdoutpid = exstack->cmdoutpid;
     cmdoutval = exstack->cmdoutval;
+    use_cmdoutval = exstack->use_cmdoutval;
     trap_return = exstack->trap_return;
     trap_state = exstack->trap_state;
     trapisfunc = exstack->trapisfunc;
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.54
diff -u -r1.54 signals.c
--- Src/signals.c	11 Feb 2009 20:42:16 -0000	1.54
+++ Src/signals.c	9 Jul 2009 14:38:41 -0000
@@ -494,6 +494,7 @@
 			*procsubval = (0200 | WTERMSIG(status));
 		    else
 			*procsubval = WEXITSTATUS(status);
+		    use_cmdoutval = 1;
 		    get_usage();
 		    cont = 1;
 		    break;
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.158
diff -u -r1.158 zsh.h
--- Src/zsh.h	2 Jul 2009 13:48:36 -0000	1.158
+++ Src/zsh.h	9 Jul 2009 14:38:41 -0000
@@ -930,6 +930,7 @@
     int badcshglob;
     pid_t cmdoutpid;
     int cmdoutval;
+    int use_cmdoutval;
     int trap_return;
     int trap_state;
     int trapisfunc;
Index: Test/A01grammar.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A01grammar.ztst,v
retrieving revision 1.22
diff -u -r1.22 A01grammar.ztst
--- Test/A01grammar.ztst	6 Jul 2009 20:44:29 -0000	1.22
+++ Test/A01grammar.ztst	9 Jul 2009 14:38:41 -0000
@@ -23,6 +23,10 @@
   true | false
 1:Exit status of pipeline with builtins (false)
 
+  false
+  $nonexistent_variable
+0:Executing command that evaluates to empty resets status
+
   fn() { local foo; read foo; print $foo; }
   coproc fn
   print -p coproc test output


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: bug: $? after empty command

[Eric Blake]

Peter Stephenson <pws <at> csr.com> writes:

> > POSIX requires that the execution of an empty command change $? to 0, as
> > shown here with bash:
> > 
> > $ zsh -c 'foo=; false; $foo; echo $?'
> > 1
> 
> I'm not aware that that's deliberate shell behaviour, and indeed I
> wouldn't have expected it.  It *is* deliberate behaviour that the status is
> not reset simply by hitting return at the interactive prompt, but that's
> actually a different issue, not affected by the following.

Good point about making the behavior dependent on a non-empty command line, and
that the exit status of command substitution is still important.  I don't see
your patch in CVS yet, so I haven't played with it.  But just from inspection,
it looks like it does not cover these related issues, which are both required by
POSIX to output 0:

$ zsh -c 'false; . /dev/null; echo $?'
1
$ zsh -c 'false; ``; echo $?'
1

-- 
Eric Blake

Re: bug: $? after empty command

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> But just from inspection,
> it looks like it does not cover these related issues, which are both required
> by POSIX to output 0:
> 
> $ zsh -c 'false; . /dev/null; echo $?'
> 1
> $ zsh -c 'false; ``; echo $?'
> 1

And another one that POSIX requires to output 0:

$ zsh -c 'false; sleep& echo $?'
1

-- 
Eric Blake

Re: bug: $? after empty command

[Peter Stephenson]

On Thu, 9 Jul 2009 21:41:51 +0000 (UTC)
Eric Blake <ebb9@byu.net> wrote:
> Eric Blake <ebb9 <at> byu.net> writes:
> 
> > But just from inspection,
> > it looks like it does not cover these related issues, which are both required
> > by POSIX to output 0:
> > 
> > $ zsh -c 'false; . /dev/null; echo $?'
> > 1
> > $ zsh -c 'false; ``; echo $?'
> > 1
> 
> And another one that POSIX requires to output 0:
> 
> $ zsh -c 'false; sleep& echo $?'
> 1

These still need fixing.

Also, I don't think it affects POSIX, because empty functions aren't
required to work, but in zsh, "fn() { }" is valid, and I suspect "fn"
logically ought to reset the status, too---basically anywhere where you've
run a complex command with an empty list of commands in it.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: bug: $? after empty command

[Peter Stephenson]

On Fri, 10 Jul 2009 10:02:28 +0100
Peter Stephenson <pws@csr.com> wrote:
> On Thu, 9 Jul 2009 21:41:51 +0000 (UTC)
> Eric Blake <ebb9@byu.net> wrote:
>> Eric Blake <ebb9 <at> byu.net> writes:
>> 
>>> But just from inspection,
>>> it looks like it does not cover these related issues, which are
>>> both required by POSIX to output 0:
>>> 
>>> $ zsh -c 'false; . /dev/null; echo $?'
>>> 1
>>> $ zsh -c 'false; ``; echo $?'
>>> 1
>> 
>> And another one that POSIX requires to output 0:
>> 
>> $ zsh -c 'false; sleep& echo $?'
>> 1
> 
> Also, I don't think it affects POSIX, because empty functions aren't
> required to work, but in zsh, "fn() { }" is valid, and I suspect "fn"
> logically ought to reset the status, too---basically anywhere where you've
> run a complex command with an empty list of commands in it.

These four were all fairly easy; in each case there is a local context
where the command in question is being executed in which the status can
be reset.  There may be a a more generic place, such as execlist(), for
these to go, but I like the transparency of resetting them locally;
execlist()/execode() are used in just too many places.  However, that's
a bit craven.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.166
diff -u -r1.166 exec.c
--- Src/exec.c	10 Jul 2009 11:08:48 -0000	1.166
+++ Src/exec.c	10 Jul 2009 21:59:52 -0000
@@ -1372,7 +1372,8 @@
 	else
 	    spawnjob();
 	child_unblock();
-	return 0;
+	/* Executing background code resets shell status */
+	return lastval = 0;
     } else {
 	if (newjob != lastwj) {
 	    Job jn = jobtab + newjob;
@@ -3512,6 +3513,7 @@
 	return retval;
     }
     /* pid == 0 */
+    lastval = 0;		/* status of empty list is zero */
     child_unblock();
     zclose(pipes[0]);
     redup(pipes[1], 1);
@@ -4259,6 +4261,7 @@
     if (trap_state == TRAP_STATE_PRIMED)
 	trap_return--;
     oldlastval = lastval;
+    lastval = 0;		/* status of empty function is zero */
     oldnumpipestats = numpipestats;
     if (noreturnval) {
 	/*
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.103
diff -u -r1.103 init.c
--- Src/init.c	9 Jul 2009 19:42:20 -0000	1.103
+++ Src/init.c	10 Jul 2009 21:59:52 -0000
@@ -1131,6 +1131,7 @@
     fstack.tp = FS_SOURCE;
     funcstack = &fstack;
 
+    lastval = 0;		/* status of empty file is zero */
     if (prog) {
 	pushheap();
 	errflag = 0;
Index: Test/A01grammar.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A01grammar.ztst,v
retrieving revision 1.23
diff -u -r1.23 A01grammar.ztst
--- Test/A01grammar.ztst	10 Jul 2009 11:08:48 -0000	1.23
+++ Test/A01grammar.ztst	10 Jul 2009 21:59:52 -0000
@@ -27,6 +27,18 @@
   $nonexistent_variable
 0:Executing command that evaluates to empty resets status
 
+  false
+  sleep 1 &
+  print $?
+  # a tidy test is a happy test
+  wait $!
+0:Starting background command resets status
+>0
+
+  false
+  . /dev/null
+0:Sourcing empty file resets status
+
   fn() { local foo; read foo; print $foo; }
   coproc fn
   print -p coproc test output
@@ -531,3 +543,13 @@
   . ./bad_syntax
 126: Attempt to "." file with bad syntax.
 ?./bad_syntax:2: parse error near `\n'
+
+  echo 'false' >dot_false
+  . ./dot_false
+  print $?
+  echo 'true' >dot_true
+  . ./dot_true
+  print $?
+0:Last status of successfully executed "." file is retained
+>1
+>0
Index: Test/C04funcdef.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C04funcdef.ztst,v
retrieving revision 1.5
diff -u -r1.5 C04funcdef.ztst
--- Test/C04funcdef.ztst	29 Dec 2008 04:24:36 -0000	1.5
+++ Test/C04funcdef.ztst	10 Jul 2009 21:59:52 -0000
@@ -1,5 +1,20 @@
 %test
 
+  fn1() { return 1; }
+  fn2() {
+      fn1
+      print $?
+      return 2
+  }
+  fn2
+2:Basic status returns from functions
+>1
+
+  fnz() { }
+  false
+  fnz
+0:Empty function body resets status
+
   function f$$ () {
     print regress expansion of function names
   }
Index: Test/D08cmdsubst.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/D08cmdsubst.ztst,v
retrieving revision 1.2
diff -u -r1.2 D08cmdsubst.ztst
--- Test/D08cmdsubst.ztst	9 Dec 2007 23:53:33 -0000	1.2
+++ Test/D08cmdsubst.ztst	10 Jul 2009 21:59:52 -0000
@@ -89,3 +89,7 @@
   X=$(exit 2) $(exit 0) || print $?
 0:variable assignments processed after other substitutions
 >2
+
+ false
+ ``
+0:Empty command substitution resets status


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

Re: bug: $? after empty command

[Peter Stephenson]

On Fri, 10 Jul 2009 23:05:54 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Fri, 10 Jul 2009 10:02:28 +0100
> Peter Stephenson <pws@csr.com> wrote:
> > On Thu, 9 Jul 2009 21:41:51 +0000 (UTC)
> > Eric Blake <ebb9@byu.net> wrote:
> >> Eric Blake <ebb9 <at> byu.net> writes:
> >>> $ zsh -c 'false; . /dev/null; echo $?'
> >>> 1
> >>> $ zsh -c 'false; ``; echo $?'
> >>> 1
> >> $ zsh -c 'false; sleep& echo $?'
> >> 1
> > 
> > "fn() { }" is valid, and I suspect "fn"
> > logically ought to reset the status, too
> 
> These four were all fairly easy; in each case there is a local context
> where the command in question is being executed in which the status can
> be reset.  There may be a a more generic place, such as execlist(), for
> these to go, but I like the transparency of resetting them locally;
> execlist()/execode() are used in just too many places.

Change of plan:  I woke up in the night and realised that doing it this
way breaks the case

  fn() { print $?; }
  false
  fn

which is a bit strange but perfectly valid and should print "1".

> However, that's a bit craven.

This takes the brave option, but actually it's easy except for the case
of "." which needs a bit more information from down below.  For both
executing a list and sourcing a file line by line we only reset the
status if there is no code to execute.

The case of starting a background function isn't affected, in that case
the foreground code is always empty.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.167
diff -u -r1.167 exec.c
--- Src/exec.c	10 Jul 2009 22:10:26 -0000	1.167
+++ Src/exec.c	11 Jul 2009 16:32:42 -0000
@@ -1056,6 +1056,10 @@
     /* Loop over all sets of comands separated by newline, *
      * semi-colon or ampersand (`sublists').               */
     code = *state->pc++;
+    if (wc_code(code) != WC_LIST) {
+	/* Empty list; this returns status zero. */
+	lastval = 0;
+    }
     while (wc_code(code) == WC_LIST && !breaks && !retflag && !errflag) {
 	int donedebug;
 
@@ -3513,7 +3517,6 @@
 	return retval;
     }
     /* pid == 0 */
-    lastval = 0;		/* status of empty list is zero */
     child_unblock();
     zclose(pipes[0]);
     redup(pipes[1], 1);
@@ -4261,7 +4264,6 @@
     if (trap_state == TRAP_STATE_PRIMED)
 	trap_return--;
     oldlastval = lastval;
-    lastval = 0;		/* status of empty function is zero */
     oldnumpipestats = numpipestats;
     if (noreturnval) {
 	/*
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.104
diff -u -r1.104 init.c
--- Src/init.c	10 Jul 2009 22:10:26 -0000	1.104
+++ Src/init.c	11 Jul 2009 16:32:42 -0000
@@ -99,11 +99,11 @@
 /* keep executing lists until EOF found */
 
 /**/
-int
+enum loop_return
 loop(int toplevel, int justonce)
 {
     Eprog prog;
-    int err;
+    int err, non_empty = 0;
 
     pushheap();
     if (!toplevel)
@@ -151,6 +151,7 @@
 	if (hend(prog)) {
 	    int toksav = tok;
 
+	    non_empty = 1;
 	    if (toplevel &&
 		(getshfunc("preexec") ||
 		 paramtab->getnode(paramtab, "preexec" HOOK_SUFFIX))) {
@@ -207,7 +208,11 @@
 	lexrestore();
     popheap();
 
-    return err;
+    if (err)
+	return LOOP_ERROR;
+    if (!non_empty)
+	return LOOP_EMPTY;
+    return LOOP_OK;
 }
 
 static char *cmd;
@@ -1131,7 +1136,6 @@
     fstack.tp = FS_SOURCE;
     funcstack = &fstack;
 
-    lastval = 0;		/* status of empty file is zero */
     if (prog) {
 	pushheap();
 	errflag = 0;
@@ -1141,8 +1145,21 @@
 	    ret = SOURCE_ERROR;
     } else {
 	/* loop through the file to be sourced  */
-	if (loop(0, 0))
+	switch (loop(0, 0))
+	{
+	case LOOP_OK:
+	    /* nothing to do but compilers like a complete enum */
+	    break;
+
+	case LOOP_EMPTY:
+	    /* Empty code resets status */
+	    lastval = 0;
+	    break;
+
+	case LOOP_ERROR:
 	    ret = SOURCE_ERROR;
+	    break;
+	}
     }
     funcstack = funcstack->prev;
     sourcelevel--;
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.159
diff -u -r1.159 zsh.h
--- Src/zsh.h	10 Jul 2009 11:08:48 -0000	1.159
+++ Src/zsh.h	11 Jul 2009 16:32:43 -0000
@@ -1726,6 +1726,17 @@
 #define PRINT_WHENCE_FUNCDEF	(1<<9)
 #define PRINT_WHENCE_WORD	(1<<10)
 
+/* Return values from loop() */
+
+enum loop_return {
+    /* Loop executed OK */
+    LOOP_OK,
+    /* Loop executed no code */
+    LOOP_EMPTY,
+    /* Loop encountered an error */
+    LOOP_ERROR
+};
+
 /* Return values from source() */
 
 enum source_return {
Index: Test/A01grammar.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A01grammar.ztst,v
retrieving revision 1.24
diff -u -r1.24 A01grammar.ztst
--- Test/A01grammar.ztst	10 Jul 2009 22:10:27 -0000	1.24
+++ Test/A01grammar.ztst	11 Jul 2009 16:32:43 -0000
@@ -553,3 +553,9 @@
 0:Last status of successfully executed "." file is retained
 >1
 >0
+
+  echo 'echo $?' >dot_status
+  false
+  . ./dot_status
+0:"." file sees status from previous command
+>1
Index: Test/C04funcdef.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/C04funcdef.ztst,v
retrieving revision 1.6
diff -u -r1.6 C04funcdef.ztst
--- Test/C04funcdef.ztst	10 Jul 2009 22:10:27 -0000	1.6
+++ Test/C04funcdef.ztst	11 Jul 2009 16:32:43 -0000
@@ -15,6 +15,13 @@
   fnz
 0:Empty function body resets status
 
+  fn3() { return 3; }
+  fnstat() { print $?; }
+  fn3
+  fnstat
+0:Status is not reset on non-empty function body
+>3
+
   function f$$ () {
     print regress expansion of function names
   }
Index: Test/D08cmdsubst.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/D08cmdsubst.ztst,v
retrieving revision 1.3
diff -u -r1.3 D08cmdsubst.ztst
--- Test/D08cmdsubst.ztst	10 Jul 2009 22:10:27 -0000	1.3
+++ Test/D08cmdsubst.ztst	11 Jul 2009 16:32:43 -0000
@@ -93,3 +93,8 @@
  false
  ``
 0:Empty command substitution resets status
+
+ false
+ echo `echo $?`
+0:Non-empty command substitution inherits status
+>1


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

Re: bug: $? after empty command

[Bart Schaefer]

On Jul 11,  5:39pm, Peter Stephenson wrote:
}
} Change of plan:  I woke up in the night and realised that doing it this
} way breaks [something]

I do that, too.  Results in some strange sleep hours, sometimes.