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 Variable Annotations Syntax #2131

Merged
merged 19 commits into from Sep 28, 2016

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Sep 14, 2016

@gvanrossum @ddfisher @JukkaL

Here is an accompanying change for python/typed_ast#16

Github says it cannot automatically merge, probably because I updated type of ast35.Assign.type_comment to Union[AST, Optional[str]] in typeshed. I don't know how to fix this.
I have made a PR to typeshed with this commit.

All mypy tests pass locally with both versions (current and PEP 526) of typed_ast.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 14, 2016

Maybe the merge conflict is due to the typeshed update. Try leaving that out.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 14, 2016

@gvanrossum
You are right, I have leaved typeshed update, and now everything is OK.

But don't forget that actual implementation is in python/typed_ast#16

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 14, 2016

So this works... A little too well! It now also accepts the new syntax in Python 3.3--3.5.

Also, maybe 'ast35' is now a misnomer? (That's mostly a Q. for @ddfisher who's out this week.)

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 14, 2016

Could you add some unit tests too?

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 14, 2016

@gvanrossum

So this works... A little too well! It now also accepts the new syntax in Python 3.3--3.5.

The simplest way to fix this is to make two modules ast35 and ast36. ast36 will accept PEP 526 (possibly also async generators and async comprehensions). ast35 will need to be changed only slightly to have the same type Str for type_comment, (and some changes due to how None is treated).

Could you add some unit tests too?

OK, will do this. I just need to figure out first how to skip tests depending on version of typed_ast.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 14, 2016

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 14, 2016

@gvanrossum

I'd rather not have the ast modules multiplying like rabbits...

I agree, I will add version dependent checks.

I have not yet finished the new semantics of prohibiting x: int = None. For some reason it is still not flagged inside functions.

@@ -1076,7 +1076,8 @@ def check_assignment(self, lvalue: Node, rvalue: Node, infer_lvalue_type: bool =
elif (is_literal_none(rvalue) and
isinstance(lvalue, NameExpr) and
isinstance(lvalue.node, Var) and
lvalue.node.is_initialized_in_class):
lvalue.node.is_initialized_in_class and
not experiments.STRICT_OPTIONAL):

This comment has been minimized.

@ddfisher

ddfisher Sep 21, 2016

Collaborator

If we don't want this behavior, the check should just be deleted because Nones are subtypes of all other types when not in strict Optional mode, so this check is unnecessary.

That said, I think what we really want is to update this check to be more permissive rather than doing AST replacement at the parser stage.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2016

Collaborator

The idea here is to prohibit things like x: int = None. Probably you are right that this could be done by just removing the check. I will try this and if everything will work, I just remove this check in the next commit.

Concerning the AST replacement in the parser, see my last comment below.

def replace_none(n: Node) -> Node:
"""
Helper for parse and fastparse.
Replace all Nones on the right of protected assignment with Any-like.

This comment has been minimized.

@ddfisher

ddfisher Sep 21, 2016

Collaborator

I don't see the term "protected assignment" in PEP 484. Could you define it here?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2016

Collaborator

@ddfisher
Currently mypy complains about assignments like this

x = None  # type: int

with --strict-optional. Guido wants this to be changed. I use the term "protected" here with meaning "protected against complains". Since when someone writes the code as above the reason is not to actually assign None, this is just a limitation of the old syntax.

I will add this explanation to the docstring in the next commit.

rvalue = self.visit(n.value)
lvalues = self.visit_list(n.targets)
# protect 'x, y = None, None # type: int, str' etc.
if not n_synt and typ is not None and experiments.STRICT_OPTIONAL:

This comment has been minimized.

@ddfisher

ddfisher Sep 21, 2016

Collaborator

This understanding should be part of a later stage rather than in the parser. It's better if (as much as possible) the parser faithfully reports the AST of the actual code.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2016

Collaborator

@ddfisher
The idea here is to synchronize the outcome of the old syntax and the new syntax. Guido wanted (and I agree with him) the outcome of these two to be identical:

x: int
x = None  # type: int

The easiest way is to make them equivalent at this stage (if someone wants a more precise information it is still provided by typed_ast). Otherwise there will be checks all the way down whether a particular None is a "real" one or just a limitation of the old syntax.

This comment has been minimized.

@ddfisher

ddfisher Sep 21, 2016

Collaborator

The None is a real one, though, and you can't be sure that it's just being used due to limitations of the old syntax -- it maybe intended to be used as an actual assignment. Also, I don't think you'll need check in a bunch of different places. I think modifying the if statement I commented on in checker.py should be sufficient.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 21, 2016

Collaborator

@ddfisher @gvanrossum
I am not 100% sure about the interpretation of the old syntax. However, as I understood, Guido is sure. And he wants this

x = None  # type: int

to be interpreted as a not real None.

Concerning implementation I will just try to move it out of here and if everything works then OK.

@ddfisher

This comment has been minimized.

Collaborator

ddfisher commented Sep 21, 2016

I'd rather not have the ast modules multiplying like rabbits... The plan
has always been to accept the most recent syntax at the grammar level and
to complain later in the pipeline.

As I recall, the plan was for the ast modules to multiply. That's why ast35 was named ast35 instead of ast3. New Python versions aren't released that often, and I'm a bit reluctant to e.g. include beta code in the ast35 parser. I also think it's valuable for clients to be able to parse specific versions of Python, though I'm open to being convinced that's not the case.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 21, 2016

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 21, 2016

@gvanrossum @ddfisher

at least not as part of this PR

OK, I agree, let us consider this separately.
I will then only leave the necessary changes in the PR in the next commit.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 21, 2016

@ddfisher
OK, I pushed a new commit. Now only the PEP 526 related stuff is in. There is only one line added apart from fastparse.py and tests: The special-casing of class bodies in checker.py will not affect the new syntax, so that x: int = None will be an error everywhere.

If you don't like this, then I could just remove this special-casing altogether. Please take a look.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 21, 2016

@ddfisher

Looks good -- just a couple small things left to fix. Thanks!

@@ -1076,7 +1076,8 @@ def check_assignment(self, lvalue: Node, rvalue: Node, infer_lvalue_type: bool =
elif (is_literal_none(rvalue) and
isinstance(lvalue, NameExpr) and
isinstance(lvalue.node, Var) and
lvalue.node.is_initialized_in_class):
lvalue.node.is_initialized_in_class and
not self.options.fast_parser):

This comment has been minimized.

@ddfisher

ddfisher Sep 26, 2016

Collaborator

This isn't correct -- it disallows the following (when run with the fast parser):

class C:
    x = None # type: int

Instead, I think you'll need to add a flag for the new assignment syntax to AssignmentStmt and check for that here.

@@ -67,6 +68,11 @@
'check-newtype.test',
]
if hasattr(typed_ast, '__version__'):
version_info = tuple(map(int, typed_ast.__version__.split('.'))) # type: ignore
if version_info >= (0, 5, 7):

This comment has been minimized.

@ddfisher

ddfisher Sep 26, 2016

Collaborator

python/typed_ast#16 contains an interface change, so it's actually going to be version 0.6.0 -- could you bump that here?

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 26, 2016

@ddfisher
Thank you for review! Will fix this soon (I am on a train right now).

@ddfisher

Thanks for fixing this up! Just a few small nits remain.

else:
typ = TypeConverter(line=n.lineno).visit(n.type_comment)
'comment instead', n.lineno, n.col_offset)
if n.type_comment is not None:

This comment has been minimized.

@ddfisher

ddfisher Sep 28, 2016

Collaborator

We should display an error if there's a type comment and a type annotation.

@@ -761,12 +761,14 @@ class AssignmentStmt(Statement):
rvalue = None # type: Expression
# Declared type in a comment, may be None.
type = None # type: mypy.types.Type
n_synt = False # type: bool

This comment has been minimized.

@ddfisher

ddfisher Sep 28, 2016

Collaborator

Could you add a comment about what this means? Also, I think new_syntax would be slightly clearer than the abbreviated n_synt.

@@ -67,6 +68,11 @@
'check-newtype.test',
]
if hasattr(typed_ast, '__version__'):
version_info = tuple(map(int, typed_ast.__version__.split('.'))) # type: ignore
if version_info >= (0, 6, 0):

This comment has been minimized.

@ddfisher

ddfisher Sep 28, 2016

Collaborator

It'd be better to do a functionality check instead of this version check (especially now that typed_ast no longer exports a version). You could do so like this:

import typed_ast.ast35
if 'annotation' in typed_ast.ast35.Assign._fields: ...
raise TypeCommentParseError('Variable annotation syntax is only '
'suppoted in Python 3.6, use type '
'comment instead', n.lineno, n.col_offset)
if n.type_comment is not None:

This comment has been minimized.

@ddfisher

ddfisher Sep 28, 2016

Collaborator

Could you add a comment noting that typed_ast prevents there from being both a type comment and an annotation?

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 28, 2016

@ddfisher
I implemented the latest comments here and in typeshed PR. Concerning both type comment and type annotation, typed_ast prohibits this with

SyntaxError: misplaced type annotation

so that I just added a comment in fastparse.py

@ddfisher

This comment has been minimized.

Collaborator

ddfisher commented Sep 28, 2016

Ah, yep. I meant to delete the comment about it when I noticed it wasn't possible, but I guess I forgot.

Thanks for responding to all the feedback! This looks good now!

@ddfisher ddfisher merged commit 5f0d02c into python:master Sep 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment