Added SWIG type translator for C functions that take GList* arguments

[featherfeet]

Fixes issues with Python SWIG bindings for functions that take GList arguments, see this thread.

[jralls]

The test failures aren't your fault, please rebase on current stable.

These type maps don't belong in base-typemaps.i You can move them to bindings/engine.i and guard them with #if defined(SWIGPYTHON) or in bindings/python/gnucash-core.i. Either way you won't need the includes.

[featherfeet]

@jralls , I have moved it to bindings/python/gnucash-core.i and put common/base-typemaps.i back to how it was.

[jralls]

The long line of tests on every list member is going to be pretty expensive for the types at the end of the else if chain, plus it conveys the wrong message to human readers that the lists (except for GList) can accept any of the types tested for. A separate typemap for each list type would be cleaner. GnuCash uses untyped GLists (by which I mean GLists that don't have a typedef'd name like AccountList for all sorts of things and lots of them aren't QofInstances. The simplest approach would be to take a void* (aka gpointer) argument, but that might require a bunch of casts in existing code to get it to compile.

There's no point in casting the pointer when calling g_list_prepend. g_list_prepend takes a void* so the compiler will ignore the cast to something else.

It's good that you're using g_list_prepend but sometimes the list order is significant so you should call g_list_reverse at the end of the loop.

Adding tests that exercise some of the function bindings that motivate this PR to make sure that it all works would be wise.

[featherfeet]

@jralls , your point about untyped GLists raises a hairier issue--we can't actually verify in the typemap for GList that every object in this list is of the type that the function wants. If you tried to pass a Python list of GncInvoice objects to something expecting a GList of GncLot objects, it would pass them through, then try to cast the GncInvoice structures in C into GncLots and get garbage or a buffer overrun.

Is this just something we're willing to accept because all these functions already take untyped GLists, or should we try to come up with some way of having the bindings check? Conceivably could make a Python class for LotList, AccountList, etc, and make those classes enforce that you can only add objects of a certain type. Would still have to have some large changes in the C or SWIG to be able to know what kind of GList a function actually wants.

[jralls]

Hairy indeed. I was refreshing my memory about typemaps this morning and found that typemaps see through typedefs so

%typemap(in) GList *, CommodityList *, SplitList *, AccountList *, LotList *, MonetaryList *, PriceList *, EntryList *

and

%typemap(in) GList* 

are equivalent because all of the others are typedefs of GList. But with separate typemaps [SWIG looks for the explicit type[]https://www.swig.org/Doc4.0/SWIGDocumentation.html#Typemaps_nn17) before checking typedefs.

It's probably not worth the effort to type-check the untypedeffed GList* arguments. If you really think it worthwhile you could write a typemap for each of the contained types and then declare the functions that use it, repeating for each type.

[featherfeet]

@jralls , I am unable to find a way to do this with just void *, I think because SWIG wants to check types through the ConvertPtr mechanism before passing them into C. If I do something like this, it segfaults later when trying to save the book:

SwigPyObject *python_object = (SwigPyObject *) PyObject_GetAttrString(python_object_wrapper, "instance");
$1 = g_list_prepend($1, python_object->ptr);

I have confirmed in a debugger that the PyObject_GetAttrString function does return a SwigPyObject when you get "instance". And I have confirmed that ->ptr looks like a GObject of type Lot, so I'm not totally clear on why this should be different from what I was doing before with explicit type conversions. Maybe ConvertPtr transforms it in some way or gets some member that is the actual GNCLot*?

[jralls]

@featherfeet You can find the code for SWIG_ConvertPtr in $BUILDDIR/bindings/python/gnucash_core.c. Note that it's a macro that (eventually) calls SWIG_Python_ConvertPtrAndOwn(PyObject *obj, void **ptr, swig_type_info *ty, int flags, int *own) with own = 0. There are a bunch of extra manipulations that affect the lifetime of the returned ptr and not doing them might be the source of the crash: If Python thinks it owns the objects memory and frees it that will cause a crash later. To learn why it crashed you have to examine the address that caused the segfault and compare it to what you got from Python in your typemap.

Looks like a GNCLot* in the debugger isn't necessarily conclusive. If you tell the debugger p *(GNCLot*)ptr it will print out the fields of a GNCLot with the values filled in from the memory pointed to by ptr. It will look pretty reasonable no matter what's in there. The only way to be sure is to p (char*)g_type_name_from_instance(&ptr->inst.object.g_type_instance), which will print out the type name. That still doesn't tell you that the object's members are all valid so it might still crash when you try to access one of them.

[featherfeet]

@jralls , based on the file you referenced, I found how to get the type directly from SWIG so we don't have the long string of type-checks. This still has the issue of letting you pass a GList of the wrong type of objects, but that's not really fixable without changing lots of things that take generic GList arguments.

Do I need to write a freearg typemap for this to not leak memory, or is that handled somewhere else in Gnucash? When I added the below code, it causes segfaults (presumably because ApplyPaymentSecs frees the GList itself for some reason, so it's a double free--but we can't rely on every function that takes a GList to do that).

%typemap(freearg) GList * {
    g_list_free($1);
}

Also, where would I start with regards to writing a test case for a Python binding function?

[jralls]

This still has the issue of letting you pass a GList of the wrong type of objects, but that's not really fixable without changing lots of things that take generic GList arguments.

Indeed. I don't think there's a good way to type-check GObjects from Python.

Do I need to write a freearg typemap for this to not leak memory, or is that handled somewhere else in Gnucash? When I added the below code, it causes segfaults (presumably because ApplyPaymentSecs frees the GList itself for some reason, so it's a double free--but we can't rely on every function that takes a GList to do that).

Correct on both counts. The best you can do is to expose a free function so that GLists can be freed from Python.

[jralls]

Also, where would I start with regards to writing a test case for a Python binding function?

The python bindings test directory has examples, though many of them are more integration than unit tests. For unit testing the sort of test you wrote for #2074 is generally sufficient: It's not up to bindings to ensure that the wrapped function does what it's supposed to, just to make sure that the inputs and outputs are what they're supposed to be.

[featherfeet]

@jralls , I have added a unit test that uses the payment function and checks that it zeroes out Accounts Receivable. The %freefunc directive appears to not be available from Python, and I can't directly write a free function that you call manually from Python because the GList* is on the C side.

[jralls]

@featherfeet I don't know how you got from

The best you can do is to expose a free function so that GLists can be freed from Python.

to %freefunc. You're right that that's not supported in Python, it's a SWIG-Ruby feature. The Arrays and Pointers section that I pointed you to has examples in Python.

Another option might be to use %newobject and typemap %newfree.

BTW, I've fixed the warnings that caused the Arch Linux CI failure, rebase to get it to pass.

[christopherlam]

I don't think this is needed. The prepends start from the end of the list. Plus, the g_list_reverse would typically be outside rather than inside the for loop.