property assumes that its subclasses have __dict__

[encukou]

property's tp_init has this code:

    if (Py_IS_TYPE(self, &PyProperty_Type)) {
        Py_XSETREF(self->prop_doc, prop_doc);
    } else {
        /* If this is a property subclass, put __doc__
           in dict of the subclass instance instead,
           otherwise it gets shadowed by __doc__ in the
           class's dict. */

        /*...*/
         PyObject_SetAttr((PyObject *)self, &_Py_ID(__doc__), prop_doc);
        /*...*/
    }

This assumes that subclasses of property have a __dict__ (or a __doc__ attribute settable by other means). That is the case when subclassed using the class statement, but might not be true using C.
A C-API reproducer is at https://github.com/wjakob/inheritance_issue/blob/master/inheritance_issue.c

I don't see a good way to fix this. We could:

• document that subclasses need to set Py_TPFLAGS_MANAGED_DICT (or provide a __doc__ descriptor), or
• ignore the AttributeError, unless __doc__ was set explicitly?

• PR: gh-98963: Add a note to the error for property subclasses without __doc__ #99058

• PR: gh-98963: Restore the ability to have a dict-less property. #105262

• PR: [3.12] gh-98963: Restore the ability to have a dict-less property. (GH-105262) #105297

[rhettinger]

Recommend ignoring the AttributeError. Loss of a __doc__ in this situation is of no consequence.

[encukou]

Thinking about it more, I came to the opposite conclusion.

Ignoring AttributeError moves the surprising behavior elsewhere – now people would wonder why doc isn't set as requested. (Worse, if the property subclass is defined in a library, it's likely that a user would bump into this, rather than the library author.)

The main issue here is that the error is surprising – it's not clear what's happening and what the right course of action is. If you're subclassing property in the C API, adding a __doc__ descriptor (or adding Py_TPFLAGS_MANAGED_DICT/tp_dictoffset) shouldn't be an issue – if you know that's what's necessary. (If losing __doc__ is OK, the descriptor can even ignore the data and always get None. Making it explicit sounds better in this case.)
So, another option is to just raise a better error – “property subclasses require a writable doc instance attribute”?

[wjakob]

I also don't think that losing the __doc__ silently is a good behavior.

Would it not be an option to relax the condition at the top? Instead of Py_IS_TYPE(self, &PyProperty_Type), the test could spend some more time to see when it is safe to use PyObject_SetAttr. AFAIK that would be

• If the type has an associated dictionary
• If somewhere in the type hierarchy, we expose a writable __doc__ field via tp_members

[encukou]

If somewhere in the type hierarchy, we expose a writable __doc__ field via tp_members

property does that. But Python doesn't consider the superclass's descriptor (even when setting it) when there's a class attribute of the same name on the subclass. Here, that's the subclass's own docstring (None by default).

To make __doc__ work, it needs to either be a descriptor on the subclass, or an attribute on the instance.

[rhettinger]

FWIW,. this code has been stable for 20 years. AFAICT, the only motivation for an edit is to accommodate single, new, and somewhat exotic C extension. So any proposed change should be minimally invasive.

Thinking about it more, I came to the opposite conclusion.

It doesn't feel like my assessment is being valued.

[encukou]

any proposed change should be minimally invasive.

I definitely agree with that! I hope to only touch the error case.

AFAICT, the only motivation for an edit is to accommodate single, new, and somewhat exotic C extension.

My opinion here is more nuanced though. The C API is not used only with C, and I've been trying to make it more useful with other languages (C++, Rust, Java). Language bindings work at a lower level than typical C extensions, so they run into subtle bugs we haven't seen before.
So, yes, it's a somewhat exotic C extension – but if we fix the issues it encounters, we'll hopefully get more extensions like it, and it'll become less exotic.

It doesn't feel like my assessment is being valued.

I'm sorry I made you feel this way. I tried to react to you and explain my reasoning. How can I do better?

[wjakob]

For what it's worth, I understand @rhettinger's concern that this is somewhat of a niche issue. While making Python behave more as expected is desirable, I would already be very happy simply with guidance on how to work around this issue in a stable manner (meaning that it works for past Python versions, and that I can have some reasonable assurance it won't break in the future).

Alas, that requirement of stability is what is making things very difficult:

• @encukou suggested that the subclass should expose a writeable __doc__ member. However, the behavior of such a combination is unstable across Python versions. A minimal C-API reproducer can be found here: https://github.com/wjakob/inheritance_issue_2/blob/master/inheritance_issue_2.c

  To try:

  $ pip install git+https://github.com/wjakob/inheritance_issue_2

  In Python 3.10

  >>> import inheritance_issue_2
  >>> print(inheritance_issue_2.my_property(lambda x: None, doc='test').__doc__)
  None

  In Python 3.12 (main)

  >>> import inheritance_issue_2
  >>> print(inheritance_issue_2.my_property(lambda x: None, doc='test').__doc__)
  test

  So the point by @rhettinger (this code has been stable for 20 years) does not seem to hold 100%. Something has subtly changed.

• Py_TPFLAGS_MANAGED_DICT suggested by @encukou is not in the limited API, which I require at least as an optional build configuration. So that one is out.

• Installing an unmanaged dictionary via __dictoffset__ is unstable across Python versions. A minimal C-API reproducer can be found here: https://github.com/wjakob/inheritance_issue_3/blob/master/inheritance_issue_3.c

  To try:

  $ pip install git+https://github.com/wjakob/inheritance_issue_3

  In Python 3.10

  >>> import inheritance_issue_3
  >>> print(inheritance_issue_3.my_property(lambda x: None, doc='test').__doc__)
  None

  In Python 3.12 (main)

  >>> import inheritance_issue_3
  >>> print(inheritance_issue_3.my_property(lambda x: None, doc='test').__doc__)
  test

• The only thing I have discovered so far which consistently works across Python versions is to set the new property type's tp_members pointer to PyProperty_Type.tp_members. This can be done even in the limited API via PyType_GetSlot, but as @encukou pointed out this makes too many assumptions about the underlying implementation. So although it happens to compile and work with -DPy_LIMITED_API right now, it is not future-proof.

If you have advice on other things to try, I am all ears.. :)

[encukou]

Aha! It turns out that this was not stable -- in fact this was just changed for 3.12: #23205
(If anything, this underscores the point that touching old code should be done extremely carefully.)

@wjakob: There's even some pybind11 fallout: pybind/pybind11#4168

Given that silencing the AttributeError would essentially revert this case to pre-3.12 behavior, it would make sense. (Edit: no, the pre-3.12 behavior was more complicated: the error was raised or not depending on where the docstring came from.)
But, before I wrote this I already wrote a patch for improving the error message, so I'll post it here: #99058

[wjakob]

Aha, it's great to know that v3.12 is the place where this change happened. Incidentally, this is also the first Python version for which -DPy_LIMITED_API will be complete enough for my project.

So what I am thinking to do now as a workaround is:

• For Python < 3.12, set the new property's tp_members field to PyProperty_Type.tp_members.

• For Python >= 3.12 and in particular limited API mode, declare an independent tp_members field that adds a __doc__ field. (That seems more economical than a full-blown dictionary)

The error message in PR #99058 looks good to me.

[encukou]

Actually, the Python 3.12 change was for the doc argument – when the docstring is being copied from a setter function, instantiation does fail as far back as Python 2. Pre-3.12, the doc argument doesn't work on subclasses at all.

[wjakob]

Are you referring to the doc argument of the constructor? This evening's latest version of nanobind passes it to a subclass of property using the two different strategies mentioned in my last message (implementation: wjakob/nanobind@e82f14a). It passes the test suite on Python 3.8-3.12, which checks that the docstring is set correctly.

The docstring is specified directly to the property constructor in this case, instead of being copied from a setter function.

Note: this commit does not yet use the features from your tentative PEP (negative basicsize etc, that is all in a different branch).

[gpshead]

This came up in protobuf because it has a

class _FieldProperty(property):
  __slots__ = ("DESCRIPTOR",)

calling property.__init__() on such a class instance fails with an exception, no matter what args are passed, doc or not:

    property.__init__(self, getter, setter, doc=doc)
AttributeError: '_FieldProperty' object attribute '__doc__' is read-only

If "__doc__" is added to the __slots__ tuple, it works again.

In older versions of Python the the property constructor silently does not assign a docstring rather than causing an AttributeError.

[gpshead]

That is narrower than the larger issue here, but I think we should mirror that silent doc-dropping behavior for 3.12 and consider if we're deprecating that behavior later or just want to make it official.

[gpshead]

That is narrower than the larger issue here

re-reading things, I don't think that is true. This issue title talks about a dict, but the issue is entirely about a the docstring being unsettable and whether that is ignored as it always has been or is an expected behavior changing error.

Existing code depends on it being ignored.

[gpshead]

closing as the 3.12 PR is set to automerge; previous exception / ignored error behavior has been restored.

[gpshead]

Related to this in general... CPython <=3.11 has some seriously smelly behavior:

Python 3.10.9 (main, Dec  7 2022, 13:47:07) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class slotted_property(property):
...   __slots__ = ()
... 
>>> slotted_property()
<__main__.slotted_property object at 0x7f36e189dad0>
>>> slotted_property(lambda x: None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'slotted_property' object attribute '__doc__' is read-only
>>> slotted_property(lambda x: None, doc='ignored').__doc__
>>> slotted_property(lambda x: None, doc='').__doc__
>>> slotted_property(lambda x: None, doc=None).__doc__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'slotted_property' object attribute '__doc__' is read-only
>>>

[gpshead]

Those surprisingly inconsistent AttributeErrors are gone in 3.12. If we ever decide to bring an exception back in this case, we'll want to be fully consistent about it.

[encukou]

Thank you for taking this on!

[wjakob]

A thanks from me as well. With your change, I was able to remove the Python 3.12-specific workaround/hack from nanobind.