Parse error (lack thereof) on incomplete loops

Parse error (lack thereof) on incomplete loops

[Bart Schaefer]

This went by in a zsh-users thread so it may have been missed:

% { while false
cursh while> }
%

This indicates a parsing problem in 5.6, because in 5.0:

% { while false
cursh while> }
zsh: parse error near `}'

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Thu, 2018-10-04 at 08:49 -0700, Bart Schaefer wrote:
> This went by in a zsh-users thread so it may have been missed:

Not sure who the additional audience is...

> % { while false
> cursh while> }
> %
> 
> This indicates a parsing problem in 5.6, because in 5.0:
> 
> % { while false
> cursh while> }
> zsh: parse error near `}'

It affects for, repeat, until and if, too, and isn't hard to turn back
into a parse error.  Patch attached, though I'm not going to apply it
at
the moment.

However, I'm very far from convinced this is actually a bug.  If you
turn off SHORT_LOOPS you'll find you get the error back; the
alternative loop syntax is defined in such a woolly fashion perhaps you
actually can get away with this.  The short-loop user is probably
primed
to notice such things as that there's no following command so it'll
just
do nothing.

I'm therefore inclined to leave it alone since the SHORT_LOOPS user
(Marc) is perfectly happy with it and a lot of us avoid the alternative
syntax like the plague.

diff --git a/Src/parse.c b/Src/parse.c
index 83383f1..a96cdcb 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -446,12 +446,17 @@ ecstrcode(char *s)
         par_list(C); \
         if (eu == ecused) ecadd(WCB_END()); \
     } while (0)
-#define par_save_list1(C) \
-    do { \
-        int eu = ecused; \
-        par_list1(C); \
-        if (eu == ecused) ecadd(WCB_END()); \
-    } while (0)
+
+/**/
+static int
+par_save_list1(int *cmplx)
+{
+    int eu = ecused;
+    int ret = par_list1(cmplx);
+    if (eu == ecused)
+	ecadd(WCB_END());
+    return ret;
+}
 
 
 /**/
@@ -766,7 +771,7 @@ par_list(int *cmplx)
 }
 
 /**/
-static void
+static int
 par_list1(int *cmplx)
 {
     int p = ecadd(0), c = 0;
@@ -774,8 +779,11 @@ par_list1(int *cmplx)
     if (par_sublist(&c)) {
 	set_list_code(p, (Z_SYNC | Z_END), c);
 	*cmplx |= c;
-    } else
+	return 1;
+    } else {
 	ecused--;
+	return 0;
+    }
 }
 
 /*
@@ -1151,8 +1159,8 @@ par_for(int *cmplx)
 	zshlex();
     } else if (unset(SHORTLOOPS)) {
 	YYERRORV(oecused);
-    } else
-	par_save_list1(cmplx);
+    } else if (!par_save_list1(cmplx))
+	YYERRORV(oecused);
 
     ecbuf[p] = (sel ?
 		WCB_SELECT(type, ecused - 1 - p) :
@@ -1439,7 +1447,8 @@ par_if(int *cmplx)
 	} else {
 	    cmdpop();
 	    cmdpush(nc);
-	    par_save_list1(cmplx);
+	    if (!par_save_list1(cmplx))
+		YYERRORV(oecused);
 	    ecbuf[pp] = WCB_IF(type, ecused - 1 - pp);
 	    incmdpos = 1;
 	    break;
@@ -1512,8 +1521,8 @@ par_while(int *cmplx)
 	zshlex();
     } else if (unset(SHORTLOOPS)) {
 	YYERRORV(oecused);
-    } else
-	par_save_list1(cmplx);
+    } else if (!par_save_list1(cmplx))
+	YYERRORV(oecused);
 
     ecbuf[p] = WCB_WHILE(type, ecused - 1 - p);
 }
@@ -1561,8 +1570,8 @@ par_repeat(int *cmplx)
 	zshlex();
     } else if (unset(SHORTLOOPS)) {
 	YYERRORV(oecused);
-    } else
-	par_save_list1(cmplx);
+    } else if (!par_save_list1(cmplx))
+	YYERRORV(oecused);
 
     ecbuf[p] = WCB_REPEAT(ecused - 1 - p);
 }

Re: Parse error (lack thereof) on incomplete loops

[Marc Chantreux]

hello Peter,

Again: thanks for your amazing job Bart and you did on zsh.

> However, I'm very far from convinced this is actually a bug.  If you
> turn off SHORT_LOOPS you'll find you get the error back; the

is there a way to turn it off from configuration? it looks like an
option in the manual but

    setopt noshortloop

gives me

    setopt: no such option: noshortloop

> I'm therefore inclined to leave it alone since the SHORT_LOOPS user
> (Marc) is perfectly happy with it and a lot of us avoid the alternative
> syntax like the plague.

is it just because people don't like it or are there technical reasons
to avoid it?

regards

Re: Parse error (lack thereof) on incomplete loops

[Bart Schaefer]

On Thu, Oct 4, 2018 at 1:04 PM Marc Chantreux <eiro@phear.org> wrote:
>
> > turn off SHORT_LOOPS you'll find you get the error back; the
>
> is there a way to turn it off from configuration? it looks like an
> option in the manual but
>
>     setopt noshortloop

Plural ...

Re: Parse error (lack thereof) on incomplete loops

[Marc Chantreux]

> > is there a way to turn it off from configuration? it looks like an
> > option in the manual but
> >
> >     setopt noshortloop
> 
> Plural ...

correct.. thanks 

marc

Re: Parse error (lack thereof) on incomplete loops

[Bart Schaefer]

On Thu, Oct 4, 2018 at 9:32 AM Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> On Thu, 2018-10-04 at 08:49 -0700, Bart Schaefer wrote:
>
> > % { while false
> > cursh while> }
> > %
> >
> > This indicates a parsing problem in 5.6
>
> It affects for, repeat, until and if, too, and isn't hard to turn back
> into a parse error.

How did it stop being a parse error in the first place?

> However, I'm very far from convinced this is actually a bug.  If you
> turn off SHORT_LOOPS you'll find you get the error back

My problem with it is that it's not consistent.

set -x
{ while false
while false
}

The shell is now in an infinite loop executing "false".  Can this
really be intentional?

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Thu, 2018-10-04 at 13:34 -0700, Bart Schaefer wrote:
> My problem with it is that it's not consistent.
> 
> set -x
> { while false
> while false
> }
> 
> The shell is now in an infinite loop executing "false".  Can this
> really be intentional?

What's happening here is to do with

while false; do
  print Never executed
done
print $?

This prints 0.  So the body of the last "while false" is actually
irrelevant.

The original expression is parsed as

{
  while {
    false
    while false; do
    done
    }; do
  done
}

which is an infinite loop because of the point above.  Or, in other
words, it's the same as

{
  while false
    true
}

This also used to give a parse error for the same reason.  However,
apart from that there's nothing new --- you can see that even with
NO_SHORT_LOOPS,

while false; true; do print Looping; done

(where that "true" could be a "while false" loop, if you like) has been
an infinite loop since way back when.  The only new feature is that all
the "do ... done" clause has become optional if it's possible to infer
there's nothing there.  That seems to me entirely consistent.

Without a "do" while doesn't know where the expression ends.  That's
fundamental to how SHORT_LOOPS works and why I regard it as so
ill-defined as to be useless in all but the simplest cases.  This new
(accidental) feature is giving it a particularly straightforward way of
telling it where the expression ends.

Anyway, I'm perfectly happy either restoring the parse error or not,
depending on the opinions of people more likely to use or fall foul of
this kind of syntax but I don't think the reason "it's all a bit weird"
is good enough on its own for restoring it.  Short loops *are* weird.

pws

Re: Parse error (lack thereof) on incomplete loops

[Daniel Shahaf]

Peter Stephenson wrote on Fri, 05 Oct 2018 10:14 +0100:
> {
>   while false
>     true
> }
...
> Without a "do" while doesn't know where the expression ends.  That's
> fundamental to how SHORT_LOOPS works and why I regard it as so
> ill-defined as to be useless in all but the simplest cases.  This new
> (accidental) feature is giving it a particularly straightforward way of
> telling it where the expression ends.
> 
> Anyway, I'm perfectly happy either restoring the parse error or not,
> depending on the opinions of people more likely to use or fall foul of
> this kind of syntax but I don't think the reason "it's all a bit weird"
> is good enough on its own for restoring it.  Short loops *are* weird.

Point taken :-)

Can we come up with a one-sided parsing rule for syntactically valid cases?
That is, a rule that says guarantees that some constructs are syntactically
valid, but doesn't necessarily say anything about other constructs.

(I'm thinking of something like the last paragraph of users/23696, though
I have no opinion on what the rule should be.  My only intersection with
shortloops is that I use the short form of 'for' in interactive shells.)

Cheers,

Daniel

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Fri, 2018-10-05 at 13:26 +0000, Daniel Shahaf wrote:
> Can we come up with a one-sided parsing rule for syntactically valid cases?
> That is, a rule that says guarantees that some constructs are syntactically
> valid, but doesn't necessarily say anything about other constructs.

That's pretty much what the the parser in practice does.  If you look at
the code, it's got tests that read...

- If SHORTLOOPS isn't set panic at this point.

- Otherwise go on and see if we can get anything sensible out of what
we did find.

So for example the while loop code has...

    } else if (unset(SHORTLOOPS)) {
	YYERRORV(oecused);
    } else
	par_save_list1(cmplx);

The case that brought this up is that we don't check the return value
from par_save_list1() (actually a macro at the moment).

For more explicit rules it's a question of decoding the tests above (if
we got a "do" then blah, else if we got a "{" then etc. etc.).

pws

Re: Parse error (lack thereof) on incomplete loops

[Daniel Shahaf]

Peter Stephenson wrote on Fri, 05 Oct 2018 13:47 +0000:
> On Fri, 2018-10-05 at 13:26 +0000, Daniel Shahaf wrote:
> > Can we come up with a one-sided parsing rule for syntactically valid cases?
> > That is, a rule that says guarantees that some constructs are syntactically
> > valid, but doesn't necessarily say anything about other constructs.
> 
> That's pretty much what the the parser in practice does.  If you look at
> the code, it's got tests that read...
> 
> - If SHORTLOOPS isn't set panic at this point.
> 
> - Otherwise go on and see if we can get anything sensible out of what
> we did find.
> 
> So for example the while loop code has...
> 
>     } else if (unset(SHORTLOOPS)) {
> 	YYERRORV(oecused);
>     } else
> 	par_save_list1(cmplx);
> 
> The case that brought this up is that we don't check the return value
> from par_save_list1() (actually a macro at the moment).
> 
> For more explicit rules it's a question of decoding the tests above (if
> we got a "do" then blah, else if we got a "{" then etc. etc.).

Thanks, but that's not quite what I meant.

I meant, we should have a documented rule for which constructs are
syntactically valid when SHORT_LOOPS is set.  Then whenever a question
such as "should <this case> raise a parse error?" comes up, we'd answer
that according to the rule we'd decided upon in advance.  This way the
syntax will be predictable.

The rule should not be defined in terms of the implementation, since it needs
to be able to be used to decide whether the change from 5.5 to 5.6 was a bugfix
or a regression.

Cheers,

Daniel

Re: Parse error (lack thereof) on incomplete loops

[Bart Schaefer]

On Fri, Oct 5, 2018 at 2:14 AM Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> The original expression is parsed as
>
> {
>   while {
>     false
>     while false; do
>     done
>     }; do
>   done
> }
>
> which is an infinite loop because of the point above.  [...]  However,
> apart from that there's nothing new [...]  The only new feature is that all
> the "do ... done" clause has become optional if it's possible to infer
> there's nothing there.  That seems to me entirely consistent.

I would contend that the point of SHORT_LOOPS is not to be able to
omit the loop body, it's to be able to abbreviate the syntax that
delimits the loop conditions and separates them from the loop body.
But of what possible use, for example, is { repeat 5 } which this
"accidental feature" allows?  Or { select foo } or { for x } ?  And if
{ for x } is allowed, why is { for x in z } an error, particularly if
you're arguing that it's obvious the "do-done" is meant to be empty?
Why should it have to be { for x in z; } when a trailing semicolon is
not required in other brace expressions?

> Without a "do" while doesn't know where the expression ends.  That's
> fundamental to how SHORT_LOOPS works

No it isn't.  The documentation explicitly says:

  For the if, while and until commands, in both these cases the test part
  of the loop must also be suitably delimited, such as by `[[ ... ]]' or
  `(( ... )), else the end of the test will not be recognized.

"Both these cases" refers to:
#1 - sublist is of the form `{ list }'
#2 - the SHORT_LOOPS option is set

So you don't get to claim that SHORT_LOOPS is intended to except you
from that restriction.  The "]]" or "))" tell while where the
expression ends.

In none of the examples given in this thread so far has the test
expression been delimited that way.

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Fri, 5 Oct 2018 18:47:01 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> > Without a "do" while doesn't know where the expression ends.  That's
> > fundamental to how SHORT_LOOPS works  
> 
> No it isn't.  The documentation explicitly says:
> 
>   For the if, while and until commands, in both these cases the test part
>   of the loop must also be suitably delimited, such as by `[[ ... ]]' or
>   `(( ... )), else the end of the test will not be recognized.
> 
> "Both these cases" refers to:
> #1 - sublist is of the form `{ list }'
> #2 - the SHORT_LOOPS option is set
> 
> So you don't get to claim that SHORT_LOOPS is intended to except you
> from that restriction.  The "]]" or "))" tell while where the
> expression ends.
> 
> In none of the examples given in this thread so far has the test
> expression been delimited that way.

But that's not what's implemented --- it just calls the code to parse
what the shell refers to as a "sublist", which is a component of the
normal parse tree.  There's never been any special short-loops code,
despite what the doc claims.  Hence there's no question, in general,
of throwing a syntax error if it isn't in the above form (unless
SHORT_LOOPS is unset).

I think the above is close to what Daniel is looking for --- a
statement of what does work, with anything else being left to
the user's peril.

It seems quite clear I should leave well alone.  If anyone thinks they
can write code that tightens it up in the above fashion, feel free.

pws

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Sat, 6 Oct 2018 12:15:19 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> But that's not what's implemented --- it just calls the code to parse
> what the shell refers to as a "sublist", which is a component of the
> normal parse tree.  There's never been any special short-loops code,
> despite what the doc claims.  Hence there's no question, in general,
> of throwing a syntax error if it isn't in the above form (unless
> SHORT_LOOPS is unset).

Hence, in fact, the following is the best we're going to be able to do
without a rewrite.

In summary, the current implementation isn't sufficiently restricted
that attempting to keep it in check in terms of empty command lists
makes sense --- but for the same reason you shouldn't be relying on that
behaviour.

pws

diff --git a/Doc/Zsh/grammar.yo b/Doc/Zsh/grammar.yo
index d2c7cd29c..11326586e 100644
--- a/Doc/Zsh/grammar.yo
+++ b/Doc/Zsh/grammar.yo
@@ -409,6 +409,12 @@ tt(for), tt(repeat), tt(case) and tt(select) commands no such special form
 for the arguments is necessary, but the other condition (the special form
 of var(sublist) or use of the tt(SHORT_LOOPS) option) still applies.
 
+Note that use of the tt(SHORT_LOOPS) option is currently implemented
+loosely in the shell, so that no check of the form of the condition is made
+before attempting to parse the subsequent command list.  This means that
+other forms not documented below may be allowed; these should not be relied
+on.
+
 startitem()
 item(tt(if) var(list) tt({) var(list) tt(}) [ tt(elif) var(list) tt({) var(list) tt(}) ] ... [ tt(else {) var(list) tt(}) ])(
 An alternate form of tt(if).  The rules mean that

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Fri, 5 Oct 2018 18:47:01 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
>   For the if, while and until commands, in both these cases the test part
>   of the loop must also be suitably delimited, such as by `[[ ... ]]' or
>   `(( ... )), else the end of the test will not be recognized.

Looked at this a bit more and I have fairly major doubts about the
documentation (although I don't mean about the intention suggested by
Bart which does seem reaonsble).

  The short versions below only work if var(sublist) is of the form `tt({)
  var(list) tt(})' or if the tt(SHORT_LOOPS) option is set.

For while and until, we only have
                     
 while list { list }
    An  alternative  form of while.  Note the limitations on the form of
    list mentioned above.
       
 until list { list }
    An alternative form of until.  Note the limitations on the  form  of
    list mentioned above.

No sublist.  The shortloops version of this does work:

while [[ -o shortloops ]] print yes

so maybe that "{ list }" should really be "sublist"?

It's also not clearly (or at all?  I can't se it) documented that you
only get the SHORT_LOOPS effect if there's no delimiter --- if you stick
one in it looks like a normal command list which it'll just go on parsing.

So actually I think the documentation here is defective even for the
cases of SHORT_LOOPS that *are* supposd to work.
 
Perhapse someone who may have to be Bart (I can't say I consider myself
an expert in this having only just found out what's going on and I can't
say anyone else is likely to, either) should suggest some words of
wisdom to document SHORT_LOOPS as separate text rather than as part of
the variant syntax section?  I don't mind writing this up.

pws

Re: Parse error (lack thereof) on incomplete loops

[Bart Schaefer]

So ... I've been looking through the git history of Doc/Zsh/grammar.yo ...

On Sat, Oct 6, 2018 at 11:21 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> On Fri, 5 Oct 2018 18:47:01 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> >   For the if, while and until commands, in both these cases the test part
> >   of the loop must also be suitably delimited, such as by `[[ ... ]]' or
> >   `(( ... )), else the end of the test will not be recognized.
>
> Looked at this a bit more and I have fairly major doubts about the
> documentation (although I don't mean about the intention suggested by
> Bart which does seem reaonsble).

   2001-07-05  Peter Stephenson  <pws@csr.com>

  +       * 15264: Doc/Zsh/grammar.yo: improve description of use of
  +       variant complex command forms to avoid confusing the present
  +       writer.
  +

So apparently you've had doubts about this for a long time.

>   The short versions below only work if var(sublist) is of the form `tt({)
>   var(list) tt(})' or if the tt(SHORT_LOOPS) option is set.

This appears to be a reference to
  item(tt(if) var(list) var(sublist))
and similar doc of "for", "repeat", and "select".

These also have the "{ list }" forms documented separately so I don't
know why the "sublist" form of while and until was never made explicit
(see guess at it below).

> so maybe that "{ list }" should really be "sublist"?

Or the form should just be added for congruence with the doc of the
other complex commands.  If they'd all been there separately in the
first place, the sentence about "var(sublist) is of the form ..."
might never have been needed.  It's also quite likely (I haven't dug
into this in the Src commits) that "while" and "until" originally did
NOT work with only a sublist and that they do is another accidental
feature from some even earlier parser change.

> It's also not clearly (or at all?  I can't see it) documented that you
> only get the SHORT_LOOPS effect if there's no delimiter --- if you stick
> one in it looks like a normal command list which it'll just go on parsing.

True that this is not called out, but implicitly from the grammar:
var(list) var(sep) var(sublist) matches a production yielding
var(list), so if there is a var(sep) then the var(list) isn't done
yet.  You have to have a grammatically concluded var(list) as the
conditional before you can tack on a var(sublist) as the body.

> Perhapse someone who may have to be Bart (I can't say I consider myself
> an expert in this having only just found out what's going on and I can't
> say anyone else is likely to, either) should suggest some words of
> wisdom to document SHORT_LOOPS as separate text rather than as part of
> the variant syntax section?  I don't mind writing this up.

I may have time to take a stab at this tomorrow, can't promise.  Maybe
the above is a start.  I also don't presently have a computer with
"yum" installed/working.

Re: Parse error (lack thereof) on incomplete loops

[Bart Schaefer]

On Sat, Oct 6, 2018 at 8:51 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> I also don't presently have a computer with
> "yum" installed/working.

That made very little sense.  I was rushing out to an engagement.  I
meant "yodl".

Re: Parse error (lack thereof) on incomplete loops

[Peter Stephenson]

On Sat, 2018-10-06 at 20:51 -0700, Bart Schaefer wrote:
> So ... I've been looking through the git history of Doc/Zsh/grammar.yo ...
> > It's also not clearly (or at all?  I can't see it) documented that you
> > only get the SHORT_LOOPS effect if there's no delimiter --- if you stick
> > one in it looks like a normal command list which it'll just go on parsing.
>
> True that this is not called out, but implicitly from the grammar:
> var(list) var(sep) var(sublist) matches a production yielding
> var(list), so if there is a var(sep) then the var(list) isn't done
> yet.  You have to have a grammatically concluded var(list) as the
> conditional before you can tack on a var(sublist) as the body.

Probably superfluous to say this now, since we can certainly improve
the documentation without dwelling on the details, but the definition of
a list is given as

  A em(list) is a sequence of zero or more sublists, in which each sublist
  is terminated by `tt(;)', `tt(&)', `tt(&|)', `tt(&!)', or a newline.

So while I agree about the logical implications, if you don't have a
degree in comupter science it might be thoroughly unclear what's
actually going on here.

I'm vaguely moving to the opinion that we should list the SHORT_LOOPS
forms separately with examples rather than by implication, which I
suspect is at the root of why I've always found this bit of the manual
unhelpful.

pws