Add extra argument to CallListFunc, to wrap return value.

[ChrisJefferson]

This has been previously requested by @fingolfin and myself.

Basically, CallListFunc (and GAP in general) doesn't provide a generic way of calling a function and capturing the return value of a function if one exists, but not breaking if there is not.

This adds CallListFuncWrap which is exactly the same as CallListFunc, except it returns a list of size 0 if the function returns nothing, and size 1 if there is a return value (containing that return value).

Description of changes (for the release notes)

Add CallListFuncWrap, an extension to CallListFunc which allows generic code to handle functions which may or may not return a value.

[markuspf]

I wanted such a function for quite some time.

The only issue I have is with naming. Maybe it is time to formalise the concept of an empty return value and give it a name (and wrap is probably not it ;)).

I think you called it the "option type" before, I might call it "Maybe". I don't know whether there are any better names around.

[ChrisJefferson]

I didn't really like the name either.

I also wondered about introducing an explicit 'option' type. It would be easy to do, but it would I think be about as efficient as this (empty list, plist of length 1 -- you could save 16 bytes on the plist of length 1 case), so I'm not sure it's worth adding a new C-level type for, unless someone wants to build a bunch of extra stuff around it.

[markuspf]

We could just give this pattern a name, and not introduce an explicit type, if people prefer that.

It'd be good to get some input from others (@fingolfin, @stevelinton, @frankluebeck come to mind) here.

[frankluebeck]

How about mimicing what happens on C-level (where NULL is returned to indicate "no return value")?

We could introduce a special GAP object, say with name NO_RETURN_VALUE and change CallFuncList such that is always returns something, that is it returns NO_RETURN_VALUE where it currently returns nothing.

[ChrisJefferson]

The problem with NO_RETURN_VALUE is that people will start returning it from functions, and then we don't know if the function had no return value, or if it returned NO_RETURN_VALUE (this is how SuPeRfail came about I think, did a function return fail, or "really fail").

We could forbid GAP-level functions from returning NO_RETURN_VALUE,but that might be a bit hacky.

[fingolfin]

We could also make it so that return NO_RETURN_VALUE actually becomes equivalent to return;`.

Of course that would make NO_RETURN_VALUE a very special value. It propably should print as the empty string, for consistency (i.e. to match this:

gap> funcWhichReturnsNothing();
gap> 

and so on.

Still, all in all I am not sure whether complicating the interpreter by adding this special case is a good idea? Is the benefit it provides really substantial enough to justify this?

For a time, I thought it was a great idea, and I wanted a NO_RETURN_VALUE in GAP matching Python's None, but the more I thought about it, the more problems and corner cases popped up.... If this was the early days of GAP development, I'd strongly push for it, but as it is, it's much too later for that.

[olexandr-konovalov]

I'd suggest to go and merge this PR as it does useful thing. I am not sure about the need for a special NO_RETURN_VALUE (taking into account the story of SuPeRfail). The only thing is I also don't like Wrap in CallListFuncWrap. Maybe CallListFuncList to emphasise that it returns a list? Or maybe better call it CallProcList to emphasise that it can be used with procedures as well?

[frankluebeck]

What is the original motivation for this extension?

Currently, GAP functions can return nothing or one object. The motivation here must be that there are situations where one does't know if a function returns something or not, and that one would like to find out what is the case. This doesn't seem to be something specific for CallFuncList (not CallListFunc).

If such situations really occur (who has convicing examples?), it would be sensible to change GAP such that every function call returns something, where a special object NO_OBJECT is returned in cases where currently nothing is returned. That would be similar to python's None or Maple's NULL which I have actually used on a few occasions. Implementing this is probably pretty easy since on C-level there is already a return value 0 if no object is returned, and this should be completely backward compatible.

[ChrisJefferson]

Here is my most recent reason, I'll see if I can think of any others.

I have a function TimeFunction(f, args), which returns a record containing the time taken for f to run, and also it's return value. Except, the function doesn't work when f returns nothing and it's hard to work around.

[ChrisJefferson]

I tried adding a 'no return' / None value to GAP, in #829 . Turned out to be very little change, and (I believe) should be completely backwards compatible with any working code.

[fingolfin]

Actually, I also wonder about use cases... And I don't quite recall requesting exactly this, despite what @ChrisJefferson wrote :-) [ don't get me wrong: I don't recall it, but I may very well have done so, and just forgotten. I do recall discussing things related to this for sure, e.g. concerning SuPeRFail etc.. ]

As it is, I couldn't give a usecase for this function, though the one @ChrisJefferson mentions is compelling. Hmm, maybe I had a usecase for this in the context of unit testing, but since I am using CALL_WITH_CATCH there, I can't think of a pressing reason there...

[ChrisJefferson]

@fingolfin : Maybe CALL_WITH_CATCH provided what you needed. I could also use CALL_WITH_CATCH (I don't care about something being thrown). But it would be nice to have the option to not catch.

There seems to be a family of functions: call a function which may or may not return, and maybe catch if it throws.

We can do:

• Function returns a value, catch (CALL_WITH_CATCH)
• Function doesn't return a value, catch (CALL_WITH_CATCH)
• Function maybe returns a value, catch (CALL_WITH_CATCH)
• Function returns a value, no catch (CallFuncList)
• Function doesn't return a value, no catch (CallFuncList)
• Function maybe returns a value, no catch (this pull request)

[fingolfin]

In principle, I am fine with this; my only (somewhat weak) concern is that this feels a bit like adding another arbitrary name and function to GAP, in an ad-hoc fashion.

Alas, it'll be a rarely used function for a rare problem; plus, GAP is already riddled with arbitrary function and inconsistent names, so this is a very weak counter argument

So, all in all, I have no deep objection to this PR. Only question I'd like to raise is whether we want to official document this (in the reference manual), which would mean we essentially could never remove it again; or whether to keep the documentation comment, but not hook it up to the reference manual, thus leaving us a hypothetical way out...

But perhaps I am just overly cautious.

[ChrisJefferson]

Alternative option:

Give CallFuncList an optional 3rd argument, which is record of options. Add the option wrapreturn.

This would also let us add things like catcherror, which would wrap the undocumented CALL_WITH_CATCH.

[ChrisJefferson]

Now changed CallFuncList to take an options record, which includes a single option wrapreturn, which implements this functionality.

[codecov-io]

Current coverage is 49.49% (diff: 83.33%)

Merging #824 into master will decrease coverage by 0.03%

@@             master       #824   diff @@
==========================================
  Files           424        424          
  Lines        223264     223274    +10   
  Methods        3429       3430     +1   
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         110600     110516    -84   
- Misses       112664     112758    +94   
  Partials          0          0          

Powered by Codecov. Last update 7bdb207...6fd721a

[fingolfin]

One problem with this version of the PR is that it changes the signature of CALL_FUNC_LIST. But at least on package (ToolsForHomalg) uses it -- granted, it shouldn't do that, but we should first get that changed (see homalg-project/homalg_project#165)

Next, the GAP library calls CALL_FUNC_LIST in several more places, which this PR does not update.

The PR also changes CallFuncList from an operation (DeclareOperationKernel) to a global function. Not sure whether this has any consequences, but it would be good if this was at least briefly looked into.

Finally, if it was me, I wouldn't want to write code dealing with records on the C level if I didn't have to... in this particular case, I'd either ditch the record, and just use a single extra param; or I'd write a high-level GAP function doing the argument validation, and a low-level C function which takes the extracted params, without doing additional validation.

All in all, I wonder if it wouldn't be much simpler and safer to just add a CallListFuncWrap function... :-)

[ChrisJefferson]

I should have looked to see if anyone was using CALL_FUNC_LIST. As you say no-one should, be let's leave it in place. I'll just go back to two C level functions (CALL_FUNC_LIST and CALL_FUNC_LIST_WRAP). That still leaves the issue of the GAP-level function of course.

[ChrisJefferson]

I remember why I didn't implement CallFuncList at the GAP level, calling CALL_FUNC_LIST and CALL_FUNC_LIST_WRAP as approriate. Because (the whole reason for this commit), we can't call CALL_FUNC_LIST generically, and return it's return value, because we don't know if it will return a value or not! So we have to implement CallFuncList in the kernel, if we want to sometimes return a value and sometimes (or change the kernel function, but then we are changing the kernel function).

[ChrisJefferson]

Seeing as git, and github, are a bit annoying, I destroyed my original calllistfuncwrap editing this pull request. Woops.

[ChrisJefferson]

I'm going to hold on this for a minute, as I think fixing #953 might fix this as part of the fallout.

[fingolfin]

@ChrisJefferson I don't understand your explanation why you didin't implement CallFuncList at the GAP level. Of course right now this is not possible, but with your patch, it is, because it could simply always call CALL_FUNC_LIST_WRAP. As I wrote: "I'd write a high-level GAP function doing the argument validation, and a low-level C function which takes the extracted params, without doing additional validation."

And then you are free to handle record arguments on the GAP level. That said, I am still not convinced this genericy (with a record argument) is necessary. If there was a compelling future usecase, even a purely hypothetical one, OK; but at this point, it seems to just add complexity with no gain.

[ChrisJefferson]

Now back to something super-simple. Could technically just have one C function and 2 at the GAP level, but this avoid making a list and unwrapping it if I just had CALL_FUNC_LIST_WRAP at the C level.

[fingolfin]

I am fine with merging this. Thouh I still wonder what the usecase is that @ChrisJefferson had in mind... ? i.e. will this be immediately used for something? Or is it more of a theoretical exercise... ?

[ChrisJefferson]

I want to use it for generic wrapping for timing. I have a number of functions that look a bit like this:

timeFunction := function(func)
    local time;
    time := 0;
    return rec(
        getNanos := function() return time; end,
        func := function(args...)
            local ret, starttime;
            starttime := NanosecondsSinceEpoch();
            ret := CallFuncListWrap(func, args);
            time := time + (NanosecondsSinceEpoch() - starttime);
            if ret = [] then
                return;
            else
                return ret[1];
            fi;
        end
    );
end;

So I can wrap any functions I want easily, to check which functions are taking time. You can do this with the function profiler, but sometimes I want to record the time taken by every call of a function, to draw nice plots, for example.

At the moment I have an extra argument to timeFunction, which records if the function returns a value or not, so I can use CallFuncList, but that's a bit annoying.