PEP 563: drop annotations for complex assign targets

[isidentical]

BPO	42737
Nosy	@gvanrossum, @serhiy-storchaka, @lysnikolaou, @pablogsal, @isidentical, @gousaiyang
PRs	python/cpython#23952 python/cpython#25511

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['interpreter-core', '3.10'] title = 'PEP 563: drop annotations for complex assign targets' updated_at = user = 'https://github.com/isidentical' ``` bugs.python.org fields: ```python activity = actor = 'BTaskaya' assignee = 'none' closed = False closed_date = None closer = None components = ['Interpreter Core'] creation = creator = 'BTaskaya' dependencies = [] files = [] hgrepos = [] issue_num = 42737 keywords = ['patch'] message_count = 20.0 messages = ['383729', '383773', '383798', '383800', '383811', '383813', '383814', '383817', '383819', '390661', '390665', '390739', '390788', '390873', '391543', '391544', '391545', '391546', '391832', '391833'] nosy_count = 6.0 nosy_names = ['gvanrossum', 'serhiy.storchaka', 'lys.nikolaou', 'pablogsal', 'BTaskaya', 'gousaiyang'] pr_nums = ['23952', '25511'] priority = 'normal' resolution = None stage = 'resolved' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue42737' versions = ['Python 3.10'] ```

[isidentical]

PEP-526 classifies everything but simple, unparenthesized names (a.b, (a), a[b]) as complex targets. The way the we handle annotations for them right now is, doing literally nothing but evaluating every part of it (just pushing the name to the stack, and popping, without even doing the attribute access);

$ cat t.py
foo[bar]: int
$ python -m dis t.py
  1           0 SETUP_ANNOTATIONS
              2 LOAD_NAME                0 (foo)
              4 POP_TOP
              6 LOAD_NAME                1 (bar)
              8 POP_TOP
             10 LOAD_NAME                2 (int)
             12 POP_TOP
             14 LOAD_CONST               0 (None)
             16 RETURN_VALUE

$ cat t.py
a.b: int
$ python -m dis t.py
  1           0 SETUP_ANNOTATIONS
              2 LOAD_NAME                0 (a)
              4 POP_TOP
              6 LOAD_NAME                1 (int)
              8 POP_TOP
             10 LOAD_CONST               0 (None)
             12 RETURN_VALUE

I noticed this while creating a patch for bpo-42725, since I had to create an extra check for non-simple annassign targets (because compiler tries to find their scope, int in this case is not compiled to string).

Since they have no real side effect but just accessing a name, I'd propose we drop this from 3.10 so that both I can simply the patch for bpo-42725, and also we have consistency with what we do when the target is simple (instead of storing this time, we'll just drop the bytecode).

$ cat t.py
a.b: int = 5
$ python -m dis t.py
  1           0 SETUP_ANNOTATIONS
              2 LOAD_CONST               0 (5)
              4 LOAD_NAME                0 (a)
              6 STORE_ATTR               1 (b)
              8 LOAD_NAME                2 (int)
             10 POP_TOP
             12 LOAD_CONST               1 (None)
             14 RETURN_VALUE

8/10 will be gone in this case.

If agreed upon, I can propose a patch.

[gvanrossum]

So the distinction between simple and complex is to define what goes into __annotations__. As long as we don't disturb that I think it's fine not to evaluate anything. (There's also the effect on what's considered a local variable, inside functions.)

[serhiy-storchaka]

Alternatively it could be evaluated in global scope. All names are globals (non-global names do not work in MyPy in any case), yield and await are forbidden outside function. It will still perform run-time check which was an initial intention.

[isidentical]

It will still perform run-time check which was an initial intention.

Well, at least from my personal perspective, I'd expect all annotations to behave like strings regardless of their targets.

[gvanrossum]

Yeah, we're talking about a future here where from __future__ import annotations is always on. And that future is now. (3.10)

[serhiy-storchaka]

Do we still need to represent annotation as a subtree in AST? Or make it just a string?

[isidentical]

Do we still need to represent annotation as a subtree in AST? Or make it just a string?

Possibly, we will just represent them as Constant()s. See bpo-41967

[serhiy-storchaka]

About the difference in behavior. Currently:

>>> (1/0).numerator: int
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ZeroDivisionError: division by zero
>>> x[0]: int
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'x' is not defined
>>> [][print('Hi!')]: int
Hi!

With PR 23952 it all will be no-op.

Also, there should be changes in the following weird example:

def f():
   (yield).x: int

[isidentical]

I think we should still evaluate the targets (even though the sole purpose of this is just having a hint, the yield example seems serious), will update my patch accordingly.

[799a973d-479e-458f-b448-ca9b830a935e]

I think we can just skip evaluating annotations for complex targets when in module or class scope (they are not stored anyway). The point of PEP-563 is to suppress any evaluation of annotations (regardless of position) at definition time, while type checkers can still analyze them as usual.

This is the current behavior (ever since 3.7, with from __future__ import annotations):

Python 3.10.0a7 (default, Apr  6 2021, 17:59:12) [GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> x = 1
>>> x.y: print('evaluated in module')
evaluated in module
>>> class A:
...     u = 2
...     u.v: print('evaluated in class')
...
evaluated in class

And I think they should become no-ops at run-time (as if the annotations were wrapped in string form).

[isidentical]

If there is enough interest, I'd like to propose to this before the beta cut

[gvanrossum]

Hm, reading the thread it's not 100% clear to me what you are proposing to do in your patch, since different people seem to have proposed different things.

I think I'd be okay if foo[bar]: baz and foo.bar: baz (etc.) didn't generate any bytecode at all. Is that what you're proposing here? If so, and assuming the code is reasonably straightforward, I'd say go ahead and make a PR (and close the old OR).

In case it's relevant, I don't peronally expect Larry's clever alternative to PEP-563 to make it past the SC, and I don't care much about it (too many tricky edge cases), but it's out of my control, so don't count on that being dead just yet.

And FWIW changing how annotations are represented in the AST is out of scope for this issue. (There are tricky edge cases there too.)

[isidentical]

I think I'd be okay if foo[bar]: baz and foo.bar: baz (etc.) didn't generate any bytecode at all. Is that what you're proposing here? If so, and assuming the code is reasonably straightforward, I'd say go ahead and make a PR (and close the old OR).

The thread raised some concerns regarding the verification of the lhs (the target), so the PR 23952 generates code for the lhs but omits the annotation part.

And FWIW changing how annotations are represented in the AST is out of scope for this issue. (There are tricky edge cases there too.)

Agreed, it already has a separate issue.

[gvanrossum]

Batuhan, can you summarize the argument from the thread? What do you think yourself? Myself, I'm not sure either way, but it seems okay to remove the remnant bytecode.

[isidentical]

Batuhan, can you summarize the argument from the thread? What do you think yourself? Myself, I'm not sure either way, but it seems okay to remove the remnant bytecode.

I feel like we shouldn't generate code for these annotations at all, and only evaluate the targets. Since this is one of the things that blocks us right now regarding the bugfix of different issues (e.g annotations have side effects on symbol table).

Even though the PEP-563 being default change is reverted, I think we should move forward with this change but only do it under the future_annotations flag. @gvanrossum @pablogsal what do you think about this?

[gvanrossum]

Hum, there seems to be an actual bug here: even with PEP-563, the annotations for "complex targets" are evaluated. For example:

from __future__ import annotations
class C:
    x.y: z.w
    a: b.c

The relevant parts of the disassembly of the code for the class object are:

3 10 LOAD_NAME 3 (x) 12 POP_TOP 14 LOAD_NAME 4 (z) 16 LOAD_ATTR 5 (w) 18 POP_TOP

4 20 LOAD_CONST 1 ('b.c') 22 LOAD_NAME 6 (annotations) 24 LOAD_CONST 2 ('a') 26 STORE_SUBSCR

So for x.y: z.w, not only do we evaluate x, we also evaluate z.w; whereas b.c is not evaluated (the stringified version is added as __annotations__['a']).

I think the "LOAD_NAME(x), POP_TOP" part is correct, but "LOAD_NAME(z), LOAD_ATTR(w), POP_TOP" should not be generated at all.

[gvanrossum]

The same thing happens at the module level. However in functions all is well (because inside functions annotations are never evaluated, apparently).

It may be as simple as the code forgetting to check the PEP-563 bit when generating code for complex annotations in classes and at the top (module) level.

[isidentical]

Hum, there seems to be an actual bug here: even with PEP-563, the annotations for "complex targets" are evaluated. For example:

Yes, that is what this issue is about. This bug only surfaced while doing other stuff and PEP-563 being the default

I think the "LOAD_NAME(x), POP_TOP" part is correct, but "LOAD_NAME(z), LOAD_ATTR(w), POP_TOP" should not be generated at all.

Agreed, which is what the PR 23952 has proposed.

I'll proceed with the PR and update it according to the revert of PEP-563, which should hopefully fix this only when the future flag is activated.

[isidentical]

New changeset 8cc3cfa8afab1651c4f6e9ba43a7ab7f10f64c32 by Batuhan Taskaya in branch 'master': bpo-42737: annotations with complex targets no longer causes any runtime effects (GH-23952) https://github.com/python/cpython/commit/8cc3cfa8afab1651c4f6e9ba43a7ab7f10f64c32

[isidentical]

Ah, seems like there is still one open PR. Keeping this up for that...