Re: broken parsing with $((`:`))

Re: broken parsing with $((`:`))

[Mikael Magnusson]

On Wed, Apr 15, 2015 at 11:03 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Tue, 14 Apr 2015 23:05:31 -0400
> Mike Frysinger <vapier@gentoo.org> wrote:
>> looks like zsh doesn't correctly parse this:
>> $ zsh -c 'echo $((`:`))'
>> zsh:1: bad math expression: illegal character: \M-]
>>
>> looks like it's related to the subshell not outputting anything.
>
> Yes, indeed --- there's a funny internal special case for empty strings
> that I never quite get my head around.
>
> (Please, God, make the problems with command and math substitution
> parsing stop now.)

% $(
cmdsubst> [press ctrl-c here]
zsh: parse error near `$('

I would expect to not get an error message about syntax after pressing
ctrl-c. It doesn't happen with $((, { or any other unbalanced thingers
I tried.

-- 
Mikael Magnusson

PATCH: unwanted error aborting continued command substitution

[Peter Stephenson]

On Fri, 17 Apr 2015 06:39:29 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> % $(
> cmdsubst> [press ctrl-c here]
> zsh: parse error near `$('
> 
> I would expect to not get an error message about syntax after pressing
> ctrl-c.

I think that means we can handle this case as part of a general tidy up
rather than agonising over the complexities of the nested parsing ---
which means we can reap the benefit [sorry, leverage the synergy] of
breaking out the interrupt flag.  The following looks plausible, though
the interactions are complicated.

I believe the fix for only resetting the interrupt flag when resetting
the local keymap if there actually *was* a local keymap has to be
correct, but you never quite know if there are scary special cases that
use things differently...

pws

diff --git a/Src/Zle/zle_keymap.c b/Src/Zle/zle_keymap.c
index cfef882..c6fae25 100644
--- a/Src/Zle/zle_keymap.c
+++ b/Src/Zle/zle_keymap.c
@@ -503,8 +503,9 @@ selectkeymap(char *name, int fb)
 mod_export void
 selectlocalmap(Keymap m)
 {
+    Keymap oldm = localkeymap;
     localkeymap = m;
-    if (!m)
+    if (oldm && !m)
     {
 	/*
 	 * No local keymap; so we are returning to the global map.  If
diff --git a/Src/lex.c b/Src/lex.c
index 184a54b..c929bb9 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -1558,10 +1558,12 @@ parsestr(char **s)
 
     if ((err = parsestrnoerr(s))) {
 	untokenize(*s);
-	if (err > 32 && err < 127)
-	    zerr("parse error near `%c'", err);
-	else
-	    zerr("parse error");
+	if (!(errflag & ERRFLAG_INT)) {
+	    if (err > 32 && err < 127)
+		zerr("parse error near `%c'", err);
+	    else
+		zerr("parse error");
+	}
     }
     return err;
 }
diff --git a/Src/parse.c b/Src/parse.c
index 0b54a90..91a81e1 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -2419,7 +2419,7 @@ yyerror(int noerr)
     for (t0 = 0; t0 != 20; t0++)
 	if (!t || !t[t0] || t[t0] == '\n')
 	    break;
-    if (!(histdone & HISTFLAG_NOEXEC)) {
+    if (!(histdone & HISTFLAG_NOEXEC) && !(errflag & ERRFLAG_INT)) {
 	if (t0 == 20)
 	    zwarn("parse error near `%l...'", t, 20);
 	else if (t0)

Re: PATCH: unwanted error aborting continued command substitution

[Mikael Magnusson]

On Fri, Apr 17, 2015 at 11:16 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Fri, 17 Apr 2015 06:39:29 +0200
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> % $(
>> cmdsubst> [press ctrl-c here]
>> zsh: parse error near `$('
>>
>> I would expect to not get an error message about syntax after pressing
>> ctrl-c.
>
> I think that means we can handle this case as part of a general tidy up
> rather than agonising over the complexities of the nested parsing ---
> which means we can reap the benefit [sorry, leverage the synergy] of
> breaking out the interrupt flag.  The following looks plausible, though
> the interactions are complicated.
>
> I believe the fix for only resetting the interrupt flag when resetting
> the local keymap if there actually *was* a local keymap has to be
> correct, but you never quite know if there are scary special cases that
> use things differently...

I didn't think to try this yesterday, but invoking push-input gives
the same error (sorry).

-- 
Mikael Magnusson

Re: PATCH: unwanted error aborting continued command substitution

[Peter Stephenson]

On Fri, 17 Apr 2015 12:52:56 +0200
Mikael Magnusson <mikachu@gmail.com> wrote:
> On Fri, Apr 17, 2015 at 11:16 AM, Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> > On Fri, 17 Apr 2015 06:39:29 +0200
> > Mikael Magnusson <mikachu@gmail.com> wrote:
> >> % $(
> >> cmdsubst> [press ctrl-c here]
> >> zsh: parse error near `$('
>
> I didn't think to try this yesterday, but invoking push-input gives
> the same error (sorry).

That's a completely different kettle of fish --- push-input is basically
a hack to try to convince the line editor we've left it so we can start
again.  But we can't start again without leaving the nested parsing of
$(.  I have no idea what to do here except document it as "don't do
that".  Maybe Bart understands the sequence for this better.

pws

Re: PATCH: unwanted error aborting continued command substitution

[Bart Schaefer]

On Apr 17, 12:02pm, Peter Stephenson wrote:
} Subject: Re: PATCH: unwanted error aborting continued command substitution
}
} On Fri, 17 Apr 2015 12:52:56 +0200
} Mikael Magnusson <mikachu@gmail.com> wrote:
} > On Fri, Apr 17, 2015 at 11:16 AM, Peter Stephenson
} > <p.stephenson@samsung.com> wrote:
} > > On Fri, 17 Apr 2015 06:39:29 +0200
} > > Mikael Magnusson <mikachu@gmail.com> wrote:
} > >> % $(
} > >> cmdsubst> [press ctrl-c here]
} > >> zsh: parse error near `$('
} >
} > I didn't think to try this yesterday, but invoking push-input gives
} > the same error (sorry).

So does send-break (using e.g. ESC-x send-break RET):

torch% $(
cmdsubst> 
execute: send-break_
zsh: parse error near `$('
torch% 

} That's a completely different kettle of fish --- push-input is basically
} a hack to try to convince the line editor we've left it so we can start
} again.

Yes it's a hack, but it's the same kettle -- push-input uses send-break,
and send-break is supposed to be a simulated interrupt for cases where
the TTY intr character is not set.

So either send-break should actually set ERRFLAG_INT, or we need a third
error flag for simulated interrupts.

Re: PATCH: unwanted error aborting continued command substitution

[Peter Stephenson]

On Fri, 17 Apr 2015 08:46:45 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> push-input uses send-break,
> and send-break is supposed to be a simulated interrupt for cases where
> the TTY intr character is not set.
> 
> So either send-break should actually set ERRFLAG_INT, or we need a third
> error flag for simulated interrupts.

Actually, it doesn't bother calling send-break, but I've found the point
where it doesn't.

Maybe the following is OK --- in fact, possibly we don't need the
ERRFLAG_ERROR bit set, but I don't think it's doing any harm.

One advantage of using a different bit would be we could abort back to
the top of ZLE rather than out of ZLE and back in again, which would
mean you finally had the ability to embed push-line in other widgets.
But that's for another day.

pws

diff --git a/Src/Zle/zle_hist.c b/Src/Zle/zle_hist.c
index 88623bb..cc66f99 100644
--- a/Src/Zle/zle_hist.c
+++ b/Src/Zle/zle_hist.c
@@ -854,7 +854,7 @@ pushlineoredit(char **args)
     }
     ret = pushline(args);
     if (!isfirstln) {
-	errflag |= ERRFLAG_ERROR;
+	errflag |= ERRFLAG_ERROR|ERRFLAG_INT;
 	done = 1;
     }
     clearlist = 1;
diff --git a/Src/Zle/zle_misc.c b/Src/Zle/zle_misc.c
index 23286fc..4669ef2 100644
--- a/Src/Zle/zle_misc.c
+++ b/Src/Zle/zle_misc.c
@@ -1041,7 +1041,7 @@ copyprevshellword(UNUSED(char **args))
 int
 sendbreak(UNUSED(char **args))
 {
-    errflag |= ERRFLAG_ERROR;
+    errflag |= ERRFLAG_ERROR|ERRFLAG_INT;
     return 1;
 }
 

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Wed, 15 Apr 2015 09:13:35 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Hmm.
> 
> Src/pattern.c:571:           * Empty metafied strings have an initial Nularg.
> 
> Maybe we NEED to have the math.c case follow this form.  *s == Nularg
> is not the same as !*s if there can be something non-null as s[1].

I think "empty" means there *is* nothing after the Nularg.

But I may update math.c just to be like the other cases anyway; I won't
bother posting.

pws

Re: broken parsing with $((`:`))

[Bart Schaefer]

On Apr 15,  4:37pm, Peter Stephenson wrote:
} Subject: Re: broken parsing with $((`:`))
}
} > } +    if (!*s || *s == Nularg) {
} > 
} 
} - We look for Nularg in a string and skip over the character because
}   ditto (we could have made the math.c cases follow this form);

Hmm.

Src/pattern.c:571:           * Empty metafied strings have an initial Nularg.

Maybe we NEED to have the math.c case follow this form.  *s == Nularg
is not the same as !*s if there can be something non-null as s[1].

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Wed, 15 Apr 2015 08:13:48 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Apr 15, 10:03am, Peter Stephenson wrote:
> } 
> } Yes, indeed --- there's a funny internal special case for empty strings
> } that I never quite get my head around.
> } 
> } -    if (!*s) {
> } +    if (!*s || *s == Nularg) {
> 
> I wonder if (a) there are other overlooked cases like this and (b) if it
> would be useful to have a #define macro for (c == '\0' || c == Nularg).
> A quick grep doesn't find existing cases of that test other than this
> new one, but of course if the Nularg part were forgotten, it wouldn't.

Just looked and the typical tests for Nularg are:

- We get it as a single character so just ignore it because there's a '\0'
  next which the following code will handle normally;

- We look for Nularg in a string and skip over the character because
  ditto (we could have made the math.c cases follow this form);

- In a few cases we explicitly check to see if there was a '\0'
  after the Nularg --- possibly me being cautious because I don't really
  know what's going on; I think that's actually unnecessary because
  Nularg means the whole string is empty.

(- Not directly relevant but in prompt.c we look to see if there's a
Nularg we can backup over, which is probably really cool.)

pws

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Wed, 15 Apr 2015 08:13:48 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> } (Please, God, make the problems with command and math substitution
> } parsing stop now.)
> 
> Indeed ... at what point should we consider 5.0.8 to get all these little
> edge-case fixes out into the field?

Soon, I think; the problems are only trickling in somewhat glacially now
so there doesn't seem a lot of point in waiting... does anybody know
anything immediately blocking...?

pws

Re: broken parsing with $((`:`))

[Bart Schaefer]

On Apr 15, 10:03am, Peter Stephenson wrote:
} 
} Yes, indeed --- there's a funny internal special case for empty strings
} that I never quite get my head around.
} 
} -    if (!*s) {
} +    if (!*s || *s == Nularg) {

I wonder if (a) there are other overlooked cases like this and (b) if it
would be useful to have a #define macro for (c == '\0' || c == Nularg).
A quick grep doesn't find existing cases of that test other than this
new one, but of course if the Nularg part were forgotten, it wouldn't.

I suspect it just isn't that common because Nularg goes away after the
parsing pass.

} (Please, God, make the problems with command and math substitution
} parsing stop now.)

Indeed ... at what point should we consider 5.0.8 to get all these little
edge-case fixes out into the field?

Re: broken parsing with $((`:`))

[Peter Stephenson]

On Tue, 14 Apr 2015 23:05:31 -0400
Mike Frysinger <vapier@gentoo.org> wrote:
> looks like zsh doesn't correctly parse this:
> $ zsh -c 'echo $((`:`))'
> zsh:1: bad math expression: illegal character: \M-]
>
> looks like it's related to the subshell not outputting anything.

Yes, indeed --- there's a funny internal special case for empty strings
that I never quite get my head around.

(Please, God, make the problems with command and math substitution
parsing stop now.)

pws

diff --git a/Src/math.c b/Src/math.c
index c047725..f2c72d5 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -1398,7 +1398,7 @@ matheval(char *s)
     if (!mlevel)
 	outputradix = outputunderscore = 0;
 
-    if (!*s) {
+    if (!*s || *s == Nularg) {
 	x.type = MN_INTEGER;
 	x.u.l = 0;
 	return x;
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index d3176dd..e2dfe56 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -383,4 +383,7 @@
   print ${$(( $1 * 100 ))%%.[0-9]#})
 0:Arithmetic substitution nested in parameter substitution
 >3246
- 
+
+  print $((`:`))
+0:Null string in arithmetic evaluation after command substitution
+>0

broken parsing with $((`:`))

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

looks like zsh doesn't correctly parse this:
$ zsh -c 'echo $((`:`))'
zsh:1: bad math expression: illegal character: \M-]

a little whitespace makes it happy:
$ zsh -c 'echo $(( `:`))'
0
$ zsh -c 'echo $((`:` ))'
0

same for $():
$ zsh -c 'echo $(($(:)))'
zsh:1: bad math expression: illegal character: \M-]
$ zsh -c 'echo $(( $(:)))'
0
$ zsh -c 'echo $(($(:) ))'
0

looks like it's related to the subshell not outputting anything.
if you use 'echo 0' instead of ':', it works out.

$ zsh --version
zsh 5.0.7 (x86_64-pc-linux-gnu)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]