Revising the design of type_caster<T>::load()

[dean0x7d]

Currently, type casters defined using PYBIND11_TYPE_CASTER require that the C++ type is default-constructible and copy/move-assignable. Apart from the fact that not all types fulfill these requirements, doing default init + assign is less efficient than directly constructing an object (both binary size and runtime overhead). There are also possible complications as in the case of #850 where an additional helper struct and specialization are required but could be avoided with a different load() design.

The proposal

The idea is to make type_caster<T>::load() a static function which returns an optional-like container. Looking only at the load-specific parts, a type caster currently looks like this:

struct caster {
    T value;

    operator type*();
    operator type&();
    template <typename U> using cast_op_type;

    bool load(handle src, bool convert);
}

This proposal changes it to:

struct caster {
    static maybe<T> load(handle src, bool convert);
}

Where maybe<T, ExtractionPolicy> is an optional-like container with some simplifications and specializations for the purpose of pybind11's casters (named maybe to avoid confusion with the standard optional). Here, the ExtractionPolicy template parameter takes on the job of cast_op_type and the cast operators. The default policy is the same as defined by PYBIND11_TYPE_CASTER which makes maybe<T> the only thing needed for user-defined casters. This also drastically reduces the footprint of the PYBIND11_TYPE_CASTER macro.

This would also simplify usage:

// currently
U value;

bool load(handle src, bool convert) {
    make_caster<T> subcaster;
    if (subcaster.load(src, convert)) {
        value = cast_op<T>(subcaster);
        return true;
    }
    return false;
}

// proposed
static maybe<U> load(handle src, bool convert) {
    if (auto maybe_value = make_caster<T>::load(src, convert))
        return maybe_value.extract<T>();
    return {};
}

The behavior of maybe::extract<T>() is determined by the ExtractionPolicy. Given that these containers have a very special purpose and are only used once, the default policy could move when T is a value or rvalue ref (to simplify caster code).

Backward compatibility

Existing user-defined casters would still function either by inserting a compatibility layer into PYBIND11_TYPE_CASTER or by using SFINAE in pybind11 internals (only if compatibility is required for user-defined casters which do not use the PYBIND11_TYPE_CASTER macro).

If this whole proposal sounds good, this might also be a good time to consider moving user-defined casters out of the detail namespace.

PR

I have a rough working prototype and I can turn this into a PR if the proposal sounds acceptable. I think that reducing the footprint of the PYBIND11_TYPE_CASTER macro and lifting the default constructor + assignment requirements would be a nice improvement.

[jagerman]

A few comments (for now):

• It seems like it's going to be a fair amount of work, and going to have some backwards compatibility concerns. Have you considered an alternative of using the maybe<T> as an implementation detail of the type caster, i.e. so that instead of declaring T value; you declare maybe<T> value;? That avoids the default construction, but requires a less drastic change. I'm not opposed to the more drastic change, but just throwing this out there.

• the backwards compatibility issue might be addressed with the move to pybind11::caster namespace: Keep a type_caster<T, SFINAE> in detail, but make the base case a special has-no-caster object. Then the new base template in caster can, if a old-style detail caster exists, load it through some compatibility object that converts it via construct-and-load into a maybe<T, appropriate_policy>.

• One thing that would be nice for type casters is to be able to return more than just yes/no: to return no with a failure message. So, for example, suppose I have two Eigen-accepting functions, f(Eigen::Vector3d), f(Eigen::Matrix3d), and I try to call f() with a 2x2 numpy array. Right now, I get:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: f(): incompatible function arguments. The following argument types are supported:
    1. (arg0: numpy.ndarray[float64[3, 3]]) -> None
    2. (arg0: numpy.ndarray[float64[3, 1]]) -> None

Invoked with: array([[ 1.,  2.],
       [ 3.,  4.]])

but it would be very nifty to be able to get something like:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: f(): incompatible function arguments. The following argument types are supported:
    1. (arg0: numpy.ndarray[float64[3, 3]]) -> None
       - arg0: incompatible matrix dimensions: expected 3x3, received 2x2
    2. (arg0: numpy.ndarray[float64[3, 1]]) -> None
       - arg0: incompatible matrix dimensions: expected 3x1, received 2x2

Invoked with: array([[ 1.,  2.],
       [ 3.,  4.]])

With maybe<T> this sort of thing could be done easily: it could have success, fail, and fail-with-error-string states. Arguments that fail without a message could get a generic "- argn: incompatible argument" message. (Another way I thought about doing this would be through throw a special exception object, but that would have the significant disadvantage of only being able to show one argument failure per call, even if multiple occurred).

[aldanor]

If this whole proposal sounds good, this might also be a good time to consider moving user-defined casters out of the detail namespace.

+1 for making type casters "public", I've written a fair share of custom type casters and it always feels a bit wrong to specialize things in pybind11::detail namespace in downstream code. Plus, doing this provides an option to deprecate the old detail type casters more gracefully at the same time.

---

A bit of bikeshedding:

static maybe<U> load(handle src, bool convert) {
    if (auto maybe_value = make_caster<T>::load(src, convert))
        return maybe_value.extract<T>();
    return {};
}

could even be

static maybe<U> load(handle src, bool convert) {
    return make_caster<T>::load(src, convert).try_extract<T>();
}

if it's a common pattern; or something like that (also try_cast, etc).

---

Another way I thought about doing this would be through throw a special exception object, but that would have the significant disadvantage of only being able to show one argument failure per call, even if multiple occurred

And an additional disadvantage of being potentially slower, depending on the exception model...

[jagerman]

One thing that #916 exposes is that a maybe<T> needs to have a dependency for casters like Eigen::Ref. More details in #916, but the short version is that the Eigen::Ref caster sometimes keeps a temporary py::array alive via a caster member when a temporary copy is needed, but the stl.h casters don't keep the subcasters alive, so we end up with an Eigen::Ref that references the data of a destroyed py::array.

[wjakob]

This is a sensible proposal, and I'm open to a significant departure from the current API (which works, but is by no means elegant). One important aspect that hasn't been mentioned above is that the new interface and maybe<> should allow compact code generation (RVO, etc.) since type casters make up a significant amount of the object code in bindings.

[jagerman]

Another couple of things to think about on this topic:

• It would be nice to be able to override the built-in casters with a custom type caster. For example, someone asked in the gitter channel about binding code that expects std::string in a non-UTF-8 encoding. Since 2.1, this is actually possible: you can declare a template <> type_caster<std::string>{ ... } which will get precedence (as a full specialiation) over the built-in string caster (a std::basic_string partial specialization). It might be possible to take this a step further by having something like: template <Type, SFINAE = void> class custom_caster : public builtin_caster<Type> {}, so that custom casters can indeed override builtin casters.

• load() would be nicer if it accepted an load_options struct rather than convert; the struct would itself contain convert, but could also contain other values. This would make adding features into the cast/load system much easier in the future (existing casters would not need any changes, while maintaining the same API). For example, the py::arg().none() feature could be extended to allow custom casters to optionally accept None through this. cast(), to be consistent, could similarly wrap the current return_value_policy and parent arguments into a cast_options struct.