Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STORE_ANNOTATION bytecode is unnecessary and can be removed. #76731

Closed
markshannon opened this issue Jan 14, 2018 · 21 comments
Closed

STORE_ANNOTATION bytecode is unnecessary and can be removed. #76731

markshannon opened this issue Jan 14, 2018 · 21 comments
Assignees
Labels
3.7 interpreter-core type-feature

Comments

@markshannon
Copy link
Member

@markshannon markshannon commented Jan 14, 2018

BPO 32550
Nosy @gvanrossum, @rhettinger, @ned-deily, @markshannon, @serhiy-storchaka, @1st1, @ilevkivskyi
PRs
  • #5181
  • 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:

    assignee = 'https://github.com/markshannon'
    closed_at = <Date 2018-01-30.00:41:39.485>
    created_at = <Date 2018-01-14.12:05:38.904>
    labels = ['interpreter-core', 'type-feature', '3.7']
    title = 'STORE_ANNOTATION bytecode is unnecessary and can be removed.'
    updated_at = <Date 2018-01-30.01:15:20.937>
    user = 'https://github.com/markshannon'

    bugs.python.org fields:

    activity = <Date 2018-01-30.01:15:20.937>
    actor = 'gvanrossum'
    assignee = 'Mark.Shannon'
    closed = True
    closed_date = <Date 2018-01-30.00:41:39.485>
    closer = 'rhettinger'
    components = ['Interpreter Core']
    creation = <Date 2018-01-14.12:05:38.904>
    creator = 'Mark.Shannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32550
    keywords = ['patch']
    message_count = 21.0
    messages = ['309918', '309919', '309920', '309921', '309922', '309923', '309924', '309930', '310291', '310329', '310354', '311075', '311142', '311163', '311183', '311189', '311190', '311191', '311201', '311205', '311209']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'rhettinger', 'ned.deily', 'Mark.Shannon', 'serhiy.storchaka', 'yselivanov', 'levkivskyi']
    pr_nums = ['5181']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue32550'
    versions = ['Python 3.7']

    @markshannon
    Copy link
    Member Author

    @markshannon markshannon commented Jan 14, 2018

    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

    @markshannon markshannon added 3.7 interpreter-core type-feature labels Jan 14, 2018
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 14, 2018

    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).

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 14, 2018

    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.

    @markshannon
    Copy link
    Member Author

    @markshannon markshannon commented Jan 14, 2018

    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?

    @markshannon
    Copy link
    Member Author

    @markshannon markshannon commented Jan 14, 2018

    1. 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)

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 14, 2018

    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.

    @markshannon
    Copy link
    Member Author

    @markshannon markshannon commented Jan 14, 2018

    Works as expected:

    >>> class C:
    ...     exec('x: int')
    ... 
    >>> C.__annotations__
    {'x': <class 'int'>}
    >>> __annotations__
    {}

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 14, 2018

    I would like to see this patch go forward. Making __annotations__ special and subtly different just adds to language complexity.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 19, 2018

    I won't object. It makes sense that the implementation of __annotations__ should be low-key.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 20, 2018

    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__).

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 20, 2018

    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.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 29, 2018

    There are a couple of merge conflicts in the patch. Once deconflicted, I think this PR is ready to apply.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 29, 2018

    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?

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 29, 2018

    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.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 29, 2018

    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.

    @markshannon
    Copy link
    Member Author

    @markshannon markshannon commented Jan 29, 2018

    If it can wait another hour, I will be at home and can do the rebase then.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 29, 2018

    Mark, at the moment, you have at least another 14 hours until the announced code freeze deadline :)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 29, 2018

    Awesome!

    @markshannon
    Copy link
    Member Author

    @markshannon markshannon commented Jan 29, 2018

    Rebased, pushed and CI is green.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 30, 2018

    New changeset 332cd5e by Raymond Hettinger (Mark Shannon) in branch 'master':
    bpo-32550. Remove the STORE_ANNOTATION bytecode. (GH-5181)
    332cd5e

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 30, 2018

    W00t!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants