Document and unify the order in which GAP evaluates function arguments and options

doc/ref/language.xml

@@ -1130,15 +1130,13 @@ Remember that a variable is a location in a ⪆ program
 that points to a value. Thus for  each formal argument and for each
 formal local such a location is allocated.
 <P/>
-Next the arguments <A>arg-expr</A>s are evaluated, and the values are assigned
+Next the arguments <A>arg-expr</A>s are evaluated from left to right,
+and the values are assigned
 to the newly created variables corresponding to the formal arguments. Of
 course the first value is assigned to the new variable corresponding to
 the first formal argument, the second  value  is assigned to the new
 variable corresponding  to  the second  formal argument, and  so on.
-However, &GAP; does not make any guarantee about the order in which the
-arguments are evaluated. They might be evaluated left to right, right to
-left, or in any other order, but each argument is evaluated once. An
-exception again occurs if the last formal argument has
+An exception again occurs if the last formal argument has
 the name <C>arg</C>. In this case the values of all the actual
 arguments not assigned to the other formal parameters are
 stored in  a list and this  list is assigned to the  new variable
@@ -1201,6 +1199,8 @@ the same syntax as the components of a record expression. The one
 exception to this is that a component name may appear without a value,
 in which case the value <K>true</K> is silently inserted.
 <P/>
+Options are evaluated from left to right, but only after all arguments have been evaluated.
+<P/>
 The following example shows a call to <Ref Attr="Size"/> passing the options
 <C>hard</C> (with the value <K>true</K>)
 and <C>tcselection</C> (with the string <C>"external"</C> as value).

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 };
@@ -134,7 +111,7 @@ static ALWAYS_INLINE Obj EvalOrExecCall(Int ignoreResult, UInt nr, Stat call)
 
     // evaluate the function
     func = EVAL_EXPR( FUNC_CALL( call ) );
- 
+
     // evaluate the arguments
     if (nr <= 6 && TNUM_OBJ(func) == T_FUNCTION) {
         for (UInt i = 1; i <= nr; i++) {
@@ -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,28 @@ 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);
-
-  return res;
+    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 EvalOrExecCall(0, narg, real_call, opts);
 }
 
 
@@ -311,45 +309,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);
 }
 
 

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

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);