Override deduced Base class when defining Derived methods #855
Conversation
|
This doesn't appear to work for |
|
Should work for |
include/pybind11/pybind11.h
Outdated
| return def_property(name, | ||
| cpp_function(fget, is_method<type>(*this)), cpp_function(fset, is_method<type>(*this)), | ||
| return_value_policy::reference_internal, extra...); | ||
| } |
There was a problem hiding this comment.
You've ended up with 4 versions of def_property, for the four combination of {cpp_function, generic} for fget and for fset. Possibly the code would be simpler given a helper template function that takes a getter/setter and a (list of) default call policy, and wraps it into a cpp_function with is_method if it isn't already a cpp_function, or returns the provided cpp_function as-is if it is. I think that would allow for only a single version of def_property, and probably also simplifies def_property_readonly.
By the way, is there any particular reason that def and def_static take their function via perfect forwarding while def_property takes it by const reference?
There was a problem hiding this comment.
I know; I don't like it that much. The alternative is adding a move constructor that takes and ignores extra arguments (i.e. template <typename... Extra> cpp_function(cpp_function &&f, const Extra &...)), but I like that approach even less (mainly because it's too late to apply any of the Extra arguments).
There was a problem hiding this comment.
I don't see the extra arguments as that problematic, but I concede that it's a matter of taste.
However, I think you might be able to delete the cpp_function &fget, const Setter &fset overload that you added (L1098) if you change the fully generic overload above to only wrap the setter and forward the getter as-is to be wrapped (if necessary) at the next layer.
There was a problem hiding this comment.
I'm not seeing how: if either of the cpp_function, Setter or Getter, cpp_function versions aren't there, then a call with one cpp_function and one lambda is going to end up at the fully-generic Getter, Setter template, which doesn't handle cpp_function arguments.
There was a problem hiding this comment.
That's why you also need to change the fully-generic template to only wrap the setter. Let's say that c, d are instances of cpp_function, C=cpp_function, G=Getter (template parameter), S=setter (template parameter) and p, q are non-cpp_function getter/setter. Then the chain of calls for each of the cases is
- p, q: <G, S> -> <G, C> -> <C, C>
- c, q: <G, S> -> <C, C>
- p, c: <G, C> -> <C, C>
- c, c: <C, C>
I don't know if that notation makes sense. If I've left you just as unclear, let me know and I can send you a PR against your branch.
| .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) |
There was a problem hiding this comment.
Missing a test for def_property_readonly (I don't think the other tests provide full coverage: def_readonly calls def_property_readonly, but passes it a cpp_function).
|
I'm on the fence here. I think it would be reasonable to expect the user to create a (tiny) binding for the base class rather than making the pybind11 implementation more complex. |
|
My worry is that which class in a hierarchy defines a member function is not normally considered part of an API. The author of the C++ class might push some members into a base class (which may be an implementation detail class) without believing that it will break API compatibility, but it will break the pybind11 bindings (and in a way that is only apparent at runtime with a cryptic error message). It's not entirely related to this PR, but as a workaround for this issue I've written a macro that takes the pain out of wrapping the PTMF: see here. If something like that was added to pybind11 it would at least be less onerous to wrap up every PTMF for robustness. |
|
A simpler approach would be to fix the error message with an error about Right now the biggest issue in call failures is that we have no way to communicate why a call failed. Sometimes you can look at the "invoked with" and see that it doesn't match, but having each overload communicate a failure reason would be pretty nice. |
|
As an example, consider that GCC derives most of its STL classes from base classes with names like More helpful error messages would definitely be better than not having them, but it doesn't address the fragility/portability concerns. |
|
What would actually be the use case of nót having the class itself as first argument of a method? Is there a situation where that's not a mistake? |
|
I've simplified this implementation now by adding a |
jagerman
force-pushed
the
iss854
branch
3 times, most recently
from
7363268
to
73cddbc
Compare
include/pybind11/common.h
Outdated
| /// 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>; |
There was a problem hiding this comment.
Might be clearer/more self-documenting to say ... && !std::is_same<Derived, Base>::value.
include/pybind11/pybind11.h
Outdated
| 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...}; |
There was a problem hiding this comment.
Any particular reason that the specialisations use const & for the extras while the generic implementation uses perfect forwarding?
There was a problem hiding this comment.
There's no advantage to forwarding extras given the way they are used. The generic one uses perfect forwarding because there could be an potential advantage in forwarding a stateful lambda (the first argument), and it's simpler to just forward everything.
|
Hope you don't mind me pushing directly here, but your simplification gave me an idea to take it further still. I wasn't sure about it, had to write it out completely and test it. Looks like it works. Now, |
|
Very nice. 👍 |
jagerman
force-pushed
the
iss854
branch
2 times, most recently
from
218bcb3
to
2f30767
Compare
|
Rebased and squashed. Any other comments before committing? |
include/pybind11/pybind11.h
Outdated
|
|
||
| template <typename Derived, typename Return, typename Class, typename... Args, | ||
| typename Adapted = Return (Derived::*)(Args...)> | ||
| Adapted method_adaptor(Return (Class::*pmf)(Args...)) { return pmf; } |
There was a problem hiding this comment.
I'm not clear why Adapted is a template parameter. It can't be explicitly specified (because Args will swallow all trailing arguments) nor inferred (because it is only used in the return type), so it seems to exist solely to assign a name to the return type. While the C++03 syntax for a function returning a function pointer is horrific, would
template <typename Derived, typename Return, typename Class, typename... Args>
auto method_adaptor(Return (Class::*pmf)(Args...)) -> Return (Derived::*)(Args...) { return pmf; }be clearer? Seems to pass on my system (GCC 5.4), haven't tried running it through Travis and Appveyor or applying it to the const version.
There was a problem hiding this comment.
(LGTM apart from this one comment)
There was a problem hiding this comment.
Adapted is only there to avoid the horror of the regular function pointer return type syntax. But your trailing return type solution looks much nicer!
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 <dean0x7d@gmail.com>
When using `method_adaptor` (usually implicitly via a `cl.def("f",
&D::f)`) a compilation failure results if `f` is actually a method of
an inaccessible base class made public via `using`, such as:
class B { public: void f() {} };
class D : private B { public: using B::f; };
pybind deduces `&D::f` as a `B` member function pointer. Since the base
class is inaccessible, the cast in `method_adaptor` from a base class
member function pointer to derived class member function pointer isn't
valid, and a cast failure results.
This was sort of a regression in 2.2, which introduced `method_adaptor`
to do the expected thing when the base class *is* accessible. It wasn't
actually something that *worked* in 2.1, though: you wouldn't get a
compile-time failure, but the method was not callable (because the `D *`
couldn't be cast to a `B *` because of the access restriction). As a
result, you'd simply get a run-time failure if you ever tried to call
the function (this is what pybind#855 fixed).
Thus the change in 2.2 essentially promoted a run-time failure to a
compile-time failure, so isn't really a regression.
This commit simply adds a `static_assert` with an accessible-base-class
check so that, rather than just a cryptic cast failure, you get
something more informative (along with a suggestion for a workaround).
The workaround is to use a lambda, e.g.:
class Derived : private Base {
public:
using Base::f;
};
// In binding code:
//cl.def("f", &Derived::f); // fails: &Derived::f is actually a base
// class member function pointer
cl.def("f", [](Derived &self) { return self.f(); });
This is a bit of a nuissance (especially if there are a bunch of
arguments to forward), but I don't really see another solution.
Fixes pybind#1124
When using `method_adaptor` (usually implicitly via a `cl.def("f",
&D::f)`) a compilation failure results if `f` is actually a method of
an inaccessible base class made public via `using`, such as:
class B { public: void f() {} };
class D : private B { public: using B::f; };
pybind deduces `&D::f` as a `B` member function pointer. Since the base
class is inaccessible, the cast in `method_adaptor` from a base class
member function pointer to derived class member function pointer isn't
valid, and a cast failure results.
This was sort of a regression in 2.2, which introduced `method_adaptor`
to do the expected thing when the base class *is* accessible. It wasn't
actually something that *worked* in 2.1, though: you wouldn't get a
compile-time failure, but the method was not callable (because the `D *`
couldn't be cast to a `B *` because of the access restriction). As a
result, you'd simply get a run-time failure if you ever tried to call
the function (this is what #855 fixed).
Thus the change in 2.2 essentially promoted a run-time failure to a
compile-time failure, so isn't really a regression.
This commit simply adds a `static_assert` with an accessible-base-class
check so that, rather than just a cryptic cast failure, you get
something more informative (along with a suggestion for a workaround).
The workaround is to use a lambda, e.g.:
class Derived : private Base {
public:
using Base::f;
};
// In binding code:
//cl.def("f", &Derived::f); // fails: &Derived::f is actually a base
// class member function pointer
cl.def("f", [](Derived &self) { return self.f(); });
This is a bit of a nuissance (especially if there are a bunch of
arguments to forward), but I don't really see another solution.
Fixes #1124
When defining method from a member function pointer (e.g.
.def("f", &Derived::f)) we run into a problem if&Derived::fis actually implemented in some base classBasewhenBaseisn'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, notDerived, and so we generate and registered an overload which takes aBase *as first argument. Trying to call this fails ifBaseisn'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.
Fixes #854 , #910