[question] customization of a trampoline function definition

[kliegeois]

Binder generates automatically some trampoline classes when needed and generates their corresponding member functions.

I am currently facing a particular case where I need to modify one of the generated member function of one of the trampoline classes (I will provide more details bellow for the interested developers/users). It seems that there is currently no ways to do it using binder configuration options. @lyskov do you have recommendation on how to do so?

---

More detailed context:

Given a virtual function meant to return new instances of a type via a pointer to a base class, and a derived type declared in python, the return value cast won't keep the python object alive, resulting in a failure when attempting to call the virtual method on the new instance via the pointer. Because this function is called from c++, the return value policy has no effect.

pybind/pybind11#1774 (comment)
pybind/pybind11#1049 (comment)

Work around that requires modification of the binder generated trampoline member function:

pybind/pybind11#1049 (comment)

Other potential work around on the pybind11 side that might solves the issues without binder modifications:
https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst I did not find an ETA for the merging of this branch into master.
pybind/pybind11#2839 this PR has no activities since a year ago

In the case that I faced, the solution of pybind/pybind11#1049 (comment) worked.
To do so, I generated the trampoline class using binder and then I modified the clone member function to be consistent with pybind/pybind11#1049 (comment) with the shared pointer class that I am using. I could write a script that post-process the binder generated files and replace the definition of the function with one that suites my need in this case but this is not the cleanest way to resolve this and I am curious to have your point of view @lyskov on this.

[lyskov]

@kliegeois If i have to face this then i would keep option writing post-processing-scripts at the bottom of my option list. While it certainly easy to do it is also most likely will be a highest maintenance option in the long run. I think it should be really straightforward way to add this functionality to the Binder and that would be my recommendation if you are do not mind writing a bit of C++ code (i do not think anything fancy will be needed here).

The way i see it we can add per-member-function Binder config option that will instruct Binder to alter trampoline function generation. Let me know if you open to this idea and would like to implement it and i will outline changes thats need to be done (should be ~one-afternoon task unless i am overlooking something big). Thanks,

[kliegeois]

@lyskov I totally agree with your point of view and your proposition. I can probably work on this by the Christmas break. You can list here or send me an email with any recommendations for this. Thanks!

[lyskov]

I have given this more thoughts and it i see a two ways we can implement this. Neither is perfect so some extra discussion might be needed:

[a] Simplest approach would probably be to add add config option that will allow user to request python-counterpart creation for pure-virtual-functions. Code to modify for this will be around this line: https://github.com/RosettaCommons/binder/blob/master/source/class.cpp#L740. I imagine the logic would be about the following: if member function we about to bind is pure-virtual (ie m->isPure()) then check if user requested special handling for this function (say if config option +trampoline_create_python_counter_part MyClass::my_function was specified) and generate code outlined in pybind/pybind11#1049 (comment). Main issue with this approach is that it is not clear how to generalize this for function with type signature different from clone (ie shared_ptr<MyClass> clone()). (i am not sure about option name so trampoline_create_python_counter_part name is just a place holder for now)

[b] another approach would be to allow user to fully customize how particular trampoline function is generated similar to options +binder and +add_on_binder that we have for classes, say +trampoline_member_function_binder (tentative name). User supplied trampoline_binder will probably need to be templated with class type, function return type etc. And have type signature identical to member function that we about to bind but take a few extra arguments (class name as string, function name as string, ...). Issue with this approach is that user might need to provide multiple custom function for different classes and also need to be very careful when implementing it (ie be familiar with https://github.com/RosettaCommons/binder/blob/master/source/class.cpp#L663). So this whole approach while quite general seems a bit of overkill to me.

At the moment I am leaning more towards [a] since it is much easier both to write and use and see how it goes. Thoughts?

[kliegeois]

Thanks @lyskov for your comment!

I was thinking about something similar to [b], mostly because [a] does not provide enough customization. For example, if you use a custom holder type, pybind/pybind11#1049 (comment) is not exactly what you need: you need to update the code snippet to be consistent with your used holder type and its corresponding functions/constructor.

I was thinking of something as follows:

Given files:
include_1.hpp:

#include <pybind11/pybind11.h>
namespace py = pybind11;

class A {
public:
    A() = default;
    A(const A&) {}
    virtual ~A() = default;

    virtual void func() = 0;
    virtual std::shared_ptr<A> clone() const = 0;
};

include_2.hpp:

template <typename T, typename T_trampoline>
std::shared_ptr<T> myCloneFunction(const T *ptr_to_this) {
        auto self = py::cast(ptr_to_this);
        auto cloned = self.attr("clone")();

        auto keep_python_state_alive = std::make_shared<py::object>(cloned);
        auto ptr = cloned.cast<T_trampoline*>();

        // aliasing shared_ptr: points to `T_trampoline* ptr` but refcounts the Python object
        return std::shared_ptr<T>(keep_python_state_alive, ptr);
    }

Config_file.cfg:

+include_for_trampoline_class A <include_2.hpp>
+trampoline_member_function_binder A::clone myCloneFunction

Then, binder would generate something similar to this:

#include <include_2.hpp>

class A_trampoline: public A {
public:
    using A::A;
    A_trampoline(const A& a) : A(a) {}

    void func() override { PYBIND11_OVERLOAD_PURE(void, A, func,); }

    std::shared_ptr<A> clone() const override {
        return myCloneFunction<A, A_trampoline>(this);
    }
};

This would allow the user to customize myCloneFunction. What do you think of this? Do you think that it could work and would be clean?

[lyskov]

I agree that approach [a] seems way to rigid. Ok, lets see if we can make [b] work. A few thoughts on proposed design:

+include_for_trampoline_class A <include_2.hpp>

-- no need to custom include directive, we can re-use include_for_class here: +include_for_class <include_2.hpp>

Generated code:

1. It would be best to allow user to fully rewrite arbitrary trampoline function generation. In order to make this work i think we will have to pass name of the class and name of the function as strings (see Binder generated code here: https://github.com/RosettaCommons/binder/blob/master/test/T10.virtual_inheritance.ref#L27) (I am also considering to pass pointer to parent class member function that we are overriding, it is currently not needed so maybe no for now. But i am curious to hear your thoughts on this). So expected type signature of trampoline function should original_return_type f(string const &, string const &, **original args)
2. I think we also need to declare function specified by trampoline_member_function_binder as class friend (otherwise it will not be able to access protected members)