classification
Title: PEP 563: drop annotations for complex assign targets
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: BTaskaya, gousaiyang, gvanrossum, lys.nikolaou, pablogsal, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-12-25 09:32 by BTaskaya, last changed 2021-04-25 02:59 by BTaskaya.

Pull Requests
URL Status Linked Edit
PR 23952 merged BTaskaya, 2020-12-26 07:39
PR 25511 open gousaiyang, 2021-04-21 23:06
Messages (20)
msg383729 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-12-25 09:32
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 issue 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 issue 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.
msg383773 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-12-25 22:18
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.)
msg383798 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-26 09:24
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.
msg383800 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-12-26 10:27
> 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.
msg383811 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-12-26 17:44
Yeah, we're talking about a future here where `from __future__ import annotations` is always on. And that future is now. (3.10)
msg383813 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-26 17:56
Do we still need to represent annotation as a subtree in AST? Or make it just a string?
msg383814 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-12-26 17:58
> 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 issue 41967
msg383817 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-26 18:21
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
msg383819 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-12-26 18:42
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.
msg390661 - (view) Author: Saiyang Gou (gousaiyang) * Date: 2021-04-09 20:01
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).
msg390665 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2021-04-09 20:46
If there is enough interest, I'd like to propose to this before the beta cut
msg390739 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-04-10 20:12
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.)
msg390788 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2021-04-11 17:18
> 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.
msg390873 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-04-12 17:26
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.
msg391543 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2021-04-21 20:29
> 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?
msg391544 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-04-21 21:24
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.
msg391545 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-04-21 21:26
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.
msg391546 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2021-04-21 21:34
> 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.
msg391832 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2021-04-25 02:31
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
msg391833 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2021-04-25 02:59
Ah, seems like there is still one open PR. Keeping this up for that...
History
Date User Action Args
2021-04-25 02:59:46BTaskayasetstatus: closed -> open
resolution: postponed ->
messages: + msg391833
2021-04-25 02:59:21BTaskayasetstatus: open -> closed
resolution: postponed
stage: patch review -> resolved
2021-04-25 02:31:28BTaskayasetmessages: + msg391832
2021-04-21 23:06:34gousaiyangsetpull_requests: + pull_request24229
2021-04-21 21:34:34BTaskayasetmessages: + msg391546
2021-04-21 21:26:48gvanrossumsetmessages: + msg391545
2021-04-21 21:24:25gvanrossumsetmessages: + msg391544
2021-04-21 20:29:04BTaskayasetmessages: + msg391543
2021-04-12 17:26:00gvanrossumsetmessages: + msg390873
2021-04-11 17:18:34BTaskayasetmessages: + msg390788
2021-04-10 20:12:21gvanrossumsetmessages: + msg390739
2021-04-09 20:46:01BTaskayasetmessages: + msg390665
2021-04-09 20:01:31gousaiyangsetnosy: + gousaiyang
messages: + msg390661
2020-12-26 18:42:51BTaskayasetmessages: + msg383819
2020-12-26 18:21:50serhiy.storchakasetmessages: + msg383817
2020-12-26 17:58:15BTaskayasetmessages: + msg383814
2020-12-26 17:56:38serhiy.storchakasetmessages: + msg383813
2020-12-26 17:44:59gvanrossumsetmessages: + msg383811
2020-12-26 10:27:27BTaskayasetmessages: + msg383800
2020-12-26 09:24:25serhiy.storchakasetmessages: + msg383798
2020-12-26 07:39:54BTaskayasetkeywords: + patch
stage: patch review
pull_requests: + pull_request22799
2020-12-25 22:18:58gvanrossumsetmessages: + msg383773
2020-12-25 09:32:50BTaskayacreate