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

Implement PEP 526 #72172

Closed
gvanrossum opened this issue Sep 6, 2016 · 33 comments
Closed

Implement PEP 526 #72172

gvanrossum opened this issue Sep 6, 2016 · 33 comments
Assignees
Labels
interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@gvanrossum
Copy link
Member

gvanrossum commented Sep 6, 2016

BPO 27985
Nosy @gvanrossum, @1st1, @ilevkivskyi
Files
  • hg-pep-526.diff: (ignore)
  • hg-pep-526-v2a.diff
  • hg-pep-526-v3.diff: (ignore)
  • hg-pep-526-v4.diff
  • hg-pep-526-v4a.diff
  • hg-pep-526-v5.diff: (Renamed to v5 to match code review labeling)
  • 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/gvanrossum'
    closed_at = <Date 2016-09-09.03:54:03.020>
    created_at = <Date 2016-09-06.23:48:46.989>
    labels = ['interpreter-core', 'type-feature']
    title = 'Implement PEP 526'
    updated_at = <Date 2016-09-09.21:48:16.681>
    user = 'https://github.com/gvanrossum'

    bugs.python.org fields:

    activity = <Date 2016-09-09.21:48:16.681>
    actor = 'python-dev'
    assignee = 'gvanrossum'
    closed = True
    closed_date = <Date 2016-09-09.03:54:03.020>
    closer = 'yselivanov'
    components = ['Interpreter Core']
    creation = <Date 2016-09-06.23:48:46.989>
    creator = 'gvanrossum'
    dependencies = []
    files = ['44415', '44416', '44469', '44470', '44474', '44478']
    hgrepos = []
    issue_num = 27985
    keywords = ['patch']
    message_count = 33.0
    messages = ['274672', '274693', '274695', '274914', '274918', '274919', '274925', '274929', '274930', '274938', '274954', '275079', '275080', '275081', '275083', '275085', '275088', '275097', '275115', '275117', '275136', '275158', '275163', '275169', '275184', '275191', '275192', '275245', '275246', '275247', '275265', '275267', '275458']
    nosy_count = 4.0
    nosy_names = ['gvanrossum', 'python-dev', 'yselivanov', 'levkivskyi']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27985'
    versions = ['Python 3.6']

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 6, 2016

    Pending PEP-526's acceptance, I am inviting Ivan Levkivskyi to upload his patch here so it can be reviewed.

    @gvanrossum gvanrossum self-assigned this Sep 6, 2016
    @gvanrossum gvanrossum added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Sep 6, 2016
    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 7, 2016

    Here is the patch for PEP-526 implementation

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 7, 2016

    Oops, sorry, forgot to add new files when converting from git to hg, here is the full patch.

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 7, 2016

    Hey Ivan, Brett and I divided the review, he started at the bottom and I started at the top, we're meeting at Lib/typing.py.

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 7, 2016

    I played with the REPL, and found this:

    >>> del __annotations__
    del __annotations__
    >>> x: int = 0
    x: int = 0
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    NameError: __annotations__ not found
    >>> 

    I would expect this to re-create __annotations__.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 7, 2016

    We discussed this at some point on python/typing. At that time we decided that

    class C:
    del __annotations__
    x: int
    

    Should be an error. Do you think that it should behave in a different way in interactive REPL and/or at module level?

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    I could change STORE_ANNOTATION opcode so that it will recreate __annotations__ if __name__ == '__main__'.

    Or do you now think that it should re-create it always? I still think that always re-creating __annotations__ if they don't exist seems like silencing a possible error. As I mentioned in previous discussion, I think we should allow people to explicitly del __annotations__ (for example if someone wants to make a class with annotations that are "invisible" to runtime tools) and warn them if later they use annotations.

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 8, 2016

    Each statement at the REPL should re-initialize __annotations__ if it
    contains any annotations. I think this is how exec() already works. It
    adds __annotations__ to the namespace as needed, but just updates it
    if present. Inside a class it's different, that should be considered a
    single block.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    I think this is how exec() already works

    This is not exactly like this, exec() emits ast.Module while interactive input emits ast.Interactive (this one skips compiler_body in compile.c), but I think I have got your point, will fix this.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Guido,

    I fixed __annotations__ in interactive REPL, will fix other minor things and submit a new patch tomorrow morning.

    There are two important questions in typing.py:

    Why is the second isinstance() needed? Isn't _ClassVar a subclass of
    TypingMeta

    and

    This being a metaclass, the name should end in Meta, like most
    other subclasses of TypingMeta. (And they don't have a leading _,
    though they're still not public.)

    The problem with ClassVar is that it is not actually a class, and _ClassVar is not a metaclass, it is just a class. I deviated from the common pattern in module for the following reason. ClassVar is going to be extensively used at class scope, where all annotations are evaluated. Therefore I wanted not to create new class objects by __getitem__ like it is going for other classes like Union etc (I tried to timeit this and ClassVar is almost ten times faster than Union)

    What do you think?
    Should I keep it like this, or rewrite in a common pattern for the module?

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 8, 2016

    Oh, dang, I misread the definition of _ClassVar. It's fine then!
    Looking forward to the next installment.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Here is the new patch. I hope I didn't miss any comment and fixed everything.

    There is still a refleak to fix.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Sorry, again attached a wrong diff, here is the correct one.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    It looks like this part is causing a refleak

        def test_do_not_recreate_annotations(self):
            class C:
                del __annotations__
                try: #with self.assertRaises(NameError):
                    x: int
                except NameError:
                    pass

    in test_opcodes

    (for both options -- try and with)

    @1st1
    Copy link
    Member

    1st1 commented Sep 8, 2016

    It looks like this part is causing a refleak

    There is one DECREF in ceval that you commented out. I think it should actually be there. But it doesn't solve the problem.

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 8, 2016

    Yury will give you some help. Also, this patch no longer applies
    cleanly to hg. :-(

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Yury,

    Commenting out was an attempt to debug. It should be there

    @1st1
    Copy link
    Member

    1st1 commented Sep 8, 2016

    Ivan, take a look at my patch - i've fixed the refleak. It was in STORE_ANNOTATION opcode, you didn't DECREF ann consistently in all error branches. Also, you should never "break" in ceval -- only "goto error" or "DISPATCH()"

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Yury, thank you for the fix and for good advice!

    (I checked everything like 10 times, except for TOS :-)

    I run the full test suite and everything seem to be fine now.

    Guido, does Yury's patch apply cleanly, or I need to regenerate a new one?

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Oh, I see there are more comments by Serhiy, I will implement them and submit a new patch.

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 8, 2016

    Ivan, I have no idea how to integrate your patch and Yury's. I only use Mercurial here, I don't trust the github clone (how far behind is it?). Please sort this out yourself. I am also going through the review on rietveld again.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Guido,

    I resolved merge conflicts in patch v4 (if it will complain, this could be because of graminit.c or importlib_external.h, just ignore those, they will be regenerated during build).

    Please take a look and sorry for a delay.

    @1st1
    Copy link
    Member

    1st1 commented Sep 8, 2016

    Please take a look and sorry for a delay.

    Ivan, I'll be the one merging the patch. Will be looking over it soon. I might fix some nits myself, so if it applies cleanly to the default branch and all tests pass - then I'll handle the rest.

    Please also run the tests with -R3:3 (grammar/parser tests specifically)

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Thank you Yury,

    I usually do

    ./python -m test -R : test___all__ test_dis test_grammar test_opcodes test_parser test_pydoc test_symtable test_tools test_typing

    and then

    ./python -m test -j3 -u all

    I just run tests with -R3:3 it also works.

    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 8, 2016

    (I've renamed the patches so they line up with the numbering in the code review tool.)

    @1st1
    Copy link
    Member

    1st1 commented Sep 8, 2016

    Ivan, is "hg-pep-526-v5.diff" patch the one I can commit?

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 8, 2016

    Yes,

    This is the latest patch that I tested and with resolved merge conflicts.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2016

    New changeset 49533a9fe322 by Yury Selivanov in branch 'default':
    Issue bpo-27985: Implement PEP-526 -- Syntax for Variable Annotations.
    https://hg.python.org/cpython/rev/49533a9fe322

    @1st1
    Copy link
    Member

    1st1 commented Sep 9, 2016

    Committed. Congrats Ivan, it's a very serious contribution. Thank you.

    @1st1 1st1 closed this as completed Sep 9, 2016
    @gvanrossum
    Copy link
    Member Author

    gvanrossum commented Sep 9, 2016

    W00t! Thank Ivan for the code! And thanks Yury and Brett for the review.

    @ilevkivskyi
    Copy link
    Member

    ilevkivskyi commented Sep 9, 2016

    Thank you Guido, Yury, Brett, and Serhiy!

    It is first time I make such kind of contribution. It was a great experience and a great opportunity to better understand CPython internals (I already have several ideas on what to work next :-)

    I will soon submit a PR to python/typing with copy of typing changes (also Python2 version) and a patch to remove com2ann script to a separate repo as discussed with Guido.

    @1st1
    Copy link
    Member

    1st1 commented Sep 9, 2016

    You're welcome.

    It is first time I make such kind of contribution. It was a great experience and a great opportunity to better understand CPython internals (I already have several ideas on what to work next :-)

    If you need any help/mentoring please don't hesitate to approach me directly.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2016

    New changeset ef3d30cc6b4f by Gregory P. Smith in branch 'default':
    bpo-27985 - fix the incorrect duplicate class name in the lib2to3
    https://hg.python.org/cpython/rev/ef3d30cc6b4f

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants