kernel: evaluate function call arguments before options

Previously, this was what the immediate interpreter did, but *not* what the executor did; the latter always evaluated options first.
gap-system · Aug 20, 2021 · 0d3d39c · 0d3d39c
1 parent 86de135
commit 0d3d39c
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 61 deletions.
diff --git a/src/funcs.c b/src/funcs.c
@@ -81,32 +81,6 @@ void SetRecursionDepth(Int depth)
     FuncsState()->RecursionDepth = depth;
 }
 
-/****************************************************************************
-**
-*F ExecProccallOpts( <call> ). . execute a procedure call with options
-**
-** Calls with options are wrapped in an outer statement, which is
-** handled here
-*/
-
-static Obj PushOptions;
-static Obj PopOptions;
-
-static UInt ExecProccallOpts(Stat call)
-{
-  Obj opts;
-
-  opts = EVAL_EXPR(READ_STAT(call, 0));
-  CALL_1ARGS(PushOptions, opts);
-
-  EXEC_STAT(READ_STAT(call, 1));
-
-  CALL_0ARGS(PopOptions);
-
-  return 0;
-}
-
-
 /****************************************************************************
 **
 *F  ExecProccall0args(<call>)  .  execute a procedure call with 0    arguments
@@ -125,7 +99,10 @@ static UInt ExecProccallOpts(Stat call)
 **  resulting from the procedure call, which in fact is always 0.
 */
 
-static ALWAYS_INLINE Obj EvalOrExecCall(Int ignoreResult, UInt nr, Stat call)
+static Obj PushOptions;
+static Obj PopOptions;
+
+static ALWAYS_INLINE Obj EvalOrExecCall(Int ignoreResult, UInt nr, Stat call, Stat opts)
 {
     Obj func;
     Obj a[6] = { 0 };
@@ -152,6 +129,10 @@ static ALWAYS_INLINE Obj EvalOrExecCall(Int ignoreResult, UInt nr, Stat call)
         }
     }
 
+    if (opts) {
+        CALL_1ARGS(PushOptions, EVAL_EXPR(opts));
+    }
+
     // call the function
     SET_BRK_CALL_TO( call );
     if (TNUM_OBJ(func) != T_FUNCTION) {
@@ -190,68 +171,91 @@ static ALWAYS_INLINE Obj EvalOrExecCall(Int ignoreResult, UInt nr, Stat call)
         GAP_THROW();
     }
 
-    if (ignoreResult) {
-        return 0;
+    if (!ignoreResult && result == 0) {
+        ErrorMayQuit("Function Calls: <func> must return a value", 0, 0);
     }
 
-    if (result == 0) {
-        ErrorMayQuit("Function Calls: <func> must return a value", 0, 0);
+    if (opts) {
+        CALL_0ARGS(PopOptions);
     }
+
     return result;
 }
 
 
+/****************************************************************************
+**
+*F  ExecProccallOpts( <call> ). . execute a procedure call with options
+**
+**  Calls with options are wrapped in an outer statement, which is
+**  handled here
+*/
+
+static UInt ExecProccallOpts(Stat call)
+{
+    Expr opts = READ_STAT(call, 0);
+    Expr real_call = READ_STAT(call, 1);
+    UInt type = TNUM_STAT(real_call);
+    GAP_ASSERT(/*STAT_PROCCALL_0ARGS <= type && */type <= STAT_PROCCALL_XARGS);
+    UInt narg = (type - STAT_PROCCALL_0ARGS);
+
+    EvalOrExecCall(1, narg, real_call, opts);
+
+    return 0;
+}
+
+
 static UInt ExecProccall0args(Stat call)
 {
-    EvalOrExecCall(1, 0, call);
+    EvalOrExecCall(1, 0, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 static UInt ExecProccall1args(Stat call)
 {
-    EvalOrExecCall(1, 1, call);
+    EvalOrExecCall(1, 1, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 static UInt ExecProccall2args(Stat call)
 {
-    EvalOrExecCall(1, 2, call);
+    EvalOrExecCall(1, 2, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 static UInt ExecProccall3args(Stat call)
 {
-    EvalOrExecCall(1, 3, call);
+    EvalOrExecCall(1, 3, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 static UInt ExecProccall4args(Stat call)
 {
-    EvalOrExecCall(1, 4, call);
+    EvalOrExecCall(1, 4, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 static UInt ExecProccall5args(Stat call)
 {
-    EvalOrExecCall(1, 5, call);
+    EvalOrExecCall(1, 5, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 static UInt ExecProccall6args(Stat call)
 {
-    EvalOrExecCall(1, 6, call);
+    EvalOrExecCall(1, 6, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
@@ -262,34 +266,29 @@ static UInt ExecProccallXargs(Stat call)
     // pass in 7 (instead of NARG_SIZE_CALL(SIZE_STAT(call)))
     // to allow the compiler to perform better optimizations
     // (as we know that the number of arguments is >= 7 here)
-    EvalOrExecCall(1, 7, call);
+    EvalOrExecCall(1, 7, call, 0);
 
     // return 0 (to indicate that no leave-statement was executed)
     return 0;
 }
 
 /****************************************************************************
 **
-*F EvalFunccallOpts( <call> ). . evaluate a function call with options
+*F  EvalFunccallOpts( <call> ). . evaluate a function call with options
 **
-** Calls with options are wrapped in an outer statement, which is
-** handled here
+**  Calls with options are wrapped in an outer statement, which is
+**  handled here
 */
 
 static Obj EvalFunccallOpts(Expr call)
 {
-  Obj opts;
-  Obj res;
-
-
-  opts = EVAL_EXPR(READ_STAT(call, 0));
-  CALL_1ARGS(PushOptions, opts);
-
-  res = EVAL_EXPR(READ_STAT(call, 1));
-
-  CALL_0ARGS(PopOptions);
+    Expr opts = READ_STAT(call, 0);
+    Expr real_call = READ_STAT(call, 1);
+    UInt type = TNUM_STAT(real_call);
+    GAP_ASSERT(EXPR_FUNCCALL_0ARGS <= type && type <= EXPR_FUNCCALL_XARGS);
+    UInt narg = (type - EXPR_FUNCCALL_0ARGS);
 
-  return res;
+    return EvalOrExecCall(0, narg, real_call, opts);
 }
 
 
@@ -311,45 +310,45 @@ static Obj EvalFunccallOpts(Expr call)
 
 static Obj EvalFunccall0args(Expr call)
 {
-    return EvalOrExecCall(0, 0, call);
+    return EvalOrExecCall(0, 0, call, 0);
 }
 
 static Obj EvalFunccall1args(Expr call)
 {
-    return EvalOrExecCall(0, 1, call);
+    return EvalOrExecCall(0, 1, call, 0);
 }
 
 static Obj EvalFunccall2args(Expr call)
 {
-    return EvalOrExecCall(0, 2, call);
+    return EvalOrExecCall(0, 2, call, 0);
 }
 
 static Obj EvalFunccall3args(Expr call)
 {
-    return EvalOrExecCall(0, 3, call);
+    return EvalOrExecCall(0, 3, call, 0);
 }
 
 static Obj EvalFunccall4args(Expr call)
 {
-    return EvalOrExecCall(0, 4, call);
+    return EvalOrExecCall(0, 4, call, 0);
 }
 
 static Obj EvalFunccall5args(Expr call)
 {
-    return EvalOrExecCall(0, 5, call);
+    return EvalOrExecCall(0, 5, call, 0);
 }
 
 static Obj EvalFunccall6args(Expr call)
 {
-    return EvalOrExecCall(0, 6, call);
+    return EvalOrExecCall(0, 6, call, 0);
 }
 
 static Obj EvalFunccallXargs(Expr call)
 {
     // pass in 7 (instead of NARG_SIZE_CALL(SIZE_EXPR(call)))
     // to allow the compiler to perform better optimizations
     // (as we know that the number of arguments is >= 7 here)
-    return EvalOrExecCall(0, 7, call);
+    return EvalOrExecCall(0, 7, call, 0);
 }
 
 

diff --git a/tst/testbugfix/2021-08-19-FuncCallOptionEvalOrder.tst b/tst/testbugfix/2021-08-19-FuncCallOptionEvalOrder.tst
@@ -0,0 +1,32 @@
+#@local myfunc, func_call, proc_call
+# Fix GitHub issue #4631: The evaluation order of arguments versus
+# options in function and procedure calls differed between the immediate
+# interpreter, and the executor for coded statements.
+gap> myfunc := function( )
+>       Display( ValueOption( "myopt" ) );
+>       return 1;
+>    end;;
+gap> func_call := function( )
+>       # a function call follows; one of the arguments uses myopt as a side effect
+>       return IdFunc( myfunc( ) : myopt := "myopt_value" );
+>    end;;
+gap> proc_call := function( )
+>     # a procedure call follows; one of the arguments uses myopt as a side effect
+>     Ignore( myfunc( ) : myopt := "myopt_value" );
+>    end;;
+
+# call as function, delayed
+gap> func_call( );;
+fail
+
+# call as function, immediately
+gap> IdFunc( myfunc( ) : myopt := "myopt_value" );;
+fail
+
+# call as procedure, delayed
+gap> proc_call( );
+fail
+
+# call as procedure, immediately
+gap> Ignore( myfunc( ) : myopt := "myopt_value" );
+fail
diff --git a/tst/testinstall/kernel/funcs.tst b/tst/testinstall/kernel/funcs.tst
@@ -1,3 +1,12 @@
+#
+# Tests for functions defined in src/funcs.c
+#
+gap> START_TEST("kernel/funcs.tst");
+
+#
 gap> SetRecursionTrapInterval(fail);
 Error, SetRecursionTrapInterval: <interval> must be a small integer greater th\
 an 5 (not the value 'fail')
+
+#
+gap> STOP_TEST("kernel/funcs.tst", 1);