msg383729 - (view) |
Author: Batuhan Taskaya (BTaskaya) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2021-04-25 02:59 |
Ah, seems like there is still one open PR. Keeping this up for that...
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:39 | admin | set | github: 86903 |
2021-04-25 02:59:46 | BTaskaya | set | status: closed -> open resolution: postponed -> messages:
+ msg391833
|
2021-04-25 02:59:21 | BTaskaya | set | status: open -> closed resolution: postponed stage: patch review -> resolved |
2021-04-25 02:31:28 | BTaskaya | set | messages:
+ msg391832 |
2021-04-21 23:06:34 | gousaiyang | set | pull_requests:
+ pull_request24229 |
2021-04-21 21:34:34 | BTaskaya | set | messages:
+ msg391546 |
2021-04-21 21:26:48 | gvanrossum | set | messages:
+ msg391545 |
2021-04-21 21:24:25 | gvanrossum | set | messages:
+ msg391544 |
2021-04-21 20:29:04 | BTaskaya | set | messages:
+ msg391543 |
2021-04-12 17:26:00 | gvanrossum | set | messages:
+ msg390873 |
2021-04-11 17:18:34 | BTaskaya | set | messages:
+ msg390788 |
2021-04-10 20:12:21 | gvanrossum | set | messages:
+ msg390739 |
2021-04-09 20:46:01 | BTaskaya | set | messages:
+ msg390665 |
2021-04-09 20:01:31 | gousaiyang | set | nosy:
+ gousaiyang messages:
+ msg390661
|
2020-12-26 18:42:51 | BTaskaya | set | messages:
+ msg383819 |
2020-12-26 18:21:50 | serhiy.storchaka | set | messages:
+ msg383817 |
2020-12-26 17:58:15 | BTaskaya | set | messages:
+ msg383814 |
2020-12-26 17:56:38 | serhiy.storchaka | set | messages:
+ msg383813 |
2020-12-26 17:44:59 | gvanrossum | set | messages:
+ msg383811 |
2020-12-26 10:27:27 | BTaskaya | set | messages:
+ msg383800 |
2020-12-26 09:24:25 | serhiy.storchaka | set | messages:
+ msg383798 |
2020-12-26 07:39:54 | BTaskaya | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request22799 |
2020-12-25 22:18:58 | gvanrossum | set | messages:
+ msg383773 |
2020-12-25 09:32:50 | BTaskaya | create | |