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 "<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) *  |
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':
issue27985 - fix the incorrect duplicate class name in the lib2to3
https://hg.python.org/cpython/rev/ef3d30cc6b4f
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:35 | admin | set | github: 72172 |
2016-09-09 21:48:16 | python-dev | set | messages:
+ msg275458 |
2016-09-09 06:29:23 | yselivanov | set | messages:
+ msg275267 |
2016-09-09 06:07:38 | levkivskyi | set | messages:
+ msg275265 |
2016-09-09 03:55:27 | gvanrossum | set | messages:
+ msg275247 |
2016-09-09 03:54:03 | yselivanov | set | status: open -> closed resolution: fixed messages:
+ msg275246
stage: needs patch -> resolved |
2016-09-09 03:51:47 | python-dev | set | nosy:
+ python-dev messages:
+ msg275245
|
2016-09-08 22:37:09 | levkivskyi | set | messages:
+ msg275192 |
2016-09-08 22:35:30 | yselivanov | set | messages:
+ msg275191 |
2016-09-08 22:24:03 | gvanrossum | set | messages:
+ msg275184 |
2016-09-08 21:32:13 | levkivskyi | set | messages:
+ msg275169 |
2016-09-08 21:27:19 | yselivanov | set | messages:
+ msg275163 |
2016-09-08 21:18:35 | levkivskyi | set | files:
+ hg-pep-526-v5.diff
messages:
+ msg275158 |
2016-09-08 20:23:32 | gvanrossum | set | messages:
+ msg275136 |
2016-09-08 19:03:43 | levkivskyi | set | messages:
+ msg275117 |
2016-09-08 18:59:17 | levkivskyi | set | messages:
+ msg275115 |
2016-09-08 18:14:29 | yselivanov | set | files:
+ hg-pep-526-v4a.diff
messages:
+ msg275097 |
2016-09-08 18:02:56 | levkivskyi | set | messages:
+ msg275088 |
2016-09-08 18:00:55 | gvanrossum | set | messages:
+ msg275085 |
2016-09-08 18:00:14 | yselivanov | set | nosy:
+ yselivanov messages:
+ msg275083
|
2016-09-08 17:56:24 | levkivskyi | set | messages:
+ msg275081 |
2016-09-08 17:51:35 | levkivskyi | set | files:
+ hg-pep-526-v4.diff
messages:
+ msg275080 |
2016-09-08 17:46:24 | levkivskyi | set | files:
+ hg-pep-526-v3.diff
messages:
+ msg275079 |
2016-09-08 02:15:17 | gvanrossum | set | messages:
+ msg274954 |
2016-09-08 00:59:47 | levkivskyi | set | messages:
+ msg274938 |
2016-09-08 00:22:07 | levkivskyi | set | messages:
+ msg274930 |
2016-09-08 00:16:21 | gvanrossum | set | messages:
+ msg274929 |
2016-09-08 00:04:11 | levkivskyi | set | messages:
+ msg274925 |
2016-09-07 23:49:46 | levkivskyi | set | messages:
+ msg274919 |
2016-09-07 23:46:17 | gvanrossum | set | messages:
+ msg274918 |
2016-09-07 23:41:57 | gvanrossum | set | messages:
+ msg274914 |
2016-09-07 01:12:56 | levkivskyi | set | files:
+ hg-pep-526-v2a.diff
messages:
+ msg274695 |
2016-09-07 01:04:18 | levkivskyi | set | files:
+ hg-pep-526.diff
nosy:
+ levkivskyi messages:
+ msg274693
keywords:
+ patch |
2016-09-06 23:48:46 | gvanrossum | create | |