GeezerVison is showing github.com — `property` assumes that its subclasses have __dict__ · Issue #98963 · python/cpython
property assumes that its subclasses have __dict__ #98963
Descriptionproperty'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?
Activityrhettinger commented on Nov 2, 2022 Recommend ignoring the AttributeError. Loss of a __doc__ in this situation is of no consequence. encukou commented on Nov 2, 2022 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”? Reacted by Erlend E. Aasland wjakob commented on Nov 2, 2022 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 commented on Nov 2, 2022
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 commented on Nov 2, 2022 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 commented on Nov 3, 2022
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? Reacted by Erlend E. Aasland and C.A.M. Gerlach wjakob commented on Nov 3, 2022 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.. :) Reacted by Erlend E. Aasland encukou commented on Nov 3, 2022 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
Reacted by Gregory P. Smith wjakob commented on Nov 3, 2022 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 commented on Nov 3, 2022 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. gpshead commented on Jun 2, 2023 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 commented on Jun 2, 2023 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 commented on Jun 3, 2023
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 commented on Jun 5, 2023 closing as the 3.12 PR is set to automerge; previous exception / ignored error behavior has been restored. gpshead commented on Jun 7, 2023 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 commented on Jun 7, 2023 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 commented on Jun 12, 2023 Thank you for taking this on! wjakob commented on Jun 12, 2023 A thanks from me as well. With your change, I was able to remove the Python 3.12-specific workaround/hack from nanobind. Reacted by Gregory P. Smith
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|