Skip to content

Add informative compilation failure for method_adaptor failures #1127

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

Merged
merged 1 commit into from
Oct 12, 2017

Conversation

jagerman
Copy link
Member

@jagerman jagerman commented Oct 5, 2017

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 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
@wjakob
Copy link
Member

wjakob commented Oct 9, 2017

This looks like a very reasonable change. The problem with using above is indeed a bit of a nuisance, but I think it's clear together with the error message.

@jagerman jagerman merged commit 7672292 into pybind:master Oct 12, 2017
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

Successfully merging this pull request may close these issues.

2.2 regression: can not bind member function with lifted access
2 participants