-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add typed_ast fast parser to mypy #1218
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
Conversation
@@ -120,6 +120,7 @@ def build(sources: List[BuildSource], | |||
pyversion: Tuple[int, int] = defaults.PYTHON3_VERSION, | |||
custom_typing_module: str = None, | |||
implicit_any: bool = False, | |||
fast_parser: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like there to be fewer boolean flags, not more. The original pattern is to define a string for each option (e.g. VERBOSE) in build and to add that string to the flags argument when the option is given. Then checking whether a flag is set can be done by asking whether e.g. VERBOSE in flags
. This way we can support many flags without a proliferation of boolean arguments. You might want to switch to this pattern for implicit_any too.
I switched |
('__debug__', bool_type), | ||
] | ||
|
||
for name, typ in literal_types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JukkaL this works properly with the real builtins but seems to cause some strange errors in unit tests. Do you know why that might be?
Can you be more specific about the failures you think are due to that code fragment in semanal.py? The test logs are completely full of tracebacks due to a missing get_line() on None. FWIW my guess it's due to the tests playing various tricks with custom builtins files. Many test data files contain a directive of the form |
I find it a little disconcerting that mypy no longer works with the latest version of typeshed (unless one also applies this PR). Could you make a separate PR to address that? It should just add |
If you don't think this PR is ready to be merged soon, I'll do that. My understanding was the main remaining issue was the build failures, which are related to the With regards to those failures: I'm a bit confused that they're happening, because |
Okay, I figured it out. All the fixtures mypy imports are imported as if they were builtins, so they all need to define bool. I hope one day we can make mypy fast enough that we'll no longer need these fixtures files -- they've caused me a lot of trouble so far. |
The current build failure is caused by some other typeshed change. Investigating. |
Honestly, I didn't review this PR very thoroughly because of the
failing tests. Given the nature of this PR I expect several more
rounds of review. Please make a separate PR with a small-as-posible
fix for the `True/False/__debug` issue -- it is already causing
problems validating other typeshed PRs (e.g.
python/typeshed#90).
|
Just completed a round of reading through the (For this PR, though, I don't think any of the details of that code are blockers; so long as this code is properly gated it won't break anything and it's a matter of reading this code itself.) |
I think the current blockers for this PR are:
I think they have to be resolved in that order, too. Also, thanks for taking a look Greg! I'll respond to your comments. |
0c80107
to
46c44df
Compare
Rebased this diff on the latest to mypy. I think it should be in a mergeable state now! @gvanrossum can you take look? |
46c44df
to
583c072
Compare
|
||
def from_operator(self, op): | ||
if isinstance(op, typed_ast.Add): | ||
return '+' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done faster and more compactly using a dict keyed by th various typed_ast classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. And doing a lookup on type(op)
? Yeah, I think that should work. I'll try it out.
Add typed_ast fast parser to mypy
To use the new fast parser, you install typed_ast and make these changes to your local typeshed, then run mypy with
--fast-parser
. Keep in mind that it currently only works on Python 3 code.This is not yet ready to be merged, as the typeshed being discussed here need to be resolved first.