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

Can't assign entry to __annotations__ unless a class has existing annotation #95532

Open
ravwojdyla opened this issue Aug 1, 2022 · 21 comments
Labels
topic-typing type-bug An unexpected behavior, bug, or error

Comments

@ravwojdyla
Copy link

Bug report

Can't assign entry to __annotations__ unless a class has existing annotation. This is related to #88067.

python 3.9:

Python 3.9.7 | packaged by conda-forge | (default, Sep 29 2021, 20:33:18)
[Clang 11.1.0 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
...     __annotations__["b"] = int
...
>>> Foo.__annotations__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'Foo' has no attribute '__annotations__'
>>> class Foo:
...     a: int
...     __annotations__["b"] = int
...
>>> Foo.__annotations__
{'a': <class 'int'>, 'b': <class 'int'>}

python 3.10/python 3.11-rc:

Python 3.10.5 (main, Jul 12 2022, 11:32:11) [GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
...     __annotations__["a"] = int
...
>>> Foo.__annotations__
{}
>>> class Foo:
...     a: float
...     __annotations__["b"] = int
...
>>> Foo.__annotations__
{'a': <class 'float'>, 'b': <class 'int'>}

Your environment

  • CPython versions tested on: 3.9, 3.10, 3.11-rc
  • Operating system and architecture: Mac, Linux

I'm interested in submitting a PR for this, but first would like to double check that such PR/change would be acceptable?

@ravwojdyla ravwojdyla added the type-bug An unexpected behavior, bug, or error label Aug 1, 2022
@ericvsmith
Copy link
Member

What's the use case for this? I'm not sure we really want to support adding items to __annotations__ directly.

@ravwojdyla
Copy link
Author

ravwojdyla commented Aug 1, 2022

👋 @ericvsmith we have a schema definition framework built atop pydantic, and directly adding items to __annotations__ makes it possible to easily "copy" fields between schema classes, we utilise this a lot. This use case works fine for us, unless the schema class doesn't have any existing fields (as illustrated in this issue). I'm happy to describe this in more details if necessary.

I'm not sure we really want to support adding items to __annotations__ directly.

From PEP-0526:

__annotations__ is writable, so this is permitted:
__annotations__['s'] = str

@ericvsmith
Copy link
Member

Thanks, @ravwojdyla

Man, the formatting of that PEP section is messed up!

I assume you're saying you like the 3.10/3.11 behavior (which doesn't fail), and want it backported to 3.9? 3.9 is only getting security fixes now.

@ravwojdyla
Copy link
Author

I assume you're saying you like the 3.10/3.11 behavior (which doesn't fail), and want it backported to 3.9? 3.9 is only getting security fixes now.

@ericvsmith I think the 3.10/11 is an improvement from 3.9, but the bug here is that you can't assign item to __annotations__ unless there's already existing annotation in the class, which is inconsistent.

So from the example above:

>>> class Foo:
...     __annotations__["b"] = int
...
>>> Foo.__annotations__
{}

>>> # BUG: assignment did not work ^ where is the `b` field?

>>> class Foo:
...     a: float
...     __annotations__["b"] = int
...
>>> Foo.__annotations__
{'a': <class 'float'>, 'b': <class 'int'>}

>>> # NOTE: now the assignment worked for both `a` and `b`, only because there was existing `a` field

Does it make sense?

@ericvsmith
Copy link
Member

Ah. I missed that the returned dict was empty in the first 3.10 example.

@larryhastings thoughts?

@ravwojdyla
Copy link
Author

Ah, I understand now where does the b field go in this scenario:

>>> class Foo:
...     __annotations__["b"] = int
...
>>> Foo.__annotations__
{}

>>> # BUG: where is `b`?

See:

Python 3.12.0a0 (heads/fix-issue-95532-dirty:698fa8bf60, Aug  2 2022, 09:34:18) [Clang 12.0.0 (clang-1200.0.32.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> __annotations__
{}
>>> # module __annotations__ is empty - so far good

>>> class Foo:
...     __annotations__["c"] = float
...
>>> Foo.__annotations__
{}
>>> __annotations__
{'c': <class 'float'>}

>>> # `c` went into the module __annotations__, but see what happens next:

>>> class Bar:
...     a: int
...     __annotations__["b"] = float
...
>>> __annotations__
{'c': <class 'float'>}
>>> Bar.__annotations__
{'a': <class 'int'>, 'b': <class 'float'>}

>>> # both `a` and `b` are now in the the Bar class __annotations__

So I would say in the Foo example, __annotations__ should point to Foo.__annotations__ not the module's __annotations__.

Also note that this works as expected:

>>> class Baz:
...     __annotations__["c"] = float
...     a: int
...     __annotations__["b"] = float
...
>>> Baz.__annotations__
{'c': <class 'float'>, 'a': <class 'int'>, 'b': <class 'float'>}
>>> __annotations__
{}

So it seems like the existence of the a annotated field in the Baz class is somehow binding __annotations__ to the class.

@ravwojdyla
Copy link
Author

ravwojdyla commented Aug 2, 2022

And this issue is reproducible with the 3.6 PEP-0526 implementation #72172. Do we agree that __annotations__ in the Foo example should bind to Foo.__annotations__? Would be happy to try to PR a fix for that.

@ravwojdyla
Copy link
Author

ravwojdyla commented Aug 8, 2022

👋 @ericvsmith what's the best way to get feedback/guidance for this issue and question above?

@ericvsmith
Copy link
Member

@ravwojdyla : Maybe mention it on discuss.python.org, pointing to this issue.

@ravwojdyla
Copy link
Author

@terryjreedy
Copy link
Member

terryjreedy commented Aug 13, 2022

This strikes me as a (supprising) bug. Any assignments in class scope scope to a writable name should be in the class namespace unless a global declaration says otherwise.

When you find the code that does this, ping the author (found using git blame or git log) either here or on a PR.

@sobolevn
Copy link
Member

sobolevn commented Sep 5, 2022

Very interesting! Let's dive into it.

>>> import dis
>>> def first():
...   class Foo:
...     a: int
...     __annotations__['b'] = int
... 
>>> def second():
...   class Foo:
...     __annotations__['b'] = int
... 

Let's see why they are different:

>>> dis.dis(first)
  1           0 RESUME                   0

  2           2 PUSH_NULL
              4 LOAD_BUILD_CLASS
              6 LOAD_CONST               1 (<code object Foo at 0x104d0cd40, file "<stdin>", line 2>)
              8 MAKE_FUNCTION            0
             10 LOAD_CONST               2 ('Foo')
             12 CALL                     2
             22 STORE_FAST               0 (Foo)
             24 LOAD_CONST               0 (None)
             26 RETURN_VALUE

Disassembly of <code object Foo at 0x104d0cd40, file "<stdin>", line 2>:
  2           0 RESUME                   0
              2 LOAD_NAME                0 (__name__)
              4 STORE_NAME               1 (__module__)
              6 LOAD_CONST               0 ('first.<locals>.Foo')
              8 STORE_NAME               2 (__qualname__)
             10 SETUP_ANNOTATIONS

  3          12 LOAD_NAME                3 (int)
             14 LOAD_NAME                4 (__annotations__)
             16 LOAD_CONST               1 ('a')
             18 STORE_SUBSCR

  4          22 LOAD_NAME                3 (int)
             24 LOAD_NAME                4 (__annotations__)
             26 LOAD_CONST               2 ('b')
             28 STORE_SUBSCR
             32 LOAD_CONST               3 (None)
             34 RETURN_VALUE

Note 10 SETUP_ANNOTATIONS

>>> dis.dis(second)
  1           0 RESUME                   0

  2           2 PUSH_NULL
              4 LOAD_BUILD_CLASS
              6 LOAD_CONST               1 (<code object Foo at 0x104cedd50, file "<stdin>", line 2>)
              8 MAKE_FUNCTION            0
             10 LOAD_CONST               2 ('Foo')
             12 CALL                     2
             22 STORE_FAST               0 (Foo)
             24 LOAD_CONST               0 (None)
             26 RETURN_VALUE

Disassembly of <code object Foo at 0x104cedd50, file "<stdin>", line 2>:
  2           0 RESUME                   0
              2 LOAD_NAME                0 (__name__)
              4 STORE_NAME               1 (__module__)
              6 LOAD_CONST               0 ('second.<locals>.Foo')
              8 STORE_NAME               2 (__qualname__)

  3          10 LOAD_NAME                3 (int)
             12 LOAD_NAME                4 (__annotations__)
             14 LOAD_CONST               1 ('b')
             16 STORE_SUBSCR
             20 LOAD_CONST               2 (None)
             22 RETURN_VALUE

Note that 10 SETUP_ANNOTATIONS is not present.
Why? Because find_ann function is looking for AnnAssign_kind statement kind to actually set annotations up.

The simpliest option I see is to always call SETUP_ANNOTATIONS for class bodies, similar to how we treat __qualname__, __name__, and __module__.

The downside of this solution is that we slow down all classes even without any annotations. How bad is it? I don't know: I will need to write a benchmark.

Other ideas?

@ravwojdyla
Copy link
Author

Nice @sobolevn!

Btw https://docs.python.org/3/library/dis.html#opcode-SETUP_ANNOTATIONS

This opcode is only emitted if a class or module body contains variable annotations statically.

Regarding Terry request:

When you find the code that does this, ping the author (found using git blame or git log) either here or on a PR.

@ilevkivskyi FYI ^

@ilevkivskyi
Copy link
Member

Yeah, IIRC we already understood there may be downsides when we decided to conditionally create __annotations__. It's totally OK to always create __annotations__ if it turns out having it defined is more important than the (modest) memory savings. (Also having class scope one bound to global one is definitely a bug, probably something is missing in symbol table analysis).

@sobolevn
Copy link
Member

sobolevn commented Sep 6, 2022

Also having class scope one bound to global one is definitely a bug, probably something is missing in symbol table analysis

@ilevkivskyi to clarify, are you talking about __annotations__ being resolved to a global variable inside a class scope (when __annotations__ is not defined), like:

>>> a = {}
>>> class Some:
...    a['a'] = int
... 
>>> a
{'a': <class 'int'>}

?

@ravwojdyla
Copy link
Author

@sobolevn I believe @ilevkivskyi is talking about this case:

Python 3.12.0a0 (heads/fix-issue-95532-dirty:698fa8bf60, Aug  2 2022, 09:34:18) [Clang 12.0.0 (clang-1200.0.32.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> __annotations__
{}
>>> # module __annotations__ is empty - so far good

>>> class Foo:
...     __annotations__["c"] = float
...
>>> Foo.__annotations__
{}
>>> __annotations__
{'c': <class 'float'>}

See above: #95532 (comment), this type of assignment to __annotations__ works if __annotations__ already exists at the class scope.

@sobolevn
Copy link
Member

sobolevn commented Sep 6, 2022

@ravwojdyla this is exactly the case I've showed in #95532 (comment) using a = {} as more general case.

@ravwojdyla
Copy link
Author

@sobolevn understood, was just pointed to the example @ilevkivskyi was referring to in the comments above. Sounds like @ilevkivskyi is ok with just creating __annotations__ at all times, which will resolve both problems highlighted in this issue. In that case unless there's any concerns will go ahead and try to PR that(?)

@sobolevn
Copy link
Member

sobolevn commented Sep 6, 2022

I think it is better to let a person to speak for themself :)
Let's wait for some more information before diving into it 😉

@ilevkivskyi
Copy link
Member

I think I misunderstood, I thought there was some case where it was bound to global one while class-level one is also defined. But it looks like the class-level one is generated lazily when accessed. IIUC this was added later (found it in #25623), while in 3.6 accessing __annotations__ on a class without annotations raised attribute error. So one would have:

>>> class C:
...     __annotations__["x"] = int
... 
>>> C.__annotations__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'C' has no attribute '__annotations__'
>>>

This was consistent with how any other name (like a above) would behave in this case. But with lazy generation of __annotations__ this now becomes confusing.

@ravwojdyla
Copy link
Author

Also FYI #88067 (comment) and #88067 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants