zsh syntax check fails on correct if [[ usage (rhbz 966911)

zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Filip Krska]

Hello zsh team,

I'd like to report a bug originally filed in 
https://bugzilla.redhat.com/show_bug.cgi?id=966911:

Description of problem:

zsh syntax check fails on simple script, which is syntactically OK, but zsh -n returns 1

Version-Release number of selected component (if applicable):

zsh-4.3.10-5.el6.x86_64

How reproducible:

always

Steps to Reproduce:
1. create a script

$ cat /tmp/test.zsh
#!/bin/zsh

if [[ $# -eq 1 ]]
then
   THE_USER=$1
else
   THE_USER=$(whoami)
fi

2. execute test
$ zsh -n /tmp/test.zsh

3. observe exit value
$ echo $?
1

   
Actual results:

1

Expected results:

0

Additional info:

Upstream zsh-5.0.0, zsh-5.0.2 behaves the same way.

The exitvalue is 0 if we provide exactly one argument:

$ zsh -n /tmp/test.zsh
$ echo $?
1
$ zsh -n /tmp/test.zsh aaa
$ echo $?
0
$ zsh -n /tmp/test.zsh aaa ddd
$ echo $?
1

The exitvalue is 0 if we add one line e.g.:

$ cat /tmp/test.zsh
#!/bin/zsh

if [[ $# -eq 1 ]]
then
   :
   THE_USER=$1
else
   THE_USER=$(whoami)
fi

or if we don't use $(command) construct in `else` branch.


If '[' test instead of '[[' is used, the syntax check returns OK, 
however '[[' syntax is recommended by manual, so the check should work 
as expected for '[[' as well.

Thanks and Regards

Filip

-- 
Filip Krška

GSS-SEG EMEA engineer, #sbr-shells (pri), #sbr-anaconda (sec)
Red Hat Czech s.r.o.
Purkyňova 99/71
612 45, Brno, Czech Republic
email:fkrska@redhat.com
office phone:+420 5322 94499

♡ All Wrongs Reversed
http://copyheart.org

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Peter Stephenson]

On Fri, 18 Oct 2013 16:02:42 +0200
Filip Krska <fkrska@redhat.com> wrote:
> $ cat /tmp/test.zsh
> #!/bin/zsh
> 
> if [[ $# -eq 1 ]]
> then
>    THE_USER=$1
> else
>    THE_USER=$(whoami)
> fi
> 
> 2. execute test
> $ zsh -n /tmp/test.zsh
> 
> 3. observe exit value
> $ echo $?
> 1

Yes, that's wrong.  It looks like when $(...) doesn't run anything
because of NO_EXEC it's keeping the status value from the parent shell,
which isn't useful.  (That's where the test above comes into the problem
--- it's setting the internal status to 1.)  I think we need to
initialise the status to 0 after the fork.  I haven't checked, but
presumably when NO_EXEC isn't set it gets far enough that's initialised
at some later point, but I don't think there's ever a case where the
forked shell should inherit the status -- in fact, maybe this should go
in entersubsh(), or is that too dangerous?

This may need to go after other forks, and then I should add a test.

diff --git a/Src/exec.c b/Src/exec.c
index 8efbbd4..5e65e1b 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3743,6 +3743,7 @@ getoutput(char *cmd, int qt)
     redup(pipes[1], 1);
     entersubsh(ESUB_PGRP|ESUB_NOMONITOR);
     cmdpush(CS_CMDSUBST);
+    lastval = 0; /* if nothing is executed, status is 0 */
     execode(prog, 0, 1, "cmdsubst");
     cmdpop();
     close(1);

pws

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Bart Schaefer]

On Oct 18,  4:02pm, Filip Krska wrote:
}
} zsh syntax check fails on simple script, which is syntactically OK, but zsh -n returns 1

Hmm, it's not actually the syntax check which is failing.

If you run the test as

    zsh -vxn /tmp/test.zsh

it becomes clear that zsh is actually executing some of the commands,
and then returning the exit status of those commands rather than of
the syntax check itself.

Which is strange, particularly given that adding the ":" command in
the if-block causes it to revert to executing nothing.  (Changing
the $(...) construct is a red herring, it's just changing the exit
status of the line zsh is wrongly executing in the first place.)

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Peter Stephenson]

On Fri, 18 Oct 2013 08:48:31 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> If you run the test as
> 
>     zsh -vxn /tmp/test.zsh
> 
> it becomes clear that zsh is actually executing some of the commands,
> and then returning the exit status of those commands rather than of
> the syntax check itself.

It's certainly forking, and I think it's peforming the assignments, and
it seems to perform the test.  The problem here is we don't actually
have a syntax checking mode, we just have a mode that tries to execute
as little as possible.  As little as possible isn't a very clear target.

pws

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Peter Stephenson]

On Fri, 18 Oct 2013 16:33:36 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> It looks like when $(...) doesn't run anything
> because of NO_EXEC it's keeping the status value from the parent shell,
> which isn't useful.  (That's where the test above comes into the problem
> --- it's setting the internal status to 1.)  I think we need to
> initialise the status to 0 after the fork. 

No, that's not right, and there's a check for it in D08cmdsubst.ztst.

 false
 echo `echo $?`
0:Non-empty command substitution inherits status
>1

We *do* need to inherit the status.  To go down this route we'd need
to detect that nothing was being executed and set the status explicitly
to zero, which we do in some cases, but not for NOEXEC.

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

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Bart Schaefer]

On Oct 18,  4:56pm, Peter Stephenson wrote:
} Subject: Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)
}
} On Fri, 18 Oct 2013 08:48:31 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > If you run the test as
} > 
} >     zsh -vxn /tmp/test.zsh
} > 
} > it becomes clear that zsh is actually executing some of the commands,
} > and then returning the exit status of those commands rather than of
} > the syntax check itself.
} 
} It's certainly forking, and I think it's peforming the assignments, and
} it seems to perform the test.  The problem here is we don't actually
} have a syntax checking mode, we just have a mode that tries to execute
} as little as possible.  As little as possible isn't a very clear target.

Well, yeah, but it's really strange that in some cases it executes NOTHING,
and in other very similar cases it executes the tests and assignments, and
it's really not obvious why there's a difference.

Compare these three commands:

Src/zsh -fvxnc 'if [[ $# -gt 0 ]]; then :; X=$1; fi'

Src/zsh -fvxnc 'if [[ $# -gt 0 ]]; then X=$1; fi'

Src/zsh -fvxnc 'if [ $# -gt 0 ]; then X=$1; fi'

Why does the inclusion of the ":" in the first case cause the execution of
the test to be skipped?  Why does the use of [[ ]] in place of [  ] cause
the test to be executed?

Additionally:
Why doesn't -v result in any output when used with -c ?  If you put any
of the above one-liners in a file and executed it as a script, it gets
printed as verbose output.

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Peter Stephenson]

On Fri, 18 Oct 2013 08:48:31 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 18,  4:02pm, Filip Krska wrote:
> }
> } zsh syntax check fails on simple script, which is syntactically OK, but zsh -n returns 1
> 
> Hmm, it's not actually the syntax check which is failing.
> 
> If you run the test as
> 
>     zsh -vxn /tmp/test.zsh
> 
> it becomes clear that zsh is actually executing some of the commands,
> and then returning the exit status of those commands rather than of
> the syntax check itself.
> 
> Which is strange, particularly given that adding the ":" command in
> the if-block causes it to revert to executing nothing.  (Changing
> the $(...) construct is a red herring, it's just changing the exit
> status of the line zsh is wrongly executing in the first place.)

Looked at from this point of view, the fix is probably not to execute
code optimised as "simple".

diff --git a/Src/exec.c b/Src/exec.c
index 8efbbd4..e95cad3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1087,6 +1087,9 @@ execsimple(Estate state)
     if (errflag)
 	return (lastval = 1);
 
+    if (!isset(EXECOPT))
+	return lastval = 0;
+
     /* In evaluated traps, don't modify the line number. */
     if (!IN_EVAL_TRAP() && !ineval && code)
 	lineno = code - 1;

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

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Peter Stephenson]

On Fri, 18 Oct 2013 12:20:00 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Well, yeah, but it's really strange that in some cases it executes NOTHING,
> and in other very similar cases it executes the tests and assignments, and
> it's really not obvious why there's a difference.

All sounds like the execsimple() optimisation mayhem --- it's all a
little bit funny what's considered "simple" and what isn't.

I've committed the fix for that --- see if any of the funnies you
spotted persist.

pws

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Bart Schaefer]

On Oct 18, 11:43pm, Peter Stephenson wrote:
}
} All sounds like the execsimple() optimisation mayhem --- it's all a
} little bit funny what's considered "simple" and what isn't.
} 
} I've committed the fix for that --- see if any of the funnies you
} spotted persist.

That seems to have done the trick for NO_EXEC.

The only oddity that persists is that "zsh -fvc 'something'" does not
display any verbose output for 'something'.

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Bart Schaefer]

On Oct 19,  2:19pm, Bart Schaefer wrote:
}
} The only oddity that persists is that "zsh -fvc 'something'" does not
} display any verbose output for 'something'.

Any objections to this?  Or should this go in init_misc() instead, so
as to not affect other internal calls to execstring()?

diff --git a/Src/exec.c b/Src/exec.c
index e95cad3..8751ac5 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1027,6 +1027,12 @@ execstring(char *s, int dont_change_job, int exiting, char *context)
     Eprog prog;
 
     pushheap();
+    if (isset(VERBOSE)) {
+      zputs(s, stderr);
+      if (s[strlen(s)-1] != '\n')
+	  zputs("\n", stderr);
+      fflush(stderr);
+    }
     if ((prog = parse_string(s, 0)))
 	execode(prog, dont_change_job, exiting, context);
     popheap();

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Peter Stephenson]

On Sat, 19 Oct 2013 14:19:58 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The only oddity that persists is that "zsh -fvc 'something'" does not
> display any verbose output for 'something'.

This affects verbosity of execution of strings for fc editing, sched,
STTY and inside zregexparse, too, but it's not clear that's wrong, and
they're all non-standard cases where we get to choose.

diff --git a/Src/exec.c b/Src/exec.c
index e95cad3..d5fe69e 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1027,6 +1027,11 @@ execstring(char *s, int dont_change_job, int exiting, char *context)
     Eprog prog;
 
     pushheap();
+    if (isset(VERBOSE)) {
+	zputs(s, stderr);
+	fputc('\n', stderr);
+	fflush(stderr);
+    }
     if ((prog = parse_string(s, 0)))
 	execode(prog, dont_change_job, exiting, context);
     popheap();

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

Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

[Filip Krska]

Many thanks, Peter,

we've tested both commits

http://sourceforge.net/p/zsh/code/ci/9a044f1a6ad4ecfdfeff2f89e1685a1d622cb029/
http://sourceforge.net/p/zsh/code/ci/8879c46a4897a0e347455334fc6b6732c203a220/

and the issue seems solved after the patches were applied.

Best Regards

Filip

----- Original Message -----
From: "Peter Stephenson" <p.w.stephenson@ntlworld.com>
To: zsh-workers@zsh.org
Sent: Saturday, October 19, 2013 11:56:02 PM
Subject: Re: zsh syntax check fails on correct if [[ usage (rhbz 966911)

On Sat, 19 Oct 2013 14:19:58 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The only oddity that persists is that "zsh -fvc 'something'" does not
> display any verbose output for 'something'.

This affects verbosity of execution of strings for fc editing, sched,
STTY and inside zregexparse, too, but it's not clear that's wrong, and
they're all non-standard cases where we get to choose.

diff --git a/Src/exec.c b/Src/exec.c
index e95cad3..d5fe69e 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1027,6 +1027,11 @@ execstring(char *s, int dont_change_job, int exiting, char *context)
     Eprog prog;
 
     pushheap();
+    if (isset(VERBOSE)) {
+	zputs(s, stderr);
+	fputc('\n', stderr);
+	fflush(stderr);
+    }
     if ((prog = parse_string(s, 0)))
 	execode(prog, dont_change_job, exiting, context);
     popheap();

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