homepage

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

Here is the patch for PEP 526 implementation

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

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.

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

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?

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.

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.

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

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?

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

Here is the new patch. I hope I didn't miss any comment and fixed everything.
There is still a refleak to fix.

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

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)

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

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

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

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()"

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?

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

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.

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.

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

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.

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

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

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

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 

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

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

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.

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.

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