`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.