nanobind enum design flawed (mutation can break things globally)

[wjakob]

The nanobind way of binding enums is based on nb::class_ for simplicity. It internally creates singletons of every enum member. However, this leads the following issue when calling a function with a reference argument, and when that function mutates this argument

m.def("mutate_enum", [](Color &value) { value = Color::Red; });

Calling this from Python with Color.Black causes the value of the singleton to change to Color.Red, which can cause problems globally and in a way that's difficult to track back to the origin.

I see two solutions

• We change the implementation so that it makes copies.
• We switch the implementation over to enum.Enum / enum.IntEnum which had been requested a few times. In this case, conversion would work via type casting, which also breaks the mutate_enum example above (but in a better way -- here, the reference update just does nothing, which is at least consistent with how type casting works overall).

Either option (but especially #2) would be a quite disruptive change, so I'm opening this ticket for discussion.

cc @oremanj, @rzhikharevich

[oremanj]

Seems like there's an intermediate option here, where we define a type caster for any std::is_enum_v that's based on type_caster_base but adds:

template <typename T> using Cast = Type;
operator Type() const { return *value; }

to prevent taking a pointer or mutable reference to an enum. (The usual type_caster strategy of saving the enum value in a member of the type_caster object would also work, but I think is unnecessary here, since we want to encourage people to take the enum by value to avoid confusion about why mutating it doesn't work.)

Probably with some additional bit of wrapping so from_python adds cast_flags::disallow_none and from_cpp forces the RVP to copy.

[oremanj]

I'd be concerned about the performance impacts of using literally Enum/IntEnum; since they're implemented in Python I would expect quite a bit more overhead in both directions of conversion, but maybe it's not as bad as I'm imagining.

[rzhikharevich]

As I understand, regular integers are not mutable by references like in the example, so maybe enums should behave similarly?

[hpkfft]

For what it's worth, I have two different uses for enums in my project. In both cases, I do not want the enum to be compared to an integer.

In the first use case, the enum values are just an unordered list of things. This, I think, is similar to the Color.red example. It's best if my users cannot compare Color.red to 3. Also, it's meaningless to compare Color.red > Color.blue. The only useful comparisons are == and != between two Colors. I have implemented this using nb::type_slots() with entry Py_tp_richcompare. This does Py_RETURN_NOTIMPLEMENTED; if the two Py_TYPE() are different. Also, it is nice that is_arithmetic is an annotation that can be left off.

The second use case is for flags. Each enum value indicates a platform capability and is a power of two. For example, half_precision may be represented by the integer value 16, single_precision by 32, etc. The flags are orthogonal. A given hardware platform may support one, or the other, or both, or neither. The enum is a bit set. In this case, >= is meaningful to test whether a capability is present: if (my_capabilities >= Capability.half_precison) ..... Again, I use nb::type_slots(). For this enum use, it might be nice to have some new annotation is_logical which enables & , |, ^, ~ but does not enable the arithmetic +, -, *, //, <<, >>, unary -, abs.

[wjakob]

I'd be concerned about the performance impacts of using literally Enum/IntEnum; since they're implemented in Python I would expect quite a bit more overhead in both directions of conversion, but maybe it's not as bad as I'm imagining.

I pushed an experiental PR (#533) that replaces nanobind's homegrown enumeration types with the builtin enum.Enum and enum.IntEnum. It uses a pair of hash tables on the nanobind side to accelerate lookups. This change removes quite a bit of code from the project-- more so if #492 is considered as well. This turned out to be surprisingly feasible, and I am considering to include it as a breaking change in nanobind v2.0.0.

Thoughts? Can you give it a try and let me know what you think?

[wjakob]

closed via #533