Simplify implementation

This simplifies the PR implementation by adding a `static cpp_function
method()` to `cpp_function` that takes care of generating the
derived-type lambdas when given a base pointer function (and otherwise
just forwards arguments to the 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

@@ -511,6 +511,11 @@ template <typename T> struct remove_class { };
 template <typename C, typename R, typename... A> struct remove_class<R (C::*)(A...)> { typedef R type(A...); };
 template <typename C, typename R, typename... A> struct remove_class<R (C::*)(A...) const> { typedef R type(A...); };
 
+/// Extract the class from a method type
+template <typename T> struct extract_class { };
+template <typename C, typename R, typename... A> struct extract_class<R (C::*)(A...)> { typedef C type; };
+template <typename C, typename R, typename... A> struct extract_class<R (C::*)(A...) const> { typedef C type; };
+
 /// Helper template to strip away type modifiers
 template <typename T> struct intrinsic_type                       { typedef T type; };
 template <typename T> struct intrinsic_type<const T>              { typedef typename intrinsic_type<T>::type type; };
@@ -582,6 +587,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,27 +66,42 @@ class cpp_function : public function {
                    (FuncType *) nullptr, extra...);
     }
 
-    /// Construct a cpp_function from a class method (non-const)
+    /// Construct a cpp_function from a class method (non-const); the exposed class argument type is
+    /// given by BindClass.
     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
     object name() const { return attr("__name__"); }
 
+    /// Used to construct a cpp_function that might be a method of a base type; in such a case, this
+    /// wraps the method in a lambda accepting the given type (rather than the derived type) as the
+    /// `self` argument.  Otherwise this simply forwards to the constructors above.
+    template <typename Derived, typename Func, typename... Extra>
+    static cpp_function method(Func &&f, const Extra&... extra) {
+        return {std::forward<Func>(f), extra...};
+    }
+    template <typename Derived, typename Return, typename Class, typename... Arg, typename... Extra,
+              detail::enable_if_t<detail::is_strict_base_of<Class, Derived>::value, int> = 0>
+    static cpp_function method(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, typename... Extra,
+              detail::enable_if_t<detail::is_strict_base_of<Class, Derived>::value, int> = 0>
+    static cpp_function method(Return (Class::*f)(Arg...) const, const Extra&... extra) {
+        return {[f](const Derived *c, Arg... args) -> Return { return (c->*f)(args...); }, extra...};
+    }
+
 protected:
     /// Space optimization: don't inline this frequently instantiated fragment
     PYBIND11_NOINLINE detail::function_record *make_function_record() {
@@ -917,8 +930,8 @@ NAMESPACE_END(detail)
 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 +997,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 = cpp_function::method<type>(
+                std::forward<Func>(f), name(name_), is_method(*this),
+                sibling(getattr(*this, name_, none())), extra...);
         attr(cf.name()) = cf;
         return *this;
     }
@@ -1048,18 +1062,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 +1095,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, cpp_function::method<type>(fget),
+                return_value_policy::reference_internal, extra...);
     }
 
     /// Uses cpp_function's return_value_policy by default
@@ -1105,21 +1120,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, cpp_function::method<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, cpp_function::method<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

@@ -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: