msg309918 - (view) |
Author: Mark Shannon (Mark.Shannon) *  |
Date: 2018-01-14 12:05 |
The STORE_ANNOTATION bytecode is used to implement annotations.
The code
name : ann
is equivalent to
__annotations__['name'] = ann
Consequently,
STORE_ANNOTATION name
can be trivially replaced with
LOAD_NAME __annotations__
LOAD_CONST 'name'
STORE_SUBSCR
|
msg309919 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2018-01-14 12:30 |
There some subtle differences.
1. Unlike to LOAD_NAME, STORE_ANNOTATION doesn't fall back to globals if '__annotations__' is not found in locals. The behavior difference is shown by the following example:
x: int
class A:
del __annotations__
y: int
2. The single STORE_ANNOTATION is faster than 3 other opcodes.
3. It doesn't add a name constant. Instead it uses a name from the names list (which already has to contain this name).
|
msg309920 - (view) |
Author: Ivan Levkivskyi (levkivskyi) *  |
Date: 2018-01-14 12:32 |
There are several corner cases. For example consider this code:
>>> class C:
... del __annotations__
... x: int
Currently this correctly raises NameError, with your replacement it will instead stick {'x': int} in the module `__annotations__`. I think there may be other special cases but I don't remember them now.
|
msg309921 - (view) |
Author: Mark Shannon (Mark.Shannon) *  |
Date: 2018-01-14 12:56 |
PEP 526 explicitly states
"as with all dunder attributes, any undocummented use of __annotations__ is subject to breakage without warning"
I consider deleting __annotations__ to be undocumented use.
Do you really think that an obscure difference in the behaviour of
>>> class C:
... del __annotations__
... x: int
justifies an extra bytecode, but the implicit return at the end of all functions (LOAD_CONST None; RETURN_VALUE) does not?
|
msg309922 - (view) |
Author: Mark Shannon (Mark.Shannon) *  |
Date: 2018-01-14 13:18 |
> 3. It doesn't add a name constant. Instead it uses a name from the names list (which already has to contain this name).
This PR moves the constant for the name from `co_names` to `co_consts`. There is no duplication.
>>> def f():
... class C:
... a : 1
(It does add __annotations__ to `co_names`, but that seems reasonable to me)
Current:
>>> f.__code__.co_consts[1].co_names
('__name__', '__module__', '__qualname__', 'a')
>>> f.__code__.co_consts[1].co_consts
('f.<locals>.C', 1, None)
With PR 5181:
>>> f.__code__.co_consts[1].co_names
('__name__', '__module__', '__qualname__', '__annotations__')
>>> f.__code__.co_consts[1].co_consts
('f.<locals>.C', 1, 'a', None)
|
msg309923 - (view) |
Author: Ivan Levkivskyi (levkivskyi) *  |
Date: 2018-01-14 13:33 |
There is also another corner case to consider:
class C:
exec('x: int')
assert C.__annotations__ == {'x': int}
assert __annotations__ == {}
I am not sure this one will be covered correctly.
But the main argument here is speed I think.
Let us see what others think about this.
|
msg309924 - (view) |
Author: Mark Shannon (Mark.Shannon) *  |
Date: 2018-01-14 13:41 |
Works as expected:
>>> class C:
... exec('x: int')
...
>>> C.__annotations__
{'x': <class 'int'>}
>>> __annotations__
{}
|
msg309930 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2018-01-14 18:19 |
I would like to see this patch go forward. Making __annotations__ special and subtly different just adds to language complexity.
|
msg310291 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2018-01-19 21:50 |
I won't object. It makes sense that the implementation of __annotations__ should be low-key.
|
msg310329 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2018-01-20 07:49 |
> This PR moves the constant for the name from `co_names` to `co_consts`. There is no duplication.
But if there is an initializer, the name is left in `co_names` too.
I don't think this (as well as possible performance difference) is important. My only concerns are about subtle behavior differences.
For example, is the name always interned (even if long or non-ASCII)? Does any code depend on interning keys in __annotations__? (There is a code that depends on interning keys in type.__dict__).
|
msg310354 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2018-01-20 15:54 |
There is very little code that depends on __annotations__ at all, and none
of it is very subtle. All actual type checking is done by separate tools
(mypy, pytype, PyCharm) that don't import the code and don't care about
__annotations__. The __annotations__ variable exists so that other people
*may* do useful stuff with it, e.g. design run-time type-checking
decorators. So please don't worry about interning.
|
msg311075 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2018-01-29 07:07 |
There are a couple of merge conflicts in the patch. Once deconflicted, I think this PR is ready to apply.
|
msg311142 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2018-01-29 15:17 |
Is this going to make it into beta 1 before tonight? If not should we abandon it or put it off till 3.8 or can it go into 3.7beta2?
|
msg311163 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2018-01-29 20:25 |
If Mark can get it in tonight, that would be great. The patch has been ready for a long time. It just needs to have the merge conflicts resolved.
Otherwise, I think 3.7beta2 would be fine.
|
msg311183 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2018-01-29 22:18 |
I expect that Mark only checks his mail occasionally. Sometimes GitHub lets you edit a PR and then you can also resolve the conflict yourself. If that's not the case feel free to fork the PR and submit it yourself.
|
msg311189 - (view) |
Author: Mark Shannon (Mark.Shannon) *  |
Date: 2018-01-29 22:33 |
If it can wait another hour, I will be at home and can do the rebase then.
|
msg311190 - (view) |
Author: Ned Deily (ned.deily) *  |
Date: 2018-01-29 22:35 |
Mark, at the moment, you have at least another 14 hours until the announced code freeze deadline :)
|
msg311191 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2018-01-29 22:35 |
Awesome!
|
msg311201 - (view) |
Author: Mark Shannon (Mark.Shannon) *  |
Date: 2018-01-29 23:50 |
Rebased, pushed and CI is green.
|
msg311205 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2018-01-30 00:41 |
New changeset 332cd5ee4ff42c9904c56e68a1028f383f7fc9a8 by Raymond Hettinger (Mark Shannon) in branch 'master':
bpo-32550. Remove the STORE_ANNOTATION bytecode. (GH-5181)
https://github.com/python/cpython/commit/332cd5ee4ff42c9904c56e68a1028f383f7fc9a8
|
msg311209 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
Date: 2018-01-30 01:15 |
W00t!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:56 | admin | set | github: 76731 |
2018-01-30 01:15:20 | gvanrossum | set | messages:
+ msg311209 |
2018-01-30 00:41:39 | rhettinger | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
2018-01-30 00:41:12 | rhettinger | set | messages:
+ msg311205 |
2018-01-29 23:50:15 | Mark.Shannon | set | messages:
+ msg311201 |
2018-01-29 22:35:56 | gvanrossum | set | messages:
+ msg311191 |
2018-01-29 22:35:13 | ned.deily | set | nosy:
+ ned.deily messages:
+ msg311190
|
2018-01-29 22:33:38 | Mark.Shannon | set | messages:
+ msg311189 |
2018-01-29 22:18:11 | gvanrossum | set | messages:
+ msg311183 |
2018-01-29 20:25:04 | rhettinger | set | messages:
+ msg311163 |
2018-01-29 15:17:27 | gvanrossum | set | messages:
+ msg311142 |
2018-01-29 07:07:30 | rhettinger | set | assignee: Mark.Shannon messages:
+ msg311075 |
2018-01-20 15:54:30 | gvanrossum | set | messages:
+ msg310354 |
2018-01-20 07:49:50 | serhiy.storchaka | set | messages:
+ msg310329 |
2018-01-19 21:50:02 | gvanrossum | set | messages:
+ msg310291 |
2018-01-14 18:19:00 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg309930
|
2018-01-14 13:41:00 | Mark.Shannon | set | messages:
+ msg309924 |
2018-01-14 13:33:27 | levkivskyi | set | messages:
+ msg309923 |
2018-01-14 13:18:18 | Mark.Shannon | set | messages:
+ msg309922 |
2018-01-14 12:56:32 | Mark.Shannon | set | messages:
+ msg309921 |
2018-01-14 12:32:44 | levkivskyi | set | messages:
+ msg309920 |
2018-01-14 12:30:18 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg309919
|
2018-01-14 12:29:48 | Mark.Shannon | set | keywords:
+ patch stage: needs patch -> patch review pull_requests:
+ pull_request5034 |
2018-01-14 12:18:20 | serhiy.storchaka | set | nosy:
+ gvanrossum, yselivanov, levkivskyi
|
2018-01-14 12:05:38 | Mark.Shannon | create | |