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

PEP 563: drop annotations for complex assign targets #86903

Open
isidentical opened this issue Dec 25, 2020 · 20 comments
Open

PEP 563: drop annotations for complex assign targets #86903

isidentical opened this issue Dec 25, 2020 · 20 comments
Labels
3.10 interpreter-core

Comments

@isidentical
Copy link
Sponsor Member

@isidentical isidentical commented Dec 25, 2020

BPO 42737
Nosy @gvanrossum, @serhiy-storchaka, @lysnikolaou, @pablogsal, @isidentical, @gousaiyang
PRs
  • #23952
  • #25511
  • 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 = None
    closed_at = None
    created_at = <Date 2020-12-25.09:32:49.995>
    labels = ['interpreter-core', '3.10']
    title = 'PEP 563: drop annotations for complex assign targets'
    updated_at = <Date 2021-04-25.02:59:46.169>
    user = 'https://github.com/isidentical'

    bugs.python.org fields:

    activity = <Date 2021-04-25.02:59:46.169>
    actor = 'BTaskaya'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2020-12-25.09:32:49.995>
    creator = 'BTaskaya'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42737
    keywords = ['patch']
    message_count = 20.0
    messages = ['383729', '383773', '383798', '383800', '383811', '383813', '383814', '383817', '383819', '390661', '390665', '390739', '390788', '390873', '391543', '391544', '391545', '391546', '391832', '391833']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'serhiy.storchaka', 'lys.nikolaou', 'pablogsal', 'BTaskaya', 'gousaiyang']
    pr_nums = ['23952', '25511']
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42737'
    versions = ['Python 3.10']

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 25, 2020

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

    @isidentical isidentical added 3.10 interpreter-core labels Dec 25, 2020
    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 25, 2020

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

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 26, 2020

    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.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 26, 2020

    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.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 26, 2020

    Yeah, we're talking about a future here where from __future__ import annotations is always on. And that future is now. (3.10)

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 26, 2020

    Do we still need to represent annotation as a subtree in AST? Or make it just a string?

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 26, 2020

    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 bpo-41967

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 26, 2020

    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

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 26, 2020

    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.

    @gousaiyang
    Copy link
    Mannequin

    @gousaiyang gousaiyang mannequin commented Apr 9, 2021

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

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 9, 2021

    If there is enough interest, I'd like to propose to this before the beta cut

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 10, 2021

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

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 11, 2021

    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.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 12, 2021

    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.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 21, 2021

    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?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 21, 2021

    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.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 21, 2021

    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.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 21, 2021

    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.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 25, 2021

    New changeset 8cc3cfa by Batuhan Taskaya in branch 'master':
    bpo-42737: annotations with complex targets no longer causes any runtime effects (GH-23952)
    8cc3cfa

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 25, 2021

    Ah, seems like there is still one open PR. Keeping this up for that...

    @isidentical isidentical reopened this Apr 25, 2021
    @isidentical isidentical reopened this Apr 25, 2021
    @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.10 interpreter-core
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants