Allow __doc__ to be settable on enums

[AWhetter]

Issue description

Currently when declaring an enum, the __doc__ attribute of the enum is not settable. However the __doc__ on object() is settable so it makes sense for the __doc__ of an enum to be settable also.

The use case where this is coming up:
I'm generating type stubs of a pybind module using stubgen. When running mypy with the generated stubs I see the following error:

Signature of "__doc__" incompatible with supertype of "object".

Potential Fix

I'm not sure what the right fix is here. I can of course make the __doc__ property writable on all enum objects, but implementing that properly would mean adding a new attribute to enum objects and that seems wasteful.

Reproducible example code

# include <pybind11/pybind11.h>

namespace py = pybind11;

enum Numbers {
    zero,
    one
};

PYBIND11_MODULE(numbers, m) {
    py::enum_<Numbers>(m, "Numbers")
        .value("zero", zero)
        .value("one", one)
        .export_values()
    ;
}

[bstaletic]

This is an interesting issue. I tried a few things and I'll quote myself.

py::enum_s are weird...
Even this doesn't help:

PYBIND11_MODULE(numbers, m) {
    py::enum_<Numbers>e(m, "Numbers");
    e
        .value("zero", zero)
        .value("one", one)
        .export_values()
    ;
    e.attr("__doc__") = "test";
}

Result:

>>> import numbers
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: AttributeError: can't set attribute

[sizmailov]

At the moment pybind doesn't allow to overwrite docstrings of functions, methods or class/module attributes:

attribute '__doc__' of 'instancemethod' objects is not writable
attribute '__doc__' of 'builtin_function_or_method' objects is not writable
'Foo' object attribute '__doc__' is read-only

Although class and module __doc__ is writable.

[bstaletic]

Except that enum_ has a def(), which turns it into a class_ and __doc__ is still not assignable.

[sizmailov]

All prohibiting cases boils down to assign-to-property problem. This can be hacked using def_property_readonly_static:

e.def_property_readonly_static("__doc__", [](py::object &) { return "test"; });

[AWhetter]

Do you mean that when pybind is declaring __doc__ on functions, enums, etc it is using def_property_readonly_static(), hence not being writeable?

[sizmailov]

It turns out that overwriting __doc__ in pybind11 (with def_property* methods) doesn't satisfy mypy, it still generates error message. Trailing return type annotation -> Any in property signature triggers mypy error while semantically it's same to not providing return type at all. This looks like mypy bug to me.

class A:
    doc = ""

class B(A):
    # remove "-> Any" to make mypy happy
    @property
    def doc(self) -> Any: ... 

[AWhetter]

I agree. It doesn't seem right that mypy would be unhappy at that.

[sizmailov]

@AWhetter Strictly speaking def_property_readonly_static is not called, but effectively the same (similar) code execucted, so yes.

[sizmailov]

Issue filed on mypy python/mypy#9134.

@AWhetter Do you think there is anything can be done about this issue on pybind11 side?

[AWhetter]

Here's three options I can think of:

1. pybind allows __doc__ to be settable, always. We add a private attribute to enums to store a custom value in and the getter uses that instead of it's usual value when set.

2. pybind allows __doc__ to be settable when asked for via an option. We add a new option that can be passed into py::enum_ that when given will use a different implementation of __doc__ (i.e. what is described above) such that it is writable.

3. pybind allows the implementation of __doc__ to be overridden by a user. pybind would need to expose the default implementation of the __doc__ getter so that a user implementation could call it when creating their own __doc__ property that makes it writable.

It's also worth noting that enums implement __getstate__, so if __doc__ is writable, technically __getstate__ should honour that and return it as part of the state.

[sizmailov]

I've spent some time trying to make docstrings writable (the route 1 in your list). The enums were the easiest part and I came up with this implementation. Setter on first invocation removes property and assigns bare value. This can be improved further to zero or even negative overhead. I think I'll submit a PR next week.

The second route seems to be too complex and non-generic to be part of main repo.

The third option can be done right now on user side, but the syntax is a bit odd:

PYBIND11_MODULE(numbers, m) {
    py::enum_<Numbers> e(m, "Numbers");
    e
        .value("zero", zero)
        .value("one", one)
        .export_values()
    ;
    std::string default_docstring = e.attr("__doc__");
    e.def_property_readonly_static("__doc__", [default_docstring](py::object &self) { return ... ; });
}

technically __getstate__ should honour that and return it as part of the state.

I disagree. __getstate__ has nothing to do with docstrings, "state" tells us the value, not its semantics and completely decoupled from docs.

[YannickJadoul]

It's also worth noting that enums implement __getstate__, so if __doc__ is writable, technically __getstate__ should honour that and return it as part of the state.

I disagree. __getstate__ has nothing to do with docstrings, "state" tells us the value, not its semantics and completely decoupled from docs.

@sizmailov is right: docstrings are set on the type, and have got nothing to do with the state of the instance.

[AWhetter]

Ah you're both right 🤦

[YannickJadoul]

I've been doing some quick digging, and found this:

1. The docstring can be set by passing it to the constructor: py::enum_<Numbers> e(m, "Numbers", "This is a docstring"); adds that to the docstring.
2. The reason you cannot set __doc__ is because pybind11 already sets __doc__ as a read-only property, listing the members and their docstrings (see support docstrings in enum:_:value() #1160).
3. This is why you cán override it with a new def_property_static or def_property_readonly_static, but can nót override it by trying to assign the the attribute (because the descriptor protocol is invoked on the metaclass, which tells you that this static property has no setter).
4. That's also why type.__setattr__(example.Numbers, "__doc__", "abc") works, since it's bypassing pybind11's custom metaclass and actually replacing the custom __setattr__ implementation that enables static classes.

Conclusion: the current issue that doesn't allow __doc__ to be overwritten might not be intentional, but at least stems from an intentional change (i.e., #1160) by @wjakob, so just changing it and expecting a PR to be accepted isn't as straightforward as we would've thought up until now.

Routes forward, ranging from conservative to probably crazy:

1. Accept the current situation, and either pass the docstring to the constructor or workaround it by calling something like PyType_Type.tp_setattro(obj, name, value); to remove the existing __doc__ read-only property. This can be documented in some dark corner of the docs.
2. Add a setter to __doc__ that does some dark magic and maybe removes itself when set. This would keep the current default, but allows you to override __doc__, if you want to get rid of the default behavior that lists enum values. (Bis: if this is technically possible, it could be nice to achieve something that only calculates the current docstring once? Maybe replacing tp_doc the first time it's called?)
3. Make the current behavior (listing the different values of the enum) optional, maybe with an argument to be passed py::enum_values_docstring (either default-on or default-off, to be discussed), or by providing the current static property to be added manually to an enum (i.e., e.def_property_static(py::enum_docstring_property) or so).
4. Give us a superior way to implement static properties. But that won't be so easy, as some smart people already worked on this in the past years.

Meta-conclusion: we need @wjakob's input as to how attached we should be to the old listing-of-enum-values behavior and its associated static read-only property to see a) how much this current is expected by users and b) what's an acceptable solution.

[AWhetter]

Fixed via #2768