classification
Title: STORE_ANNOTATION bytecode is unnecessary and can be removed.
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Mark.Shannon Nosy List: Mark.Shannon, gvanrossum, levkivskyi, ned.deily, rhettinger, serhiy.storchaka, yselivanov
Priority: normal Keywords: patch

Created on 2018-01-14 12:05 by Mark.Shannon, last changed 2018-01-30 01:15 by gvanrossum. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5181 merged Mark.Shannon, 2018-01-14 12:29
Messages (21)
msg309918 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2018-01-29 22:35
Awesome!
msg311201 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2018-01-29 23:50
Rebased, pushed and CI is green.
msg311205 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) 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) * (Python committer) Date: 2018-01-30 01:15
W00t!
History
Date User Action Args
2018-01-30 01:15:20gvanrossumsetmessages: + msg311209
2018-01-30 00:41:39rhettingersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-01-30 00:41:12rhettingersetmessages: + msg311205
2018-01-29 23:50:15Mark.Shannonsetmessages: + msg311201
2018-01-29 22:35:56gvanrossumsetmessages: + msg311191
2018-01-29 22:35:13ned.deilysetnosy: + ned.deily
messages: + msg311190
2018-01-29 22:33:38Mark.Shannonsetmessages: + msg311189
2018-01-29 22:18:11gvanrossumsetmessages: + msg311183
2018-01-29 20:25:04rhettingersetmessages: + msg311163
2018-01-29 15:17:27gvanrossumsetmessages: + msg311142
2018-01-29 07:07:30rhettingersetassignee: Mark.Shannon
messages: + msg311075
2018-01-20 15:54:30gvanrossumsetmessages: + msg310354
2018-01-20 07:49:50serhiy.storchakasetmessages: + msg310329
2018-01-19 21:50:02gvanrossumsetmessages: + msg310291
2018-01-14 18:19:00rhettingersetnosy: + rhettinger
messages: + msg309930
2018-01-14 13:41:00Mark.Shannonsetmessages: + msg309924
2018-01-14 13:33:27levkivskyisetmessages: + msg309923
2018-01-14 13:18:18Mark.Shannonsetmessages: + msg309922
2018-01-14 12:56:32Mark.Shannonsetmessages: + msg309921
2018-01-14 12:32:44levkivskyisetmessages: + msg309920
2018-01-14 12:30:18serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg309919
2018-01-14 12:29:48Mark.Shannonsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request5034
2018-01-14 12:18:20serhiy.storchakasetnosy: + gvanrossum, yselivanov, levkivskyi
2018-01-14 12:05:38Mark.Shannoncreate