Make fast parser handles type annotations + type comments better

[afrieder]

This PR has two parts:

1. Disallows two return annotations (ie addresses Fast parser does not error on functions with both type comments and type annotations #2733)
2. Enforces that type annotations and the type comment are consistent.

I assume 1 is not controversial, but 2 might be.

The point of it is to prevent:

def f(x: str):
  # type: (int) -> int
  return x

which is currently perfectly allowed.

Thus, this code checks, for every argument with a type annotation, that the type in the type comment is identical.
The way the code works, # type: (...) -> int will always pass.
The reason this might controversial is that it does string comparison, so it's not always right. In fact, it's very simple to get a false positive:

def f(x: Union[int, str]):
  # type: (Union[str, int]) -> int
  return 5

will now complain about an inconsistent type signature.

Now, I feel this is fine, since if you're going to go through the trouble of using both a type annotation and a non ... type comment, then you might as well make them the same. The only reason this might be an actual problem is for something like:

T = Union[int, str]
def f(x: Union[int, str]):
  # type: (T) -> int
  return 5

Which fails. Because we only keep a single source of truth for what the arg types are, we cannot postpone this check until we have actual types without a large and unnecessary (imo) refactor.

So in summary, I think it's perfectly fine to insist that if users are going to use both type annotations and type comments in a function that they match identically.
But if the Powers That Be disagree, I'll happily remove the second part from this PR.

[afrieder]

I should also add that because of how the typed AST handles type comments,

def f(x: int):
  return x

and

def f(x  # type: int
   ):
  return x

are indistinguishable.

This is... annoying, because in the old parser

def f(x  # type: int
  ):
  # type: (...) -> int
  return x

is allowed, while

def f(x: int):
  # type: (...) -> 5
  return x

is not.

Since we cannot replicate this behavior, I chose to just enforce that the two methods of annotating convey the same type information.

[ddfisher]

Thanks for the PR! To my understanding, there's no reason anyone should be mixing type comments and type annotations, so it would be better to disallow mixing them entirely rather than to check for consistency. I agree that it would be better if typed_ast distinguished between per-argument type comments and type annotations, but I don't think that's hugely important. Because of that limitation, let's just disallow type annotations for all non-ellipsis-parameter type comments.

[ddfisher]

(see comment above)

[gvanrossum]

Indeed. PEP 484 says "It is not allowed for an argument or return value to have both a type annotation and a type comment"

[ddfisher]

Thanks for the update! Just one tiny nit.

[ddfisher]

It's slightly better to explicitly check that a.type_annotation is not None than just to check the truthiness here.

[afrieder]

Fixed (in both places).
Completely unrelated, sorry about clogging AppVeyor.

[ddfisher]

(and here also)

[afrieder]

Cleaned up the commits, should be good to go? It's all much cleaner now.

[ddfisher]

Looks good, thanks! I'll merge once the tests finish running.

FYI we do squash merges on PRs, so (aside from having things be understandable during review) you don't need to worry about a nice commit history.

[afrieder]

Ack.
It was purely for review legibility.