Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate the consequences of PEP-563 #15

Open
sallner opened this issue Jun 28, 2018 · 16 comments · May be fixed by #24
Open

Investigate the consequences of PEP-563 #15

sallner opened this issue Jun 28, 2018 · 16 comments · May be fixed by #24

Comments

@sallner
Copy link
Member

sallner commented Jun 28, 2018

In PEP-563 a system for storing type hints is proposed and the implementation started in Python 3.7. The type hints are stored in __annotations__. It would be helpful to see, whether this could effect somehow the working principle of zope.annotation.

@jamadden
Copy link
Member

I've looked at this a little. __annotations__ was introduced way back in PEP-3107 for Python 3. Functions and methods grew the possibility to have an __annotations__ attribute:

Python 3.4.8 (default, Mar 29 2018, 16:18:25)
[GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
...     def thing(a:int) -> int:
...         return 1
...
>>> Foo.thing.__annotations__
{'a': <class 'int'>, 'return': <class 'int'>}

Python 3.6 accepted PEP-563 which let you put annotations on raw variables. Prior to that, only function/method arguments and return types could be annotated AIUI, and I can't find a way to get __annotations__ on a class. But with 3.6, you can now have __annotations__ showing up at class scope:

Python 3.6.2 (v3.6.2:5fd33b5926, Jul 16 2017, 20:11:06)
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
...     a:int
...
>>> Foo.__annotations__
{'a': <class 'int'>}

PEP 563 only changes the values stored in the annotations dictionary into strings always. It's not enabled by default in 3.7 and requires a __future__ import:

Python 3.7.0rc1 (default, Jun 13 2018, 16:20:06)
[Clang 9.1.0 (clang-902.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from __future__ import annotations
>>> class Foo:
...   a:int
...
>>> Foo.__annotations__
{'a': 'int'}

z.a.attribute.AttributeAnnotations uses __annotations__ and can thus conflict in Python 3.6 and above, leading to false sharing (and failure to persist):

>>> from zope.annotation import attribute
>>> foo = Foo()
>>> attr_ann = AttributeAnnotations(foo)
>>> len(attr_ann)
1
>>> attr_ann['baz'] = 'bar'
>>> foo.__annotations__
{'a': 'int', 'baz': 'bar'}
>>> Foo.__annotations__
{'a': 'int', 'baz': 'bar'}

I'm not sure how to avoid it. The obvious solution is to use vars directly instead of __getitem__, but that breaks in the presence of __slots__, or more generally when a descriptor is used.

@jamadden
Copy link
Member

I guess we could add a check like if self.obj.__annotations__ is type(self.obj).__annotations__...

@mgedmin
Copy link
Member

mgedmin commented Jun 28, 2018

Should we rename the attribute to __zope_annotations__ and add backwards-compatibility code to migrate old persistent instances?

@freddrake
Copy link

freddrake commented Jun 28, 2018 via email

@icemac
Copy link
Member

icemac commented Oct 19, 2023

Closing this issue as we did not find any problems since the PEP was implemented.

@icemac icemac closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2023
@icemac
Copy link
Member

icemac commented May 29, 2024

Today I finally came across a problem: Using type annotations on a class which implements IAttributeAnnotatable creates __annotations__ on the class which is writeable and also appears on the instance. When using adapting an instance of such a class to IAnnotation the object on the class is changed!

So never do this:

@zope.interface.implementer(zope.annotation.interfaces.IAttributeAnnotatable)
class C:
    a: str | None = None

inst = C()
IAnnotations(inst)['foo'] = 'bar'

because then:

C.__annotations__ == {'a': str | None, 'foo': 'bar'}
C.__annotations__ is inst.__annotations__

Using Python 3.11 with current zope.annotation version.

@jamadden
Copy link
Member

jamadden commented Dec 2, 2024

I was also just bitten by the same thing @icemac was. Type annotations on a class that's later adapted to IAnnotations via IAttributeAnnotatable using the default adapter is broken. This is really bad because adding a type annotation to any base class breaks annotations on all subclassess as well, and this can be extremely hard to track down. (I only found it because some object was getting location proxied because of how zope.annotation.factory.factory works.)

>>> from zope.configuration import xmlconfig
>>> import zope.annotation
>>> xmlconfig.file('configure.zcml', zope.annotation)
<zope.configuration.config.ConfigurationMachine object at 0x101e97350>
>>> from zope.annotation.interfaces import *
>>> from zope.interface import *
>>> @implementer(IAttributeAnnotatable)
... class C:
...    instance_attr:str
...
>>> inst = C()
>>> inst.instance_attr
╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ in <module>:1                                                                                    │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: 'C' object has no attribute 'instance_attr'
>>> C.__annotations__
{'instance_attr': <class 'str'>}
>>> IAnnotations(inst)['key'] = 42
>>> C.__annotations__ # The class value has been mutated!
{'instance_attr': <class 'str'>, 'key': 42}
>>> other_inst = C()
>>> IAnnotations(other_inst)['key'] # So annotations are shared between objects!
42
>>> class D(C):
...   pass
...
>>> d = D()
>>> IAnnotations(d)['key']
42

Earlier, I wrote:

we could add a check like if self.obj.__annotations__ is type(self.obj).__annotations__

That would appear to solve this problem. Are there any alternatives?

@d-maurer
Copy link

...
Earlier, I wrote:

we could add a check like if self.obj.__annotations__ is type(self.obj).__annotations__

That would appear to solve this problem. Are there any alternatives?

For the moment, inst.__annotations__ does not seem to have an important use for type hints: I expect that type checkers and optimizers need to determine the annotations for all base classes to get the complete picture as inst.__annotations__ reflects only the annotations from the nearest class defining __annotations__. Therefore, we might still abuse it for our purposes. However, things might change in the future.

As an alternative to your check we could use isinstance(self.obj.__annotations__, AttributeAnnotations) or directly look into inst.__dict__. Those, too, suffer from abusing the __annotations__ attribute.

I assume that using a different attribute (_zope_annotations has been proposed earlier) would be safer in the long run. We would need to provide migration code and it could use one of the above checks to determine whether inst.__annotations__ really needs to be migrated.

@jamadden
Copy link
Member

For the moment, inst.__annotations__ does not seem to have an important use for type hints: I expect that type checkers and optimizers need to determine the annotations for all base classes to get the complete picture as inst.__annotations__ reflects only the annotations from the nearest class defining __annotations__.

I think that's a red herring and we can ignore it. The point of type checkers is that they're static, i.e., independent of the runtime state of the program, based only on what is declared in classes and annotations. Things happening to instances are pretty much ignored. E.g., mypy can't see/doesn't care that the instance in fact does have the desired attribute; it only cares about the (static) class:

$ cat foo.py
───────┬────────────────────────────────────────────────
       │ File: /tmp/foo.py
       │ Size: 89 B
───────┼────────────────────────────────────────────────
   1   │ class Foo:
   2   │     pass
   3   │
   4   │ foo = Foo()
   5   │ foo.thing = lambda: print('I am dynamic')
   6   │
   7   │ foo.thing()
───────┴───────────────────────────────────────────────
$ python foo.py
I am dynamic
$ mypy foo.py
/tmp/foo.py:5: error: "Foo" has no attribute "thing"  [attr-defined]
/tmp/foo.py:7: error: "Foo" has no attribute "thing"  [attr-defined]
Found 2 errors in 1 file (checked 1 source file)

As an alternative to your check we could use isinstance(self.obj.__annotations__, AttributeAnnotations)

isinstance is about three times slower than is. But that doesn't work anyway because self.obj.__annotations__ is either a OOBTree or a dict; and Class.__annotations__ is also just a dict. If they're both dict, you still have to do an identity check.

or directly look into inst.__dict__.

inst is not required to have a __dict__, slots are perfectly acceptable (and yes, I use that).

However, things might change in the future....We would need to provide migration code

To me, that's a (potential) problem for the future. I'm more interested in actually making this work now than dealing with a future-yet-to-be.

@d-maurer
Copy link

The problem might be bigger: a class can get an __annotations__ attribute without any type hints:
observed with Python 3.13.1

>>> class C: pass
... 
... C.__annotations__
... 
{}
>>> o = C()
>>> o.__annotations__
{}

@jamadden
Copy link
Member

Wow, TIL! The descriptor for __annotations__ automatically creates __annotations__ as soon as it is accessed.

Interestingly, though, the "official" API to access annotations, inspect.get_annotations, doesn't provoke this behaviour.

>>> class C:
...     pass
>>> '__annotations__' in dir(C)
False
>>> import inspect
>>> inspect.get_annotations(C)
{}
>>> '__annotations__' in dir(C)
False
>>> C.__annotations__
{}
>>> '__annotations__' in dir(C)
True

@jamadden
Copy link
Member

Ah, the auto-creation is new in 3.10, and I think is there to eliminate exactly the same kind of problem we're dealing with (false inheritance):

....As of Python 3.10, o.__annotations__ is guaranteed to always work on Python...classes,...Before Python 3.10, accessing __annotations__ on a class that defines no annotations but that has a parent class with annotations would return the parent’s __annotations__. In Python 3.10 and newer, the child class’s annotations will be an empty dict instead

@d-maurer d-maurer linked a pull request Dec 10, 2024 that will close this issue
@d-maurer
Copy link

d-maurer commented Dec 10, 2024 via email

@jamadden
Copy link
Member

jamadden commented Dec 10, 2024

Unfortunately, this means that your check is not safe

Right. On Python 3.10, you're supposed to use inspect.get_annotations. On earlier versions, you're supposed to use the dict directly: if isinstance(o, type): ann = o.__dict__.get('__annotations__', None).

@d-maurer
Copy link

d-maurer commented Dec 10, 2024 via email

@jamadden
Copy link
Member

The official way to get annotations, inspect.get_annotations, flat-out refuses to work on instances. So I think the chances of __annotations__ on an instance meaning anything official anytime in the near future is close to nil.

>>> class C:
...     var:str
...
>>> inst = C()
>>> inspect.get_annotations(inst)
Traceback (most recent call last) 
...
TypeError: <__main__.C object at 0x10298a1d0> is not a module, class, or callable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

6 participants