Arith expansion accepts extra closing parenthesis

Arith expansion accepts extra closing parenthesis

[Martijn Dekker]

I just found some more arithmetic parsing strangeness. zsh 5.0.8 and
4.3.11 both happily accept and ignore one (1) extra closing parenthesis
in variable expansion within arithmetic expansion (even in POSIX mode).

% X='1)'
% echo $(($X))
1
% echo $((X))
1

(Expected output: error message in both cases)

Yet, 'echo $((1)))' errors out as expected, as do 'echo $(($X))' and
'echo $((X))' for X='1))', X='1)))', X='(1', etc.

- Martijn

Re: Arith expansion accepts extra closing parenthesis

[Peter Stephenson]

On Mon, 1 Jun 2015 18:32:14 +0200
Martijn Dekker <martijn@inlv.org> wrote:
> I just found some more arithmetic parsing strangeness. zsh 5.0.8 and
> 4.3.11 both happily accept and ignore one (1) extra closing parenthesis
> in variable expansion within arithmetic expansion (even in POSIX mode).
> 
> % X='1)'
> % echo $(($X))
> 1
> % echo $((X))
> 1

I think this is a special case, down to the way we handle parenthesesed
expressions --- we treat it as if it's a top-level expression but
expect a closing parenthesis next.  So checking for a stray closing
parenthesis on return from the top level of parsing ought to be a robust
fix.

pws

diff --git a/Src/math.c b/Src/math.c
index 97a97b3..e20a90c 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -407,6 +407,13 @@ mathevall(char *s, enum prec_type prec_tp, char **ep)
     stack[0].val.type = MN_INTEGER;
     stack[0].val.u.l = 0;
     mathparse(prec_tp == MPREC_TOP ? TOPPREC : ARGPREC);
+    /*
+     * Internally, we parse the contents of parentheses at top
+     * precedence... so we can return a parenthesis here if
+     * there are too many at the end.
+     */
+    if (mtok == M_OUTPAR && !errflag)
+	zerr("unexpected ')'");
     *ep = ptr;
     DPUTS(!errflag && sp > 0,
 	  "BUG: math: wallabies roaming too freely in outback");
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index d284e08..2d35ea6 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -395,3 +395,17 @@
 >6
 >7
 >120
+
+  foo="(1)"
+  print $((foo))
+  print $(($foo))
+  print $(((2)))
+  foo="3)"
+  (print $((foo))) 2>&1
+  (print $(($foo))) 2>&1
+1: Good and bad trailing parentheses
+>1
+>1
+>2
+>(eval):6: unexpected ')'
+>(eval):7: unexpected ')'

Re: Arith expansion accepts extra closing parenthesis

[Peter Stephenson]

Some of the errors from arithmetic evaluation are a bit cryptic in that
it's not obvious the failure reported is actually to do with arithmetic
at all: "unexpected ')'" could refer to lots of uses of ')'.  This
improves the previous patch so that more messages indicate where the
error has come from.  I've left alone those that don't seem as cryptic.

I'm glad I've never been "out of integers".  That sounds bad.

pws

diff --git a/Src/math.c b/Src/math.c
index 97a97b3..977e923 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -407,6 +407,13 @@ mathevall(char *s, enum prec_type prec_tp, char **ep)
     stack[0].val.type = MN_INTEGER;
     stack[0].val.u.l = 0;
     mathparse(prec_tp == MPREC_TOP ? TOPPREC : ARGPREC);
+    /*
+     * Internally, we parse the contents of parentheses at top
+     * precedence... so we can return a parenthesis here if
+     * there are too many at the end.
+     */
+    if (mtok == M_OUTPAR && !errflag)
+	zerr("bad math expression: unexpected ')'");
     *ep = ptr;
     DPUTS(!errflag && sp > 0,
 	  "BUG: math: wallabies roaming too freely in outback");
@@ -791,7 +798,7 @@ zzlex(void)
 
 		    ptr++;
 		    if (!*ptr) {
-			zerr("character missing after ##");
+			zerr("bad math expression: character missing after ##");
 			return EOI;
 		    }
 		    ptr = getkeystring(ptr, NULL, GETKEYS_MATH, &v);
@@ -914,7 +921,7 @@ setmathvar(struct mathvalue *mvp, mnumber v)
 	mvp->pval = NULL;
     }
     if (!mvp->lval) {
-	zerr("lvalue required");
+	zerr("bad math expression: lvalue required");
 	v.type = MN_INTEGER;
 	v.u.l = 0;
 	return v;
@@ -1256,7 +1263,7 @@ op(int what)
 			/* Error if (-num ** b) and b is not an integer */
 			double tst = (double)(zlong)b.u.d;
 			if (tst != b.u.d) {
-			    zerr("imaginary power");
+			    zerr("bad math expression: imaginary power");
 			    return;
 			}
 		    }
@@ -1338,7 +1345,7 @@ op(int what)
 	push(((a.type & MN_FLOAT) ? a.u.d : a.u.l) ? b : c, NULL, 0);
 	break;
     case COLON:
-	zerr("':' without '?'");
+	zerr("bad math expression: ':' without '?'");
 	break;
     case PREPLUS:
 	if (spval->type & MN_FLOAT)
@@ -1355,7 +1362,7 @@ op(int what)
 	setmathvar(stack + sp, *spval);
 	break;
     default:
-	zerr("out of integers");
+	zerr("bad math expression: out of integers");
 	return;
     }
 }
@@ -1525,7 +1532,7 @@ mathparse(int pc)
 	    mathparse(TOPPREC);
 	    if (mtok != M_OUTPAR) {
 		if (!errflag)
-		    zerr("')' expected");
+		    zerr("bad math expression: ')' expected");
 		return;
 	    }
 	    break;
@@ -1543,7 +1550,7 @@ mathparse(int pc)
 		noeval--;
 	    if (mtok != COLON) {
 		if (!errflag)
-		    zerr("':' expected");
+		    zerr("bad math expression: ':' expected");
 		return;
 	    }
 	    if (q)
diff --git a/Test/C01arith.ztst b/Test/C01arith.ztst
index d284e08..2d35ea6 100644
--- a/Test/C01arith.ztst
+++ b/Test/C01arith.ztst
@@ -395,3 +395,17 @@
 >6
 >7
 >120
+
+  foo="(1)"
+  print $((foo))
+  print $(($foo))
+  print $(((2)))
+  foo="3)"
+  (print $((foo))) 2>&1
+  (print $(($foo))) 2>&1
+1: Good and bad trailing parentheses
+>1
+>1
+>2
+>(eval):6: unexpected ')'
+>(eval):7: unexpected ')'

Re: Arith expansion accepts extra closing parenthesis

[Bart Schaefer]

On Jun 1,  6:32pm, Martijn Dekker wrote:
} Subject: Arith expansion accepts extra closing parenthesis
}
} I just found some more arithmetic parsing strangeness. zsh 5.0.8 and
} 4.3.11 both happily accept and ignore one (1) extra closing parenthesis
} 
} % X='1)'
} % echo $((X))
} 1

Easier way to see this:

% let "1)"
%

It appears mathparse() around lines 1566-1568 of math.c consumes one
token of lookahead with zzlex() and inadvertently discards it when it
turns out to be a close-paren.  This is partly because of the pseuedo-
precedence assigned to M_OUTPAR "for convenience".

I'm not sure what the correct fix is; maybe checkunary() should throw
an error on M_OUTPAR?

Re: Arith expansion accepts extra closing parenthesis

[Bart Schaefer]

Oops, sorry for the delayed reaction email on this -- I was AFK all day
yesterday and didn't notice that there had been other replies.