Override deduced Base class when defining Derived methods

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, 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 extends the pybind11::is_method annotation into a templated
annotation containing the class being registered, which we can then
extract to override the first argument to the derived type when
attempting to register a base class method for a derived class.

This also slightly simplifies def_readwrite/def_readonly, which can rely
on def_property to wrap their lambdas into cpp_functions with the
appropriate rvp.

Author: jagerman

include/pybind11/attr.h

@@ -18,7 +18,15 @@ NAMESPACE_BEGIN(pybind11)
 /// @{
 
 /// Annotation for methods
-struct is_method { handle class_; is_method(const handle &c) : class_(c) { } };
+template <typename CppClass>
+struct is_method {
+    handle class_;
+    is_method(const handle &c) : class_(c) { }
+    using Class = CppClass;
+    template <typename DeducedClass>
+    using BindClass = detail::conditional_t<std::is_base_of<DeducedClass, Class>::value,
+        Class, DeducedClass>;
+};
 
 /// Annotation for operators
 struct is_operator { };
@@ -321,8 +329,8 @@ template <> struct process_attribute<sibling> : process_attribute_default<siblin
 };
 
 /// Process an attribute which indicates that this function is a method
-template <> struct process_attribute<is_method> : process_attribute_default<is_method> {
-    static void init(const is_method &s, function_record *r) { r->is_method = true; r->scope = s.class_; }
+template <typename Class> struct process_attribute<is_method<Class>> : process_attribute_default<is_method<Class>> {
+    static void init(const is_method<Class> &s, function_record *r) { r->is_method = true; r->scope = s.class_; }
 };
 
 /// Process an attribute which indicates the parent scope of a method
@@ -462,7 +470,7 @@ using extract_guard_t = typename exactly_one_t<is_call_guard, call_guard<>, Extr
 /// Check the number of named arguments at compile time
 template <typename... Extra,
           size_t named = constexpr_sum(std::is_base_of<arg, Extra>::value...),
-          size_t self  = constexpr_sum(std::is_same<is_method, Extra>::value...)>
+          size_t self  = constexpr_sum(is_instantiation<is_method, Extra>::value...)>
 constexpr bool expected_num_args(size_t nargs, bool has_args, bool has_kwargs) {
     return named == 0 || (self + named + has_args + has_kwargs) == nargs;
 }

include/pybind11/pybind11.h

@@ -44,6 +44,8 @@ NAMESPACE_BEGIN(pybind11)
 
 /// Wraps an arbitrary C++ function/method/lambda function/.. into a callable Python object
 class cpp_function : public function {
+private:
+    template <typename Extra> using is_method_annotation = detail::is_instantiation<is_method, Extra>;
 public:
     cpp_function() { }
 
@@ -69,15 +71,19 @@ class cpp_function : public function {
     /// Construct a cpp_function from a class method (non-const)
     template <typename Return, typename Class, typename... Arg, typename... Extra>
     cpp_function(Return (Class::*f)(Arg...), const Extra&... extra) {
-        initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(args...); },
-                   (Return (*) (Class *, Arg...)) nullptr, extra...);
+        using ClassArg = typename detail::exactly_one_t<is_method_annotation, is_method<Class>, Extra...>
+            ::template BindClass<Class>;
+        initialize([f](ClassArg *c, Arg... args) -> Return { return (c->*f)(args...); },
+                   (Return (*) (ClassArg *, Arg...)) nullptr, extra...);
     }
 
     /// Construct a cpp_function from a class method (const)
     template <typename Return, typename Class, typename... Arg, typename... Extra>
     cpp_function(Return (Class::*f)(Arg...) const, const Extra&... extra) {
-        initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(args...); },
-                   (Return (*)(const Class *, Arg ...)) nullptr, extra...);
+        using ClassArg = typename detail::exactly_one_t<is_method_annotation, is_method<Class>, Extra...>
+            ::template BindClass<Class>;
+        initialize([f](const ClassArg *c, Arg... args) -> Return { return (c->*f)(args...); },
+                   (Return (*)(const ClassArg *, Arg ...)) nullptr, extra...);
     }
 
     /// Return the function name
@@ -978,7 +984,7 @@ class class_ : public detail::generic_type {
 
     template <typename Func, typename... Extra>
     class_ &def(const char *name_, Func&& f, const Extra&... extra) {
-        cpp_function cf(std::forward<Func>(f), name(name_), is_method(*this),
+        cpp_function cf(std::forward<Func>(f), name(name_), is_method<type>(*this),
                         sibling(getattr(*this, name_, none())), extra...);
         attr(cf.name()) = cf;
         return *this;
@@ -1042,16 +1048,18 @@ class class_ : public detail::generic_type {
 
     template <typename C, typename D, typename... Extra>
     class_ &def_readwrite(const char *name, D C::*pm, const Extra&... extra) {
-        cpp_function fget([pm](const C &c) -> const D &{ return c.*pm; }, is_method(*this)),
-                     fset([pm](C &c, const D &value) { c.*pm = value; }, is_method(*this));
-        def_property(name, fget, fset, return_value_policy::reference_internal, extra...);
+        using BindC = typename is_method<type>::template BindClass<C>;
+        def_property(name,
+            [pm](const BindC &c) -> const D &{ return c.*pm; },
+            [pm](BindC &c, const D &value) { c.*pm = value; },
+            extra...);
         return *this;
     }
 
     template <typename C, typename D, typename... Extra>
     class_ &def_readonly(const char *name, const D C::*pm, const Extra& ...extra) {
-        cpp_function fget([pm](const C &c) -> const D &{ return c.*pm; }, is_method(*this));
-        def_property_readonly(name, fget, return_value_policy::reference_internal, extra...);
+        using BindC = typename is_method<type>::template BindClass<C>;
+        def_property_readonly(name, [pm](const BindC &c) -> const D &{ return c.*pm; }, extra...);
         return *this;
     }
 
@@ -1073,7 +1081,7 @@ class class_ : public detail::generic_type {
     /// Uses return_value_policy::reference_internal by default
     template <typename Getter, typename... Extra>
     class_ &def_property_readonly(const char *name, const Getter &fget, const Extra& ...extra) {
-        return def_property_readonly(name, cpp_function(fget), return_value_policy::reference_internal, extra...);
+        return def_property_readonly(name, cpp_function(fget, is_method<type>(*this)), return_value_policy::reference_internal, extra...);
     }
 
     /// Uses cpp_function's return_value_policy by default
@@ -1095,15 +1103,23 @@ class class_ : public detail::generic_type {
     }
 
     /// Uses return_value_policy::reference_internal by default
+    template <typename Getter, typename Setter, typename... Extra>
+    class_ &def_property(const char *name, const Getter &fget, const Setter &fset, const Extra& ...extra) {
+        return def_property(name, fget,
+                cpp_function(fset, is_method<type>(*this), return_value_policy::reference_internal),
+                extra...);
+    }
     template <typename Getter, typename... Extra>
     class_ &def_property(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) {
-        return def_property(name, cpp_function(fget), fset, return_value_policy::reference_internal, extra...);
+        return def_property(name,
+                cpp_function(fget, is_method<type>(*this), return_value_policy::reference_internal),
+                fset, extra...);
     }
 
     /// Uses cpp_function's return_value_policy by default
     template <typename... Extra>
     class_ &def_property(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) {
-        return def_property_static(name, fget, fset, is_method(*this), extra...);
+        return def_property_static(name, fget, fset, is_method<type>(*this), extra...);
     }
 
     /// Uses return_value_policy::reference by default

tests/test_methods_and_attributes.cpp

@@ -159,7 +159,7 @@ template <> struct type_caster<ArgAlwaysConverts> {
 };
 }}
 
-/// Issue/PR #648: bad arg default debugging output
+// Issue/PR #648: bad arg default debugging output
 class NotRegistered {};
 
 // Test None-allowed py::arg argument policy
@@ -170,6 +170,23 @@ int none3(std::shared_ptr<NoneTester> &obj) { return obj ? obj->answer : -1; }
 int none4(std::shared_ptr<NoneTester> *obj) { return obj && *obj ? (*obj)->answer : -1; }
 int none5(std::shared_ptr<NoneTester> obj) { return obj ? obj->answer : -1; }
 
+// Issue #854: incompatible function args when member function/pointer is in unregistered base class
+class UnregisteredBase {
+public:
+    void do_nothing() const {}
+    void increase_value() { rw_value++; ro_value += 0.25; }
+    void set_int(int v) { rw_value = v; }
+    int get_int() const { return rw_value; }
+    double get_double() const { return ro_value; }
+    int rw_value = 42;
+    double ro_value = 1.25;
+};
+class RegisteredDerived : public UnregisteredBase {
+public:
+    using UnregisteredBase::UnregisteredBase;
+    double sum() const { return rw_value + ro_value; }
+};
+
 test_initializer methods_and_attributes([](py::module &m) {
     py::class_<ExampleMandA> emna(m, "ExampleMandA");
     emna.def(py::init<>())
@@ -316,7 +333,7 @@ test_initializer methods_and_attributes([](py::module &m) {
     m.def("floats_preferred", [](double f) { return 0.5 * f; }, py::arg("f"));
     m.def("floats_only", [](double f) { return 0.5 * f; }, py::arg("f").noconvert());
 
-    /// Issue/PR #648: bad arg default debugging output
+    // Issue/PR #648: bad arg default debugging output
 #if !defined(NDEBUG)
     m.attr("debug_enabled") = true;
 #else
@@ -344,4 +361,19 @@ test_initializer methods_and_attributes([](py::module &m) {
     m.def("ok_none4", &none4, py::arg().none(true));
     m.def("ok_none5", &none5);
 
+    // Issue #854: incompatible function args when member function/pointer is in unregistered base class
+    // The methods and member pointers below actually resolve to members/pointers in
+    // UnregisteredBase; before this test/fix they would be registered via lambda with a first
+    // argument of an unregistered type, and thus uncallable.
+    py::class_<RegisteredDerived>(m, "RegisteredDerived")
+        .def(py::init<>())
+        .def("do_nothing", &RegisteredDerived::do_nothing)
+        .def("increase_value", &RegisteredDerived::increase_value)
+        .def_readwrite("rw_value", &RegisteredDerived::rw_value)
+        .def_readonly("ro_value", &RegisteredDerived::ro_value)
+        .def_property("rw_value_prop", &RegisteredDerived::get_int, &RegisteredDerived::set_int)
+        .def_property_readonly("ro_value_prop", &RegisteredDerived::get_double)
+        // This one is in the registered class:
+        .def("sum", &RegisteredDerived::sum)
+        ;
 });

tests/test_methods_and_attributes.py

@@ -413,3 +413,23 @@ def test_accepts_none():
     assert ok_none3(None) == -1
     assert ok_none4(None) == -1
     assert ok_none5(None) == -1
+
+
+def test_unregistered_base_implementations():
+    from pybind11_tests import RegisteredDerived
+
+    a = RegisteredDerived()
+    a.do_nothing()
+    assert a.rw_value == 42
+    assert a.ro_value == 1.25
+    a.rw_value += 5
+    assert a.sum() == 48.25
+    a.increase_value()
+    assert a.rw_value == 48
+    assert a.ro_value == 1.5
+    assert a.sum() == 49.5
+    assert a.rw_value_prop == 48
+    a.rw_value_prop += 1
+    assert a.rw_value_prop == 49
+    a.increase_value()
+    assert a.ro_value_prop == 1.75