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.

classification
Title: Possible regression in __annotations__ descr for heap type subclasses
Type: behavior Stage: resolved
Components: C API Versions: Python 3.11, Python 3.10
process
Status: closed Resolution: third party
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, corona10, grahamd, larry, pablogsal, petr.viktorin
Priority: normal Keywords: 3.10regression

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) * (Python committer) 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) * (Python committer) Date: 2021-09-29 12:20
Christian, could you bisect using your reproducer?
msg402861 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) 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) * (Python committer) Date: 2021-09-29 12:35
I'm bisecting now.
msg402865 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2021-11-01 15:49
(Preferably not using "tox".  I don't know that tool.)
msg405448 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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:50adminsetgithub: 89482
2022-03-10 08:40:26larrysetmessages: + msg414832
2022-03-10 08:23:16grahamdsetmessages: + msg414831
2022-03-10 06:14:14larrysetmessages: + msg414829
2022-03-10 05:30:54grahamdsetmessages: + msg414828
2022-03-10 05:12:06larrysetmessages: + msg414827
2022-03-10 04:58:11larrysetstatus: open -> closed
resolution: third party
messages: + msg414826

stage: test needed -> resolved
2021-11-08 16:54:37vstinnersetnosy: - vstinner
2021-11-01 16:06:30christian.heimessetmessages: + msg405448
2021-11-01 15:49:49larrysetmessages: + msg405447
2021-11-01 15:49:18larrysetmessages: + msg405446
2021-10-29 18:41:17larrysetmessages: + msg405328
2021-09-29 15:02:53christian.heimessetnosy: + larry
messages: + msg402882
2021-09-29 13:21:46pablogsalsetmessages: + msg402870
2021-09-29 12:49:05christian.heimessetmessages: + msg402865
2021-09-29 12:46:43corona10setnosy: + corona10
2021-09-29 12:35:16christian.heimessetnosy: + vstinner, petr.viktorin
messages: + msg402862
2021-09-29 12:26:32pablogsalsetnosy: - vstinner, petr.viktorin
messages: + msg402861
2021-09-29 12:25:39vstinnersetnosy: + vstinner, petr.viktorin
2021-09-29 12:20:28pablogsalsetmessages: + msg402859
2021-09-29 12:13:30grahamdsetnosy: + grahamd
2021-09-29 11:51:39christian.heimescreate