classification
Title: Implement PEP 526
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: gvanrossum, levkivskyi, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2016-09-06 23:48 by gvanrossum, last changed 2016-09-09 21:48 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
hg-pep-526.diff levkivskyi, 2016-09-07 01:04 (ignore) review
hg-pep-526-v2a.diff levkivskyi, 2016-09-07 01:12 review
hg-pep-526-v3.diff levkivskyi, 2016-09-08 17:46 (ignore) review
hg-pep-526-v4.diff levkivskyi, 2016-09-08 17:51 review
hg-pep-526-v4a.diff yselivanov, 2016-09-08 18:14
hg-pep-526-v5.diff levkivskyi, 2016-09-08 21:18 (Renamed to v5 to match code review labeling) review
Messages (33)
msg274672 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-06 23:48
Pending PEP 526's acceptance, I am inviting Ivan Levkivskyi to upload his patch here so it can be reviewed.
msg274693 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-07 01:04
Here is the patch for PEP 526 implementation
msg274695 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-07 01:12
Oops, sorry, forgot to add new files when converting from git to hg, here is the full patch.
msg274914 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-07 23:41
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.
msg274918 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-07 23:46
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__.
msg274919 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-07 23:49
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?
msg274925 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 00:04
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.
msg274929 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-08 00:16
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.
msg274930 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 00:22
> 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.
msg274938 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 00:59
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?
msg274954 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-08 02:15
Oh, dang, I misread the definition of _ClassVar. It's fine then!
Looking forward to the next installment.
msg275079 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 17:46
Here is the new patch. I hope I didn't miss any comment and fixed everything.

There is still a refleak to fix.
msg275080 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 17:51
Sorry, again attached a wrong diff, here is the correct one.
msg275081 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 17:56
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)
msg275083 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-08 18:00
> 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.
msg275085 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-08 18:00
Yury will give you some help. Also, this patch no longer applies
cleanly to hg. :-(
msg275088 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 18:02
Yury,

Commenting out was an attempt to debug. It should be there
msg275097 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-08 18:14
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()"
msg275115 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 18:59
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?
msg275117 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 19:03
Oh, I see there are more comments by Serhiy, I will implement them and submit a new patch.
msg275136 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-08 20:23
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.
msg275158 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 21:18
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.
msg275163 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-08 21:27
> 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)
msg275169 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 21:32
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.
msg275184 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-08 22:24
(I've renamed the patches so they line up with the numbering in the code review tool.)
msg275191 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-08 22:35
Ivan, is "hg-pep-526-v5.diff" patch the one I can commit?
msg275192 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-08 22:37
Yes,

This is the latest patch that I tested and with resolved merge conflicts.
msg275245 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-09 03:51
New changeset 49533a9fe322 by Yury Selivanov in branch 'default':
Issue #27985: Implement PEP 526 -- Syntax for Variable Annotations.
https://hg.python.org/cpython/rev/49533a9fe322
msg275246 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-09 03:54
Committed.  Congrats Ivan, it's a very serious contribution.  Thank you.
msg275247 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-09 03:55
W00t! Thank Ivan for the code! And thanks Yury and Brett for the review.
msg275265 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2016-09-09 06:07
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.
msg275267 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-09 06:29
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.
msg275458 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-09 21:48
New changeset ef3d30cc6b4f by Gregory P. Smith in branch 'default':
issue27985 - fix the incorrect duplicate class name in the lib2to3
https://hg.python.org/cpython/rev/ef3d30cc6b4f
History
Date User Action Args
2016-09-09 21:48:16python-devsetmessages: + msg275458
2016-09-09 06:29:23yselivanovsetmessages: + msg275267
2016-09-09 06:07:38levkivskyisetmessages: + msg275265
2016-09-09 03:55:27gvanrossumsetmessages: + msg275247
2016-09-09 03:54:03yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg275246

stage: needs patch -> resolved
2016-09-09 03:51:47python-devsetnosy: + python-dev
messages: + msg275245
2016-09-08 22:37:09levkivskyisetmessages: + msg275192
2016-09-08 22:35:30yselivanovsetmessages: + msg275191
2016-09-08 22:24:03gvanrossumsetmessages: + msg275184
2016-09-08 21:32:13levkivskyisetmessages: + msg275169
2016-09-08 21:27:19yselivanovsetmessages: + msg275163
2016-09-08 21:18:35levkivskyisetfiles: + hg-pep-526-v5.diff

messages: + msg275158
2016-09-08 20:23:32gvanrossumsetmessages: + msg275136
2016-09-08 19:03:43levkivskyisetmessages: + msg275117
2016-09-08 18:59:17levkivskyisetmessages: + msg275115
2016-09-08 18:14:29yselivanovsetfiles: + hg-pep-526-v4a.diff

messages: + msg275097
2016-09-08 18:02:56levkivskyisetmessages: + msg275088
2016-09-08 18:00:55gvanrossumsetmessages: + msg275085
2016-09-08 18:00:14yselivanovsetnosy: + yselivanov
messages: + msg275083
2016-09-08 17:56:24levkivskyisetmessages: + msg275081
2016-09-08 17:51:35levkivskyisetfiles: + hg-pep-526-v4.diff

messages: + msg275080
2016-09-08 17:46:24levkivskyisetfiles: + hg-pep-526-v3.diff

messages: + msg275079
2016-09-08 02:15:17gvanrossumsetmessages: + msg274954
2016-09-08 00:59:47levkivskyisetmessages: + msg274938
2016-09-08 00:22:07levkivskyisetmessages: + msg274930
2016-09-08 00:16:21gvanrossumsetmessages: + msg274929
2016-09-08 00:04:11levkivskyisetmessages: + msg274925
2016-09-07 23:49:46levkivskyisetmessages: + msg274919
2016-09-07 23:46:17gvanrossumsetmessages: + msg274918
2016-09-07 23:41:57gvanrossumsetmessages: + msg274914
2016-09-07 01:12:56levkivskyisetfiles: + hg-pep-526-v2a.diff

messages: + msg274695
2016-09-07 01:04:18levkivskyisetfiles: + hg-pep-526.diff

nosy: + levkivskyi
messages: + msg274693

keywords: + patch
2016-09-06 23:48:46gvanrossumcreate