expr: fix adjacent terms in function calls

Previously two adjacent terms were incorrectly accepted, such as in
pow(2 3)

Correctly generate an error in this case and improve the error
message when an expression fails to parse correctly.

Signed-off-by: Steve Bennett <steveb@workware.net.au>

3 files changed, 104 insertions, 104 deletions

diff --git a/jim.c b/jim.c
index 9e41d8c..5b0d09c 100644
--- a/jim.c
+++ b/jim.c
@@ -1151,6 +1151,10 @@ void Jim_FreeStackElements(Jim_Stack *stack, void (*freeFunc) (void *ptr))
 #define JIM_TT_EXPR_OP        20
 
 #define TOKEN_IS_SEP(type) (type >= JIM_TT_SEP && type <= JIM_TT_EOF)
+/* Can this token start an expression? */
+#define TOKEN_IS_EXPR_START(type) (type == JIM_TT_NONE || type == JIM_TT_SUBEXPR_START || type == JIM_TT_SUBEXPR_COMMA)
+/* Is this token an expression operator? */
+#define TOKEN_IS_EXPR_OP(type) (type >= JIM_TT_EXPR_OP)
 
 /**
  * Results of missing quotes, braces, etc. from parsing.
@@ -9038,7 +9042,7 @@ static void ExprTernaryReorderExpression(Jim_Interp *interp, ExprByteCode *expr)
     }
 }
 
-static ExprByteCode *ExprCreateByteCode(Jim_Interp *interp, const ParseTokenList *tokenlist, Jim_Obj *fileNameObj)
+static ExprByteCode *ExprCreateByteCode(Jim_Interp *interp, const ParseTokenList *tokenlist, Jim_Obj *exprObjPtr, Jim_Obj *fileNameObj)
 {
     Jim_Stack stack;
     ExprByteCode *expr;
@@ -9084,112 +9088,109 @@ static ExprByteCode *ExprCreateByteCode(Jim_Interp *interp, const ParseTokenList
             break;
         }
 
-        switch (t->type) {
-            case JIM_TT_STR:
-            case JIM_TT_ESC:
-            case JIM_TT_VAR:
-            case JIM_TT_DICTSUGAR:
-            case JIM_TT_EXPRSUGAR:
-            case JIM_TT_CMD:
-            case JIM_TT_EXPR_BOOLEAN:
-                token->type = t->type;
-strexpr:
-                token->objPtr = Jim_NewStringObj(interp, t->token, t->len);
-                if (t->type == JIM_TT_CMD) {
-                    /* Only commands need source info */
-                    JimSetSourceInfo(interp, token->objPtr, fileNameObj, t->line);
-                }
-                expr->len++;
-                break;
+        if (TOKEN_IS_EXPR_OP(t->type)) {
+            const struct Jim_ExprOperator *op;
+            ParseToken *tt;
 
-            case JIM_TT_EXPR_INT:
-            case JIM_TT_EXPR_DOUBLE:
-                {
-                    char *endptr;
-                    if (t->type == JIM_TT_EXPR_INT) {
-                        token->objPtr = Jim_NewIntObj(interp, jim_strtoull(t->token, &endptr));
-                    }
-                    else {
-                        token->objPtr = Jim_NewDoubleObj(interp, strtod(t->token, &endptr));
-                    }
-                    if (endptr != t->token + t->len) {
-                        /* Conversion failed, so just store it as a string */
-                        Jim_FreeNewObj(interp, token->objPtr);
-                        token->type = JIM_TT_STR;
-                        goto strexpr;
-                    }
-                    token->type = t->type;
-                    expr->len++;
+            /* Convert -/+ to unary minus or unary plus if necessary */
+            if (prevtt == JIM_TT_NONE || prevtt == JIM_TT_SUBEXPR_START || prevtt == JIM_TT_SUBEXPR_COMMA || prevtt >= JIM_TT_EXPR_OP) {
+                if (t->type == JIM_EXPROP_SUB) {
+                    t->type = JIM_EXPROP_UNARYMINUS;
                 }
-                break;
+                else if (t->type == JIM_EXPROP_ADD) {
+                    t->type = JIM_EXPROP_UNARYPLUS;
+                }
+            }
 
-            case JIM_TT_SUBEXPR_START:
-                Jim_StackPush(&stack, t);
-                break;
+            op = JimExprOperatorInfoByOpcode(t->type);
 
-            case JIM_TT_SUBEXPR_END:
-            case JIM_TT_SUBEXPR_COMMA:
-                ok = 0;
-                while (Jim_StackLen(&stack)) {
-                    ParseToken *tt = Jim_StackPop(&stack);
+            /* Handle precedence */
+            while ((tt = Jim_StackPeek(&stack)) != NULL) {
+                const struct Jim_ExprOperator *tt_op =
+                    JimExprOperatorInfoByOpcode(tt->type);
 
-                    if (tt->type == JIM_TT_SUBEXPR_START || tt->type == JIM_TT_SUBEXPR_COMMA) {
-                        if (t->type == JIM_TT_SUBEXPR_COMMA) {
-                            /* Need to push back the previous START or COMMA in the case of comma */
-                            Jim_StackPush(&stack, tt);
-                        }
-                        ok = 1;
-                        break;
-                    }
+                /* Note that right-to-left associativity of ?: operator is handled later */
 
+                if (op->arity != 1 && tt_op->precedence >= op->precedence) {
                     if (ExprAddOperator(interp, expr, tt) != JIM_OK) {
+                        ok = 0;
                         goto err;
                     }
+                    Jim_StackPop(&stack);
                 }
-                if (!ok) {
-                    Jim_SetResultString(interp, "Unexpected close parenthesis", -1);
-                    goto err;
+                else {
+                    break;
                 }
-                break;
+            }
+            Jim_StackPush(&stack, t);
+        }
+        else if (t->type == JIM_TT_SUBEXPR_START) {
+            Jim_StackPush(&stack, t);
+        }
+        else if (t->type == JIM_TT_SUBEXPR_END || t->type == JIM_TT_SUBEXPR_COMMA) {
+            /* Reduce the expression back to the previous ( or , */
+            ok = 0;
+            while (Jim_StackLen(&stack)) {
+                ParseToken *tt = Jim_StackPop(&stack);
 
-            default:{
-                    /* Must be an operator */
-                    const struct Jim_ExprOperator *op;
-                    ParseToken *tt;
-
-                    /* Convert -/+ to unary minus or unary plus if necessary */
-                    if (prevtt == JIM_TT_NONE || prevtt == JIM_TT_SUBEXPR_START || prevtt == JIM_TT_SUBEXPR_COMMA || prevtt >= JIM_TT_EXPR_OP) {
-                        if (t->type == JIM_EXPROP_SUB) {
-                            t->type = JIM_EXPROP_UNARYMINUS;
-                        }
-                        else if (t->type == JIM_EXPROP_ADD) {
-                            t->type = JIM_EXPROP_UNARYPLUS;
-                        }
+                if (tt->type == JIM_TT_SUBEXPR_START || tt->type == JIM_TT_SUBEXPR_COMMA) {
+                    if (t->type == JIM_TT_SUBEXPR_COMMA) {
+                        /* Need to push back the previous START or COMMA in the case of comma */
+                        Jim_StackPush(&stack, tt);
                     }
+                    ok = 1;
+                    break;
+                }
+                if (ExprAddOperator(interp, expr, tt) != JIM_OK) {
+                    goto err;
+                }
+            }
+            if (!ok) {
+                Jim_SetResultFormatted(interp, "Unexpected close parenthesis in expression: \"%#s\"", exprObjPtr);
+                goto err;
+            }
+        }
+        else {
+            Jim_Obj *objPtr = NULL;
 
-                    op = JimExprOperatorInfoByOpcode(t->type);
+            /* This is a simple non-operator term, so create and push the appropriate object */
+            token->type = t->type;
 
-                    /* Now handle precedence */
-                    while ((tt = Jim_StackPeek(&stack)) != NULL) {
-                        const struct Jim_ExprOperator *tt_op =
-                            JimExprOperatorInfoByOpcode(tt->type);
+            /* Two consecutive terms without an operator is invalid */
+            if (!TOKEN_IS_EXPR_START(prevtt) && !TOKEN_IS_EXPR_OP(prevtt)) {
+                Jim_SetResultFormatted(interp, "missing operator in expression: \"%#s\"", exprObjPtr);
+                ok = 0;
+                goto err;
+            }
 
-                        /* Note that right-to-left associativity of ?: operator is handled later */
+            /* Immediately create a double or int object? */
+            if (t->type == JIM_TT_EXPR_INT || t->type == JIM_TT_EXPR_DOUBLE) {
+                char *endptr;
+                if (t->type == JIM_TT_EXPR_INT) {
+                    objPtr = Jim_NewIntObj(interp, jim_strtoull(t->token, &endptr));
+                }
+                else {
+                    objPtr = Jim_NewDoubleObj(interp, strtod(t->token, &endptr));
+                }
+                if (endptr != t->token + t->len) {
+                    /* Conversion failed, so just store it as a string */
+                    Jim_FreeNewObj(interp, objPtr);
+                    objPtr = NULL;
+                }
+            }
 
-                        if (op->arity != 1 && tt_op->precedence >= op->precedence) {
-                            if (ExprAddOperator(interp, expr, tt) != JIM_OK) {
-                                ok = 0;
-                                goto err;
-                            }
-                            Jim_StackPop(&stack);
-                        }
-                        else {
-                            break;
-                        }
-                    }
-                    Jim_StackPush(&stack, t);
-                    break;
+            if (objPtr) {
+                token->objPtr = objPtr;
+            }
+            else {
+                /* Everything else is stored a simple string term */
+                token->objPtr = Jim_NewStringObj(interp, t->token, t->len);
+                if (t->type == JIM_TT_CMD) {
+                    /* Only commands need source info */
+                    JimSetSourceInfo(interp, token->objPtr, fileNameObj, t->line);
                 }
+            }
+            expr->len++;
         }
         prevtt = t->type;
     }
@@ -9290,7 +9291,7 @@ static int SetExprFromAny(Jim_Interp *interp, struct Jim_Obj *objPtr)
     }
 
     /* Now create the expression bytecode from the tokenlist */
-    expr = ExprCreateByteCode(interp, &tokenlist, fileNameObj);
+    expr = ExprCreateByteCode(interp, &tokenlist, objPtr, fileNameObj);
 
     /* No longer need the token list */
     ScriptTokenListFree(&tokenlist);
diff --git a/tests/expr-old.test b/tests/expr-old.test
index 0bd9b53..f5422eb 100644
--- a/tests/expr-old.test
+++ b/tests/expr-old.test
@@ -747,7 +747,7 @@ test expr-old-34.2 {errors in math functions} -body {
 test expr-old-34.3 {errors in math functions} -body {
     expr {hypot("a b", 2.0)}
 } -returnCodes error -match glob -result *
-test expr-old-34.4 {errors in math functions} -constraints tcl -body {
+test expr-old-34.4 {errors in math functions} -body {
     expr {hypot(1.0 2.0)}
 } -returnCodes error -match glob -result *
 test expr-old-34.5 {errors in math functions} -body {
diff --git a/tests/expr.test b/tests/expr.test
index 682af89..dbe84f5 100644
--- a/tests/expr.test
+++ b/tests/expr.test
@@ -83,22 +83,21 @@ test expr-1.17 "Rotate left" jim {
 } {2}
 
 # This crashes older jim
-test expr-2.1 "bogus unarymin" {
-	catch {expr {unarymin 1}}
-	return 1
-} {1}
+test expr-2.1 "bogus unarymin" -body {
+	expr {unarymin 1}
+} -returnCodes error -match glob -result *
 
-test expr-2.2 "Ternary operator - missing colon" {
-	list [catch {expr {1 ? 2 3}} msg]
-} {1}
+test expr-2.2 "Ternary operator - missing colon" -body {
+	expr {1 ? 2 3}
+} -returnCodes error -match glob -result {missing operator*}
 
-test expr-2.3 "Ternary operator - missing third term" {
-	list [catch {expr {1 ? 2}} msg]
-} {1}
+test expr-2.3 "Ternary operator - missing third term" -body {
+	expr {1 ? 2}
+} -returnCodes error -match glob -result *
 
-test expr-2.4 "Ternary operator - missing question" {
-	list [catch {expr {1 : 2}} msg]
-} {1}
+test expr-2.4 "Ternary operator - missing question" -body {
+	expr {1 : 2}
+} -returnCodes error -match glob -result *
 
 test expr-3.1 "in, ni operators" {
 	set l {a b c d}