Simplify implementation

This simplifies the PR implementation by adding a `method_adaptor`
function which returns a cpp_function wrapped around a lambda with the
argument type switched to the requested type if given a member function
pointer; otherwise the adaptor just forwards the argument to
a cpp_function constructor.

Author: jagerman

include/pybind11/attr.h

@@ -18,15 +18,7 @@ NAMESPACE_BEGIN(pybind11)
 /// @{
 
 /// Annotation for methods
-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>;
-};
+struct is_method { handle class_; is_method(const handle &c) : class_(c) { } };
 
 /// Annotation for operators
 struct is_operator { };
@@ -329,8 +321,8 @@ template <> struct process_attribute<sibling> : process_attribute_default<siblin
 };
 
 /// Process an attribute which indicates that this function is a method
-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_; }
+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_; }
 };
 
 /// Process an attribute which indicates the parent scope of a method
@@ -470,7 +462,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(is_instantiation<is_method, Extra>::value...)>
+          size_t self  = constexpr_sum(std::is_same<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/common.h

@@ -582,6 +582,11 @@ using exactly_one_t = typename exactly_one<Predicate, Default, Ts...>::type;
 template <typename T, typename... /*Us*/> struct deferred_type { using type = T; };
 template <typename T, typename... Us> using deferred_t = typename deferred_type<T, Us...>::type;
 
+/// Like is_base_of, but requires a strict base (i.e. `is_strict_base_of<T, T>::value == false`,
+/// unlike `std::is_base_of`)
+template <typename Base, typename Derived> using is_strict_base_of = bool_constant<
+    std::is_base_of<Base, Derived>::value && !std::is_base_of<Derived, Base>::value>;
+
 template <template<typename...> class Base>
 struct is_template_base_of_impl {
     template <typename... Us> static std::true_type check(Base<Us...> *);

include/pybind11/pybind11.h

@@ -44,8 +44,6 @@ 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() { }
 
@@ -68,22 +66,17 @@ class cpp_function : public function {
                    (FuncType *) nullptr, extra...);
     }
 
-    /// 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) {
-        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...);
+        initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(args...); },
+                   (Return (*) (Class *, 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) {
-        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...);
+        initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(args...); },
+                   (Return (*)(const Class *, Arg ...)) nullptr, extra...);
     }
 
     /// Return the function name
@@ -912,13 +905,40 @@ void call_operator_delete(T *p) { T::operator delete(p); }
 
 inline void call_operator_delete(void *p) { ::operator delete(p); }
 
+// Wrapper around cpp_function that binds a method pointer of a base class with a derived class
+// argument, as opposed to calling cpp_function directly which always uses the inferred class.  For
+// any other type of argument, this is identical to cpp_function.
+template <typename Derived, typename Func, typename SFINAE = void> struct method_adaptor_impl {
+    template <typename... Args> static cpp_function adapt(Args &&... args) { return {std::forward<Args>(args)...}; }
+};
+template <typename Derived, typename Return, typename Class, typename... Arg>
+struct method_adaptor_impl<Derived, Return (Class::*)(Arg...), enable_if_t<is_strict_base_of<Class, Derived>::value>> {
+    template <typename... Extra> static cpp_function adapt(Return (Class::*f)(Arg...), const Extra&... extra) {
+        return {[f](Derived *c, Arg... args) -> Return { return (c->*f)(args...); }, extra...};
+    }
+};
+template <typename Derived, typename Return, typename Class, typename... Arg>
+struct method_adaptor_impl<Derived, Return (Class::*)(Arg...) const, enable_if_t<is_strict_base_of<Class, Derived>::value>> {
+    template <typename... Extra> static cpp_function adapt(Return (Class::*f)(Arg...) const, const Extra&... extra) {
+        return {[f](const Derived *c, Arg... args) -> Return { return (c->*f)(args...); }, extra...};
+    }
+};
+
 NAMESPACE_END(detail)
 
+/// Returns a cpp_function around the given method or function; if given a method pointer to a base
+/// class of Derived, this results in a lambda that takes a `Derived *` as first argument, rather
+/// than the usual deduced `Base *` type.
+template <typename Derived, typename Func, typename... Extra> cpp_function method_adaptor(Func &&f, const Extra&... extra) {
+    return detail::method_adaptor_impl<Derived, detail::remove_cv_t<detail::remove_reference_t<Func>>>::adapt(
+            std::forward<Func>(f), extra...);
+}
+
 template <typename type_, typename... options>
 class class_ : public detail::generic_type {
     template <typename T> using is_holder = detail::is_holder_type<type_, T>;
-    template <typename T> using is_subtype = detail::bool_constant<std::is_base_of<type_, T>::value && !std::is_same<T, type_>::value>;
-    template <typename T> using is_base = detail::bool_constant<std::is_base_of<T, type_>::value && !std::is_same<T, type_>::value>;
+    template <typename T> using is_subtype = detail::is_strict_base_of<type_, T>;
+    template <typename T> using is_base = detail::is_strict_base_of<T, type_>;
     // struct instead of using here to help MSVC:
     template <typename T> struct is_valid_class_option :
         detail::any_of<is_holder<T>, is_subtype<T>, is_base<T>> {};
@@ -984,8 +1004,9 @@ 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<type>(*this),
-                        sibling(getattr(*this, name_, none())), extra...);
+        auto cf = method_adaptor<type>(
+                std::forward<Func>(f), name(name_), is_method(*this),
+                sibling(getattr(*this, name_, none())), extra...);
         attr(cf.name()) = cf;
         return *this;
     }
@@ -1048,18 +1069,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) {
-        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...);
+        static_assert(std::is_base_of<C, type>::value, "def_readwrite() requires a class member (or base class member)");
+        cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)),
+                     fset([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this));
+        def_property(name, fget, fset, return_value_policy::reference_internal, extra...);
         return *this;
     }
 
     template <typename C, typename D, typename... Extra>
     class_ &def_readonly(const char *name, const D C::*pm, const Extra& ...extra) {
-        using BindC = typename is_method<type>::template BindClass<C>;
-        def_property_readonly(name, [pm](const BindC &c) -> const D &{ return c.*pm; }, extra...);
+        static_assert(std::is_base_of<C, type>::value, "def_readonly() requires a class member (or base class member)");
+        cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this));
+        def_property_readonly(name, fget, return_value_policy::reference_internal, extra...);
         return *this;
     }
 
@@ -1081,7 +1102,8 @@ 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, is_method<type>(*this)), return_value_policy::reference_internal, extra...);
+        return def_property_readonly(name, method_adaptor<type>(fget),
+                return_value_policy::reference_internal, extra...);
     }
 
     /// Uses cpp_function's return_value_policy by default
@@ -1105,21 +1127,17 @@ 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...);
+        return def_property(name, fget, method_adaptor<type>(fset, 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, is_method<type>(*this), return_value_policy::reference_internal),
-                fset, extra...);
+        return def_property(name, method_adaptor<type>(fget), fset, return_value_policy::reference_internal, 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<type>(*this), extra...);
+        return def_property_static(name, fget, fset, is_method(*this), extra...);
     }
 
     /// Uses return_value_policy::reference by default

tests/test_methods_and_attributes.cpp

@@ -170,7 +170,7 @@ 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
+// Issues #854, #910: incompatible function args when member function/pointer is in unregistered base class
 class UnregisteredBase {
 public:
     void do_nothing() const {}
@@ -361,8 +361,8 @@ 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
+    // Issues #854/910: 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")
@@ -371,6 +371,9 @@ test_initializer methods_and_attributes([](py::module &m) {
         .def("increase_value", &RegisteredDerived::increase_value)
         .def_readwrite("rw_value", &RegisteredDerived::rw_value)
         .def_readonly("ro_value", &RegisteredDerived::ro_value)
+        // These should trigger a static_assert if uncommented
+        //.def_readwrite("fails", &SimpleValue::value) // should trigger a static_assert if uncommented
+        //.def_readonly("fails", &SimpleValue::value) // should trigger a static_assert if uncommented
         .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: