Skip to content

Problems wrapping member function defined in base class #854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bmerry opened this issue May 16, 2017 · 3 comments
Closed

Problems wrapping member function defined in base class #854

bmerry opened this issue May 16, 2017 · 3 comments

Comments

@bmerry
Copy link
Contributor

bmerry commented May 16, 2017

Issue description

If a member function being wrapped by py::class_::def is actually implemented in a base class, then calling the member fails.

In the example code, it gives

Traceback (most recent call last):
  File "./testit.py", line 6, in <module>
    b.do_nothing()
TypeError: do_nothing(): incompatible function arguments. The following argument types are supported:
    1. (self: A) -> None

Invoked with: <pbtest._pbtest.B object at 0x7f1a691c3810>

This is presumably because the type of &B::do_nothing is actually void (A::*)() rather than void (B::*)(). While it can be fixed by explicitly casting it, it seems like it should be possible for pybind11 to fix this itself, since it knows at compile time which class is being wrapped.

Reproducible example code

C++:

#include <pybind11/pybind11.h>

namespace py = pybind11;

class A
{
public:
    void do_nothing() {}
};

class B : public A
{
public:
    using A::A;
};

PYBIND11_PLUGIN(_pbtest)
{
    py::module m("_pbtest");

    py::class_<B>(m, "B")
        .def(py::init<>())
        .def("do_nothing", &B::do_nothing);

    return m.ptr();
}

Python:

from pbtest._pbtest import B

b = B()
b.do_nothing()
@bmerry
Copy link
Contributor Author

bmerry commented May 16, 2017

One fix might to be an overload py::class_::def that takes pointers-to-members, then casts them to fix the referring class. One question I don't have a strong opinion on is what should happen if one passes a pointer-to-member of an unrelated class for which there is an implicit conversion: should it be a compile error or should *this be converted to the other type before invoking the pointer to member? At the moment the latter seems to happen.

@bmerry
Copy link
Contributor Author

bmerry commented May 16, 2017

The issue also seem to affect pointers to members defined by def_readonly and def_readwrite.

@jagerman
Copy link
Member

#855 should address this. I didn't do anything about non-base-class member function pointers; I don't see that they hurt anything, even if they are a bit obscure.

jagerman added a commit to jagerman/pybind11 that referenced this issue Jun 28, 2017
When defining method from a member function pointer (e.g. `.def("f",
&Derived::f)`) we run into a problem if `&Derived::f` is actually
implemented in some base class `Base` when `Base` isn't
pybind-registered.

This happens because the class type is deduced from the member function
pointer, which then becomes a lambda with first argument this deduced
type.  For a base class implementation, the deduced type is `Base`, not
`Derived`, and so we generate and registered an overload which takes a
`Base *` as first argument.  Trying to call this fails if `Base` isn't
registered (e.g.  because it's an implementation detail class that isn't
intended to be exposed to Python) because the type caster for an
unregistered type always fails.

This commit adds a `method_adaptor` function that rebinds a member
function to a derived type member function and otherwise (i.e. regular
functions/lambda) leaves the argument as-is.  This is now used for class
definitions so that they are bound with type being registered rather
than a potential base type.

A closely related fix in this commit is to similarly update the lambdas
used for `def_readwrite` (and related) to bind to the class type being
registered rather than the deduced type so that registering a property
that resolves to a base class member similarly generates a usable
function.

Fixes pybind#854, pybind#910.

Co-Authored-By: Dean Moldovan <[email protected]>
jagerman added a commit to jagerman/pybind11 that referenced this issue Jun 28, 2017
When defining method from a member function pointer (e.g. `.def("f",
&Derived::f)`) we run into a problem if `&Derived::f` is actually
implemented in some base class `Base` when `Base` isn't
pybind-registered.

This happens because the class type is deduced from the member function
pointer, which then becomes a lambda with first argument this deduced
type.  For a base class implementation, the deduced type is `Base`, not
`Derived`, and so we generate and registered an overload which takes a
`Base *` as first argument.  Trying to call this fails if `Base` isn't
registered (e.g.  because it's an implementation detail class that isn't
intended to be exposed to Python) because the type caster for an
unregistered type always fails.

This commit adds a `method_adaptor` function that rebinds a member
function to a derived type member function and otherwise (i.e. regular
functions/lambda) leaves the argument as-is.  This is now used for class
definitions so that they are bound with type being registered rather
than a potential base type.

A closely related fix in this commit is to similarly update the lambdas
used for `def_readwrite` (and related) to bind to the class type being
registered rather than the deduced type so that registering a property
that resolves to a base class member similarly generates a usable
function.

Fixes pybind#854, pybind#910.

Co-Authored-By: Dean Moldovan <[email protected]>
jagerman added a commit to jagerman/pybind11 that referenced this issue Jul 3, 2017
When defining method from a member function pointer (e.g. `.def("f",
&Derived::f)`) we run into a problem if `&Derived::f` is actually
implemented in some base class `Base` when `Base` isn't
pybind-registered.

This happens because the class type is deduced from the member function
pointer, which then becomes a lambda with first argument this deduced
type.  For a base class implementation, the deduced type is `Base`, not
`Derived`, and so we generate and registered an overload which takes a
`Base *` as first argument.  Trying to call this fails if `Base` isn't
registered (e.g.  because it's an implementation detail class that isn't
intended to be exposed to Python) because the type caster for an
unregistered type always fails.

This commit adds a `method_adaptor` function that rebinds a member
function to a derived type member function and otherwise (i.e. regular
functions/lambda) leaves the argument as-is.  This is now used for class
definitions so that they are bound with type being registered rather
than a potential base type.

A closely related fix in this commit is to similarly update the lambdas
used for `def_readwrite` (and related) to bind to the class type being
registered rather than the deduced type so that registering a property
that resolves to a base class member similarly generates a usable
function.

Fixes pybind#854, pybind#910.

Co-Authored-By: Dean Moldovan <[email protected]>
jagerman added a commit to jagerman/pybind11 that referenced this issue Jul 3, 2017
When defining method from a member function pointer (e.g. `.def("f",
&Derived::f)`) we run into a problem if `&Derived::f` is actually
implemented in some base class `Base` when `Base` isn't
pybind-registered.

This happens because the class type is deduced from the member function
pointer, which then becomes a lambda with first argument this deduced
type.  For a base class implementation, the deduced type is `Base`, not
`Derived`, and so we generate and registered an overload which takes a
`Base *` as first argument.  Trying to call this fails if `Base` isn't
registered (e.g.  because it's an implementation detail class that isn't
intended to be exposed to Python) because the type caster for an
unregistered type always fails.

This commit adds a `method_adaptor` function that rebinds a member
function to a derived type member function and otherwise (i.e. regular
functions/lambda) leaves the argument as-is.  This is now used for class
definitions so that they are bound with type being registered rather
than a potential base type.

A closely related fix in this commit is to similarly update the lambdas
used for `def_readwrite` (and related) to bind to the class type being
registered rather than the deduced type so that registering a property
that resolves to a base class member similarly generates a usable
function.

Fixes pybind#854, pybind#910.

Co-Authored-By: Dean Moldovan <[email protected]>
jagerman added a commit that referenced this issue Jul 3, 2017
When defining method from a member function pointer (e.g. `.def("f",
&Derived::f)`) we run into a problem if `&Derived::f` is actually
implemented in some base class `Base` when `Base` isn't
pybind-registered.

This happens because the class type is deduced from the member function
pointer, which then becomes a lambda with first argument this deduced
type.  For a base class implementation, the deduced type is `Base`, not
`Derived`, and so we generate and registered an overload which takes a
`Base *` as first argument.  Trying to call this fails if `Base` isn't
registered (e.g.  because it's an implementation detail class that isn't
intended to be exposed to Python) because the type caster for an
unregistered type always fails.

This commit adds a `method_adaptor` function that rebinds a member
function to a derived type member function and otherwise (i.e. regular
functions/lambda) leaves the argument as-is.  This is now used for class
definitions so that they are bound with type being registered rather
than a potential base type.

A closely related fix in this commit is to similarly update the lambdas
used for `def_readwrite` (and related) to bind to the class type being
registered rather than the deduced type so that registering a property
that resolves to a base class member similarly generates a usable
function.

Fixes #854, #910.

Co-Authored-By: Dean Moldovan <[email protected]>
bmerry added a commit to ska-sa/spead2 that referenced this issue Oct 9, 2017
It's no longer needed now that the vendored pybind11 has been updated to
2.2.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants