| msg274672 - (view) |
Author: Guido van Rossum (gvanrossum) *  |
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) * |
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) * |
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 "", line 1, in 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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2016-09-09 03:54 |
| Committed. Congrats Ivan, it's a very serious contribution. Thank you. |
|
|
| msg275247 - (view) |
Author: Guido van Rossum (gvanrossum) * |
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) * |
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': - fix the incorrect duplicate class name in the lib2to3 https://hg.python.org/cpython/rev/ef3d30cc6b4f |
|
|