Skip to content
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

Support Python 3 syntax back to 3.0 #261

Merged
merged 13 commits into from
Mar 20, 2020
Merged

Conversation

thatch
Copy link
Contributor

@thatch thatch commented Mar 9, 2020

Summary

This adds support for machine-readable supported versions, and the minor changes necessary to support 3.0, 3.1, and 3.3. This includes future detection for future Python 2 parsing, but isn't required here given the PEP 401 findings.

Machine-readable versions are at libcst.KNOWN_PYTHON_VERSION_STRINGS.

Test Plan

New included tests, and ran against my python-grammar-changes project. Each entry should either have ".." or "oo" to say that the LibCST success matches that cpython version.

[tim@rygel python-grammar-changes]$ ../LibCST/.tox/py37/bin/python metarun.py
                                                  30 31 33 35 36 37 38
examples/py31-0002-with1.py                       oo oo oo oo oo oo oo 
examples/py31-0002-with2.py                       .. oo oo oo oo oo oo 
examples/py31-0002-with3.py                       .. oo oo oo oo oo oo 
examples/py33-0011-yield-from.py                  .. .. oo oo oo oo oo 
examples/py33-0012-uprefix1.py                    .. .. oo oo oo oo oo 
examples/py35-0008-else-in-call1.py               oo oo oo oo oo oo oo 
examples/py35-0008-else-in-call2.py               oo oo oo oo oo oo oo 
examples/py35-0009-async-def.py                   .. .. .. oo oo oo oo 
examples/py35-0010-dict-literals.py               .. .. .. oo oo oo oo 
examples/py36-0005-types1.py                      .. .. .. .. oo oo oo 
examples/py36-0006-fstrings.py                    .. .. .. .. oo oo oo 
examples/py36-0007-trailing-comma-def.py          .. .. .. .. oo oo oo 
examples/py36-0028-variable-annotations-pep526.py .. .. .. .. oo oo oo 
examples/py38-0003-walrus1.py                     .. .. .. .. .. .. oo 
examples/py38-0003-walrus2.py                     .. .. .. .. .. .. oo 
examples/py38-0003-walrus3.py                     .. .. .. .. .. .. .. 
examples/py38-0003-walrus4.py                     .. .. .. .. .. .. oo 
Legend:
 green header means will test with libcst

 first result is python, second [optional] result is libcst
 o   parses
 .   does not parse

@thatch thatch requested review from zsol and jimmylai March 9, 2020 16:28
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 9, 2020
@thatch
Copy link
Contributor Author

thatch commented Mar 9, 2020

This is the first half of #184

@thatch
Copy link
Contributor Author

thatch commented Mar 9, 2020

@jimmylai I could use another pair of eyes on the failing pyre test; I do not see what's different about test_matrix_multiply vs e.g. test_atom which has similar imports and the same test_versions signature.

Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'll let @jimmylai look it over as well - I'm not very familiar with the parser details here

@@ -33,8 +33,8 @@ A Concrete Syntax Tree (CST) parser and serializer library for Python

.. intro-start

LibCST parses Python 3.5, 3.6, 3.7 or 3.8 source code as a CST tree that keeps all
formatting details (comments, whitespaces, parentheses, etc). It's useful for
LibCST parses Python 3.0, 3.1, 3.3, 3.5, 3.6, 3.7 or 3.8 source code as a CST tree that keeps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would reword this to say

Suggested change
LibCST parses Python 3.0, 3.1, 3.3, 3.5, 3.6, 3.7 or 3.8 source code as a CST tree that keeps
LibCST parses source code compatible with all Python versions between 3.0 and 3.8 (inclusive) as a CST tree that keeps

Copy link
Contributor

@jimmylai jimmylai Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python 3.2 and 3.4 are not mentioned here, assuming they don't have new syntax change?
If that's the case, we can also add those versions as supported version in KNOWN_PYTHON_VERSION_STRINGS.

Copy link
Contributor Author

@thatch thatch Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimmylai there were no grammar changes in 3.2 or 3.4, so I didn't include them (they would be duplicates of the 3.1 and 3.3 grammar if allowed). My intent with KNOWN_PYTHON_VERSION_STRINGS is that you could do

for v in reversed(KNOWN_PYTHON_VERSION_STRINGS):
  try:
    mod = parse(...python_version=v)
    return mod
  except ParseError:
    pass

to see if it parses on any. I'm happy to add some helpers here (e.g. if you have a /usr/bin/pythonx.y and want to pick an ideal grammar version).

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use pyre-strict in every file including test files. So each parameter need to have type annotated.
Many of the added test functions miss no type annotations.
Here are pyre errors on my dev machine. Those need to be fixed.

libcst/_nodes/tests/test_atom.py:1040:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_atom.py:1040:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_atom.py:1040:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_dict.py:190:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_dict.py:190:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_dict.py:190:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_funcdef.py:2007:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_funcdef.py:2007:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_funcdef.py:2007:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_list.py:129:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_list.py:129:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_list.py:129:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_matrix_multiply.py:72:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_matrix_multiply.py:72:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_matrix_multiply.py:72:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_set.py:136:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_set.py:136:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_set.py:136:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_tuple.py:282:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_tuple.py:282:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_tuple.py:282:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_with.py:233:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_with.py:233:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_with.py:233:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.
libcst/_nodes/tests/test_yield.py:243:28 Missing parameter annotation [2]: Parameter `code` has no type specified.
libcst/_nodes/tests/test_yield.py:243:34 Missing parameter annotation [2]: Parameter `parser` has no type specified.
libcst/_nodes/tests/test_yield.py:243:42 Missing parameter annotation [2]: Parameter `expect_success` has no type specified.

@jimmylai
Copy link
Contributor

Regarding the weird type errors about parse_expression_as and parse_statement_as, it's a chick and egg situation.
pyre-check depends on libcst, so when we install pyre in the venv, an older version of libcst is also installed.
When you do pyre --search-path ${VIRTUAL_ENV}/lib/python*/site-packages check or pyre --preserve-pythonpath check, it read the old libcst and thus don't know the exist of parse_expression_as and parse_statement_as.

One way to fix this is to use the LibCST source of truth in the local repo.
By doing this, it installs libcst from local git repo.
pip uninstall libcst
pip install -e .
Then pyre understands new code and don't emit those errors. The missing parameter annotation errors still apply.

@thatch
Copy link
Contributor Author

thatch commented Mar 10, 2020

These changes should almost work, although CI will claim a failure and I don't think I can do anything about that. I only have two issues remaining when I run pyre on my machine:

libcst/codemod/_command.py:169:20 Undefined attribute [16]: Optional type has no attribute __getitem__.

@jimmylai
Copy link
Contributor

jimmylai commented Mar 11, 2020

These changes should almost work, although CI will claim a failure and I don't think I can do anything about that. I only have two issues remaining when I run pyre on my machine:

  • tokenize.Intnumber etc (there's an entry in stubs/ but python/typeshed#3839 should fix this for everyone)
  • argspec kwonlydefaults can be None:

libcst/codemod/_command.py:169:20 Undefined attribute [16]: Optional type has no attribute __getitem__.

I also see the type error in libcst/codemod/_command.py on my dev machine but it doesn't show up in CI. Not sure what's the cause but I've been ignoring it for a long time. It could be related to platform if you also develop on a Mac. (@DragonMinded who use Linux doesn't have this issue.)

@jimmylai
Copy link
Contributor

jimmylai commented Mar 11, 2020

Regarding the tokenize type error. The installed tokenize source code is not typed. When Pyre reads the source code, it generates the type errors. We used a stub file to help pyre understand the types https://github.com/Instagram/LibCST/blob/master/stubs/tokenize.pyi
I'm working on a fix to teach Pyre to read both the stubs and the site-packages correctly in #262

@thatch
Copy link
Contributor Author

thatch commented Mar 11, 2020

Regarding the tokenize type error. python/typeshed#3839 is merged, you shouldn't need the stub anymore if you --typeshed=/path/to/checkout too.

@jimmylai
Copy link
Contributor

jimmylai commented Mar 12, 2020

Looks like Pyre packs the typeshed. We can probably get your fix for free in the next release. https://github.com/facebook/pyre-check/tree/master/stubs

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My CI change is merged and you can rebase to see if the Pyre errors pass.
The helper function for automatically pick a good grammar version can be useful for some use case and can leave as a follow up.
Otherwise, LGTM.

@thatch thatch merged commit d2b86be into Instagram:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants