Issue45319
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2021-09-29 11:51 by christian.heimes, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Messages (17) | |||
---|---|---|---|
msg402855 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2021-09-29 11:51 | |
While I was working on a patch to port wrapt to limited API and stable ABI, I noticed a possible regression in __annotations__ PyGetSetDef. It looks like C heap type subclasses no longer inherit the __annotations__ descriptor from a C heap type parent class. A gdb session confirmed that 3.10 no longer calls the WraptObjectProxy_set_annotations setter of the parent class while 3.9 does. I had to add { "__annotations__", (getter)WraptObjectProxy_get_annotations, (setter)WraptObjectProxy_set_annotations, 0}, to PyGetSetDef of the child class in order to fix the behavior. Python 3.9 and older work as expected. You can reproduce the behavior by disabling WRAPT_ANNOTATIONS_GETSET_WORKAROUND and run "tox -e py310-install-extensions". The PR is https://github.com/GrahamDumpleton/wrapt/pull/187. |
|||
msg402859 - (view) | Author: Pablo Galindo Salgado (pablogsal) * | Date: 2021-09-29 12:20 | |
Christian, could you bisect using your reproducer? |
|||
msg402861 - (view) | Author: Pablo Galindo Salgado (pablogsal) * | Date: 2021-09-29 12:26 | |
If you want a potential fix to be included in 3.10.0 it must be commited in the next 24/48 hours, otherwise it will need to wait for 3.10.1 |
|||
msg402862 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2021-09-29 12:35 | |
I'm bisecting now. |
|||
msg402865 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2021-09-29 12:49 | |
The culprit is commit 2f2b69855d6524e15d12c15ddc0adce629e7de84 for bpo-43901: Lazy-create an empty annotations dict in all unannotated user classes and modules (GH-25623). |
|||
msg402870 - (view) | Author: Pablo Galindo Salgado (pablogsal) * | Date: 2021-09-29 13:21 | |
Ah, that is likely because type_getsets now gets directly an __annotations__ field. |
|||
msg402882 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2021-09-29 15:02 | |
Larry, could you please take a look? Your commit is causing a regression for C subclasses of heap types when the parent class has an "__annotations__" descriptor. The child class no longer uses the descriptor of the parent class. |
|||
msg405328 - (view) | Author: Larry Hastings (larry) * | Date: 2021-10-29 18:41 | |
I finally have some bandwidth to look at this--sorry for being a bit slow. I wasn't able to reproduce, because the patch didn't apply cleanly. I downloaded the patch ( https://patch-diff.githubusercontent.com/raw/GrahamDumpleton/wrapt/pull/187.patch ) and ran against current wrapt main, and about 50% of the "hunks" failed to apply. |
|||
msg405446 - (view) | Author: Larry Hastings (larry) * | Date: 2021-11-01 15:49 | |
I got the PR locally, but the command-line you gave me fails, tox can't find python3.10 despite it being on the path. Rather than force me to reverse-engineer and recreate your build environment, can you give me a straightforward command-line or shell script to reproduce the problem? |
|||
msg405447 - (view) | Author: Larry Hastings (larry) * | Date: 2021-11-01 15:49 | |
(Preferably not using "tox". I don't know that tool.) |
|||
msg405448 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2021-11-01 16:06 | |
I created a new branch in my clone for you that has the 3.10 workaround disabled. # checkout: git clone -b bpo45319 https://github.com/tiran/wrapt.git cd wrapt # build src/wrapt/_wrappers.abi3.so python3 setup.py build_ext -i # run tests $ PYTHONPATH=src python3.9 tests/test_update_attributes.py .......... ---------------------------------------------------------------------- Ran 10 tests in 0.001s OK $ PYTHONPATH=src python3.10 tests/test_update_attributes.py FF........ ====================================================================== FAIL: test_update_annotations (__main__.TestUpdateAttributes) ---------------------------------------------------------------------- Traceback (most recent call last): File "/tmp/wrapt/tests/test_update_attributes.py", line 149, in test_update_annotations self.assertEqual(function.__wrapped__.__annotations__, override_annotations) AssertionError: {} != {'override_annotations': ''} - {} + {'override_annotations': ''} ====================================================================== FAIL: test_update_annotations_modified_on_original (__main__.TestUpdateAttributes) ---------------------------------------------------------------------- Traceback (most recent call last): File "/tmp/wrapt/tests/test_update_attributes.py", line 173, in test_update_annotations_modified_on_original self.assertEqual(function.__annotations__, override_annotations) AssertionError: {} != {'override_annotations': ''} - {} + {'override_annotations': ''} ---------------------------------------------------------------------- Ran 10 tests in 0.001s FAILED (failures=2) |
|||
msg414826 - (view) | Author: Larry Hastings (larry) * | Date: 2022-03-10 04:58 | |
This isn't a CPython bug. It's a change in CPython behavior that wrapt needs to accommodate. In particular, it isn't 'causing a regression for C subclasses of heap types when the parent class has an "__annotations__" descriptor', nor do child classes have any difficulty inheriting the descriptor of their parent classes. That's unsurprising; after all, if I had broken child classes inheriting the descriptors of their parent classes, a lot more would have broken than just wrapt. The problem is in WraptObjectProxy_setattro(). (Which--just to drive my point home--*is* getting called when you set __annotations__ on one of wrapt's various proxy objects.) WraptObjectProxy_setattro() proxies setattr calls for wrapped objects to the original object--if "o" is a wrapt proxy object wrapping "fn", and you run "o.__annotations__ = x", it should actually execute "fn.__annotations__ = x" under the covers. Except WraptObjectProxy_setattro() executes *this* code first, starting at line 1531 in my copy of _wrapped.c: if (PyObject_HasAttr((PyObject *)Py_TYPE(self), name)) return PyObject_GenericSetAttr((PyObject *)self, name, value); If the *type* has the attribute, then it doesn't proxy the setattr to the wrapped object. Instead it does a "generic setattr" on the object itself. PyObject_HasAttr works by attempting a getattr on the type. If that getattr call succeeds, PyObject_HasAttr returns true. The type here is FunctionWrapper (WraptFunctionWrapper_Type). Since we're now looking it up on this type object, we use the type of the type object, which is "type", to access the attribute. And getting the "__annotations__" attribute from an object of type "type" means calling type_get_annotations(), a new descriptor which ensures that the annotations dict always exists, which means the HasAttr call succeeds and returns true. In short, this change to the semantics of the "__annotations__" attribute means wrapt no longer proxies the setattr to the underlying wrapped object when setting the "__annotations__" attribute on *any* of its objects. In my opinion, wrapt needs to accommodate this new behavior. In my testing I changed the above code to this: if (!annotations_str) { annotations_str = PyUnicode_InternFromString("__annotations__"); } if (PyObject_RichCompareBool(name, annotations_str, Py_NE) && PyObject_HasAttr((PyObject *)Py_TYPE(self), name)) return PyObject_GenericSetAttr((PyObject *)self, name, value); I also declared static PyObject *annotations_str = NULL; at the top of the function. With that change in place, the tests now passed. My hunch is, this approach is more or less what wrapt should do. It *might* be undersophisticated; it's possible that there are classes out there playing their own weird descriptor tricks with the "__annotations__" attribute. Perhaps the fix needs to be on a case-by-case basis, based on the type of the wrapped object. Anyway this is obviously up to Graham, which is for the best anyway--he has far more experience than I do with this sort of object proxying wizardry. |
|||
msg414827 - (view) | Author: Larry Hastings (larry) * | Date: 2022-03-10 05:12 | |
(It's also possible your workaround where wrapt explicitly defines an "__annotations__" getset on every child of ObjectProxy is the right fix. I don't know; I don't know the fine points of attribute access, like when does it access getsets on the type, vs getsets on base types, vs setattro, etc.) |
|||
msg414828 - (view) | Author: Graham Dumpleton (grahamd) | Date: 2022-03-10 05:30 | |
I don't know about the comment "he has far more experience than I do with this sort of object proxying wizardry". I read what you said and my brain melted. I am getting too old for this and trying to understand how anything works anymore takes me ages. :-) Anyway, will try and digest what you said a half dozen more times when my brain is working again and see if I can make sense of it. |
|||
msg414829 - (view) | Author: Larry Hastings (larry) * | Date: 2022-03-10 06:14 | |
Graham: I'm happy to help. I can write a more elaborate explanation of what's happening, or answer questions, whatever you like. I'm a fan of wrapt so I'd certainly like to see this issue resolved. And, seeing as you're the author and maintainer of wrapt, I assure you you've got more battle-won experience with object proxying than most developers--myself included. You're *absolutely* the right person to ameliorate this issue! |
|||
msg414831 - (view) | Author: Graham Dumpleton (grahamd) | Date: 2022-03-10 08:23 | |
Let me try and summarise what I do understand about this. The existing wrapt code as written and its tests, work fine for Python 2.7 and 3.6-3.11. No special case handling is done in tests related to checking annotations that I can remember. The only reason this issue came up is because Christian tried to convert wrapt C code to only use the limited API and stable ABI, and as a result the existing test suite then started to fail for newer versions of Python (3.10+) on tests related to annotations, because the way the limited API and stable ABI for Python 3.10+ didn't behave the same as it did in older versions. So if wrapt doesn't change to use the limited API and stable ABI then wrapt doesn't need to make any changes as it all seems to work fine when using the older APIs. For the time being therefore at least it seems wrapt doesn't need to make any changes, since switching to the limited API and stable ABI is not a confirmed direction yet, and can't be done anyway until at least Python 2.7 support is dropped, and perhaps some Python 3.X version support as well. |
|||
msg414832 - (view) | Author: Larry Hastings (larry) * | Date: 2022-03-10 08:40 | |
I only did my experiments with the _wrappers.c Christian gave me. If that's not the shipping version of wrapt, and wrapt doesn't exhibit this behavior in 3.10, then that's good. I agree you can wait to address this behavior until it affects code you actually plan to ship. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:50 | admin | set | github: 89482 |
2022-03-10 08:40:26 | larry | set | messages: + msg414832 |
2022-03-10 08:23:16 | grahamd | set | messages: + msg414831 |
2022-03-10 06:14:14 | larry | set | messages: + msg414829 |
2022-03-10 05:30:54 | grahamd | set | messages: + msg414828 |
2022-03-10 05:12:06 | larry | set | messages: + msg414827 |
2022-03-10 04:58:11 | larry | set | status: open -> closed resolution: third party messages: + msg414826 stage: test needed -> resolved |
2021-11-08 16:54:37 | vstinner | set | nosy:
- vstinner |
2021-11-01 16:06:30 | christian.heimes | set | messages: + msg405448 |
2021-11-01 15:49:49 | larry | set | messages: + msg405447 |
2021-11-01 15:49:18 | larry | set | messages: + msg405446 |
2021-10-29 18:41:17 | larry | set | messages: + msg405328 |
2021-09-29 15:02:53 | christian.heimes | set | nosy:
+ larry messages: + msg402882 |
2021-09-29 13:21:46 | pablogsal | set | messages: + msg402870 |
2021-09-29 12:49:05 | christian.heimes | set | messages: + msg402865 |
2021-09-29 12:46:43 | corona10 | set | nosy:
+ corona10 |
2021-09-29 12:35:16 | christian.heimes | set | nosy:
+ vstinner, petr.viktorin messages: + msg402862 |
2021-09-29 12:26:32 | pablogsal | set | nosy:
- vstinner, petr.viktorin messages: + msg402861 |
2021-09-29 12:25:39 | vstinner | set | nosy:
+ vstinner, petr.viktorin |
2021-09-29 12:20:28 | pablogsal | set | messages: + msg402859 |
2021-09-29 12:13:30 | grahamd | set | nosy:
+ grahamd |
2021-09-29 11:51:39 | christian.heimes | create |