Implicit conversion from Python string to C++ enum type

[stukowski]

Let me propose an extension:
Sometimes it would be nice if a string could be passed from a Python script to a C++ function where an enum parameter is expected. For example:

struct Pet {
    enum Kind {
        Dog = 0,
        Cat
    };
    Pet(Kind type) : type(type) { }
    Kind type;
};

Binding code:

auto pet = py::class_<Pet>(m, "Pet")
    .def(py::init<Pet::Kind>());

py::enum_<Pet::Kind>(pet, "Kind")
    .value("Dog", Pet::Kind::Dog)
    .value("Cat", Pet::Kind::Cat);

Python:

>>> p = Pet(Pet.Cat) # Works
>>> p = Pet("Cat")   # Would like to make this work too

In principle it is possible to add optional support for this implicit conversion from a Python string to the enum type. I propose to add the following method to the pybind11::enum_ class:

template <typename Type> class enum_ : public class_<Type> {
    ...

    /// Set up atomatic Python string -> C++ enum conversion.
    enum_& enable_conversion_from_string() {
        auto entries = m_entries;
	def("__init__", [entries](Type& v, const char* str) {
            for(const auto& item : *entries) {
                if(!strcmp(str, item.second)) {
                    v = (Type)item.first;
                    return;
                }
            }
            std::string tname = typeid(Type).name();
            detail::clean_type_id(tname);
            throw value_error("\"" + std::string(str) + "\" is not a valid value for enum type " + tname);
        });
	implicitly_convertible<str, Type>();
        return *this;
    }
    ...
}

This would allow writing binding codes like this:

py::enum_<Pet::Kind>(pet, "Kind")
    .value("Dog", Pet::Kind::Dog)
    .value("Cat", Pet::Kind::Cat)
    .enable_conversion_from_string();

There is only one problem with this solution: The exception thrown by the conversion constructor when the string does not match any of the enum entries is swallowed by pybind11. Instead, a general cast error is raised because the conversion from string to the enum type failed.

I am wondering what you think about this idea. Does it make sense to offer an automatic conversion like this? At least in my project there is one situation where an automatic conversion from a string is desirable.

By the way: The pybind11::enum_ class constructor registers two __init__ methods: One initializing the value via assignment, the other via in-place new. I think one of them is redundant. Furthermore, I noticed that the std::unordered_map allocated by the constructor is never freed. Does it make sense to use a shared_ptr<unordered_map> here so that the map gets destroyed eventually?

[jagerman]

The second __init__ indeed looks weird; and moreover I don't think it'll ever actually be invoked. It was added in 6fb4849 along with other changes to fix a problem, but I don't think the second constructor was actually the fix here. (I also don't think the second __init__ will ever be called, since pybind looks for things to call based on their signatures, and these have identical signatures).

The non-deleted pointer is intentional: registration of classes/enums is permanent, and that pointer needs to be alive (because __repr__ uses it) as long as the enum is registered, i.e. forever. The enum_ instance itself, however, only persists during the initialization code: but its effects, which depend on that unordered map, remain forever.

[jagerman]

One other suggestion: the use of const char* as the value type in the map in the current code (and even more so in your code) seems to be complicating the code; changing it to std::string would be a decent cleanup. That would change __repr__ to:

        def("__repr__", [name, entries](Type value) -> std::string {
            auto it = entries->find((UnderlyingType) value);
            return name + (it == entries->end() ? std::string(".???") : "." + it->second);
        });

and would change your code to take a std::string, then use regular == comparison instead of the strcmp.

Another minor comment:

std::string tname = typeid(Type).name();
detail::clean_type_id(tname);
throw value_error("\"" + std::string(str) + "\" is not a valid value for enum type " + tname);

the get-name-and-clean-it is already available via pybind11::type_id<Type>(), so, with the change of str to std::string, you can replace all of the above with:

throw value_error("\"" + str + "\" is not a valid value for enum type " + type_id<T>());

Edit: actually, type_id<T>() isn't really what you want here: you actually want the name of the Python type, so you'd need to store the name given to the constructor in a private member, then use a capture-by-value of it in your added lambda.

[jagerman]

(I also suggest you code this up, add some tests for it, and submit a PR--it'll make it a bit easier to discuss the code and see it in action).

[wjakob]

all agreed -- please resubmit the modified version as a PR, and we can take it from there. I will close this ticket since this is a feature request and not a pybind11 issue.

[stukowski]

Thank you all for the suggestions. I'll submit a PR as soon as I find time to code this up.

As I already mentioned, I don't know yet how to generate a meaningful error from this conversion constructor. pybind11 swallows any exception thrown by the constructor function and generates a general casting error message instead. So the nice error message generated by my function never makes it to the Python caller. If you have any idea how to inform the user that his input string did not match any of the registered enum values, let me know.

@jagerman: My comment on the non-deleted map pointer was based on my (wrong) assumption that the enum_ instance (and with it all lamdas it has registered) would get destroyed on Py_Finalize(). I now learned that this is not the case and the memory leak is intentional. Of course, there is not point in using a shared_ptr then.

[wjakob]

pybind11 swallows any exception thrown by the constructor function and generates a general casting error message instead.

This sounds quite mysterious to me. Why don't you submit your PR with that issue still present, and we can take a look?

[YannickJadoul]

Has there been any progress on this pull request? If @stukowski is not planning to make one, I might try to implement the proposed solution such that I'm able to use it in my project.

[TD22057]

+1 - I'd really like this feature as well. Our current python code generator does this and it's really useful. I can't find an open or closed PR for this anywhere.

[ezyang]

Using #1176 I was able to achieve this effect by defining a type_caster directly on the enum type while inheriting from type_caster_base. Example:

template <>
struct type_caster<c10::DispatchKey> : public type_caster_base<c10::DispatchKey> {
  using base = type_caster_base<c10::DispatchKey>;
  c10::DispatchKey tmp;
 public:
  bool load(handle src, bool convert) {
    if (base::load(src, convert)) {
      return true;
    } else if (py::isinstance(src, py::module_::import("builtins").attr("str"))) {
      tmp = c10::parseDispatchKey(py::cast<std::string>(src));
      value = &tmp;
      return true;
    } 
    return false;
  }   

  static handle cast(
      c10::DispatchKey src,
      return_value_policy policy,
      handle parent) {
    return base::cast(src, policy, parent);
  } 
};

Unfortunately, you will have to somehow implement the string parsing yourself. I had a helper C++ function which I used.

I'm not entirely sure this doesn't leak, as I'm doing a dynamic allocation in order to get a value of the correct form.

EDIT: Unfortunately this leaks memory

EDIT 2: Updated with a non-leaking version, though IDK how kosher it is. I know that converters have some safety to avoid returning temporaries so hoping this is kosher.