Support most Python 2 syntax back to 2.3 #275

thatch · 2020-03-27T16:12:50Z

Summary

This now parses a good amount of real-world setup.py code, and I've validated back to 2.3 (the earliest version I have handy). It passes most of python-grammar-changes with only a few remaining failures:

                                                  23 24 25 26 27 30 31 32 33 34 35 36 37 38
examples/py27-0024-as-keyword.py                  o. o. o  .. .. .. .. .  .. .  .. .. .. .. 
examples/py30-0004-unpacking-args1.py             o. o. o  o. o. .. .. .  .. .  .. .. .. .. 
examples/py30-0013-kwargs-after-star.py           .o .o .  oo oo oo oo o  oo o  oo oo oo oo 
examples/py30-0018-except-as.py                   .o .o .  oo oo oo oo o  oo o  oo oo oo oo 
examples/py30-0023-relative-imports1.py           .o .o o  oo oo oo oo o  oo o  oo oo oo oo 
examples/py30-0023-relative-imports2.py           .o .o o  oo oo oo oo o  oo o  oo oo oo oo

Both relative-imports and except-as are important. unpacking-args is a feature I haven't seen used since about 2004. The other two are for additional strictness to reject bad code, but are not necessary for my immediate needs.

Integer validation is reasonably strict at parse time, and handles both the octal changes and L suffix change.

                                                  23 24 25 26 27 30 31 32 33 34 35 36 37 38
examples/py30-0030-int-literals1.py               oo oo o  oo oo .. .. .  .. .  .. .. .. .. 
examples/py30-0030-int-literals2.py               oo oo o  oo oo .. .. .  .. .  .. .. .. .. 
examples/py30-0030-int-literals3.py               oo oo o  oo oo .. .. .  .. .  .. .. .. .. 
examples/py30-0030-int-literals4.py               .. .. .  oo oo oo oo o  oo o  oo oo oo oo 
examples/py30-0030-int-literals5.py               .. .. .  .. .. .. .. .  .. .  .. oo oo oo 
Legend:
 green header means will test with libcst

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

New classes have Py2-prefixed names, to make it a conscious decision for people so they don't accidentally using them when constructing trees by hand.

Test Plan

$ ../libcst-venv/bin/pyre
 ƛ No type errors found
$ tox -e py36
...
py36 run-test: commands[0] | python -m unittest
...
Ran 1687 tests in 85.662s

OK (skipped=3, expected failures=2)

This is relevant to #184

codecov-io · 2020-03-27T16:23:11Z

Codecov Report

Attention: Patch coverage is 74.08000% with 162 lines in your changes missing coverage. Please review.

Project coverage is 93.81%. Comparing base (31bae01) to head (3452f97).
Report is 439 commits behind head on main.

Files with missing lines	Patch %	Lines
libcst/_nodes/statement.py	52.50%	76 Missing ⚠️
libcst/_typed_visitor.py	79.51%	51 Missing ⚠️
libcst/_parser/conversions/statement.py	61.33%	29 Missing ⚠️
libcst/_nodes/expression.py	88.57%	4 Missing ⚠️
libcst/_parser/conversions/expression.py	94.11%	1 Missing ⚠️
libcst/_parser/parso/python/tokenize.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
- Coverage   94.30%   93.81%   -0.49%     
==========================================
  Files         232      233       +1     
  Lines       22442    23053     +611     
==========================================
+ Hits        21163    21627     +464     
- Misses       1279     1426     +147

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thatch · 2020-03-27T16:33:46Z

For completeness, here are future import not covered in the python-grammar-changes automated tests, which are still to do:

unicode_literals always enabled (needs to store a bit for evaluated_value to be correct on unprefixed strings, and probably some allowed escapes changes)
absolute_imports always enabled (which may require changes in the imports codemods)
with_statement always enabled (only affects python 2.5, this was always on in 2.6)

carljm

Did a run-through but would defer to @jimmylai for approval, my LibCST is a bit rusty.

Thanks for this work!

libcst/_nodes/expression.py

libcst/_nodes/statement.py

libcst/_parser/conversions/expression.py

carljm · 2020-04-23T04:46:05Z

Did another more careful run-through, this looks pretty good to me. In an ideal world I'd prefer to bump the test coverage up a bit; a number of the new Py2 nodes don't really get validation or codegen exercised at all by the test suite, and overall this brings test coverage down. Seems like your change-examples repo goes a long way to giving confidence in the correctness here, but I wish we could get more of that confidence built in to LibCST's own test suite so that future changes are validated too. But I'm also not that invested in Py2, either, so not going to block on that :)

Thanks for all the hard work here!

@jimmylai do you want me to hold off on merging so you can review this too, or just go ahead?

jimmylai · 2020-04-23T15:29:14Z

@jimmylai do you want me to hold off on merging so you can review this too, or just go ahead?

I can review this today. Maybe just wait a bit for me.

thatch · 2020-04-23T18:05:55Z

I will work on increasing coverage, if you want to wait a day...

jimmylai

A couple of new Node types (especially Py2Raise and Py2ExecTarget) and some new functions in statement.py don't have good test coverage.
https://codecov.io/gh/Instagram/LibCST/pull/275/diff

For documentation page, https://libcst.readthedocs.io/en/latest/nodes.html
(which is generated by https://github.com/Instagram/LibCST/blob/621d9a949a57a9100b7f2d1465ebd32aaeddb05c/docs/source/nodes.rst),
we can probably have another doc page for Python2 to prevent confusion. It's also better to include the Python versions availability in each node types in the doc by integrating information from
https://github.com/thatch/python-grammar-changes
That can be an enhancement as follow up PRs.

Otherwise looks great. Thanks for working on this.

jimmylai · 2020-04-24T05:56:49Z

.gitignore

@@ -13,3 +13,4 @@ build/
 .coverage
 .hypothesis/
 .pyre_configuration
+pip-wheel-metadata


How is this file generated?

I don't remember exactly, but pypa/pip#6213 says "pip wheel ." does it, and that seems reasonable.

libcst/_nodes/statement.py

jimmylai · 2020-04-24T06:01:13Z

libcst/_nodes/statement.py

+    expr: BaseExpression
+    target: Optional[Py2ExecTarget]
+
+    whitespace_after_exec: SimpleWhitespace = SimpleWhitespace.field(" ")
+
+    semicolon: Union[Semicolon, MaybeSentinel] = MaybeSentinel.DEFAULT


Those attributes miss some detailed Sphinx inline comments like other node type.

libcst/_nodes/statement.py

thatch · 2020-10-27T20:20:45Z

I'll try to rebase this over the holidays.

If this isn't of the expected type, the `.value` reference later would fail.

From testing, PEP 401 does not work in source files, only in the REPL, so this does not need to check for `barry_as_FLUFL`. Since the tokenization is unambiguous (`<`+`>` is not a valid sequence otherwise), we include it unconditionally.

Functionally this is an additional syntax to run `repr()`.

thatch · 2020-11-22T16:14:53Z

@jimmylai I think this is ready for another review. I fixed some docs and tests, and pyre is passing locally (but I don't entirely believe that). Is there an easy way to kick off a circleci run?

jimmylai · 2020-11-30T04:54:05Z

@jimmylai I think this is ready for another review. I fixed some docs and tests, and pyre is passing locally (but I don't entirely believe that). Is there an easy way to kick off a circleci run?

Circle CI shows all tests were passed. It automatically runs whenever you push an update to a PR. Yeah, I'll do another round of review.

jimmylai · 2020-11-30T05:05:31Z

Some functions are not covered by tests. Can you add some tests?

convert_py2_raise_stmt
convert_exec_stmt
Py2Exec. _codegen_impl
Py2ExecTarget. _validate

https://codecov.io/gh/Instagram/LibCST/pull/275/diff?src=pr&el=tree#diff-bGliY3N0L19faW5pdF9fLnB5

Did you try running this run on our large codebase through Fixit rules and unit tests? That can help us identify some issue from major changes like this PR.
Otherwise, LTGM.

facebook-github-bot · 2022-12-02T16:09:47Z

Hi @thatch!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

thatch requested a review from jimmylai March 27, 2020 16:12

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 27, 2020

carljm reviewed Mar 29, 2020

View reviewed changes

libcst/_nodes/expression.py Outdated Show resolved Hide resolved

libcst/_nodes/statement.py Outdated Show resolved Hide resolved

libcst/_nodes/statement.py Outdated Show resolved Hide resolved

libcst/_parser/conversions/expression.py Outdated Show resolved Hide resolved

thatch force-pushed the syntax-2.0 branch from 631f43e to 4eda6ce Compare April 22, 2020 18:09

jimmylai approved these changes Apr 24, 2020

View reviewed changes

thatch added 14 commits November 14, 2020 15:44

Ignore metadata written by pip install -e .

e2c2e85

Make type failure a little more obvious.

cfd90ee

If this isn't of the expected type, the `.value` reference later would fail.

[parsing 2.x] Support <> operator

c4cc252

From testing, PEP 401 does not work in source files, only in the REPL, so this does not need to check for `barry_as_FLUFL`. Since the tokenization is unambiguous (`<`+`>` is not a valid sequence otherwise), we include it unconditionally.

[parsing 2.x] Support exec and raise statements

216e85c

[parsing 2.x] allow passing versions down to 2.3

9176f60

[parsing 2.6] various literals and with statement

9ce447b

[parsing 2.x] L-suffix long int literals

87bd9e4

[parsing 2.x] support print statements

584854f

[parsing 2.x] correct detection of future imports

16e69c0

Codegen for print/raise

3457e14

[parsing 2.x] Handle backticks.

e120ec8

Functionally this is an additional syntax to run `repr()`.

[parsing 2.x] Disallow PEP-484 typehints on py2

96ef1cf

[parsing 2.x] Address review comments, regen, fix imports

538f8b1

Docs, better whitespace handling, codegen for py2 support

3452f97

thatch force-pushed the syntax-2.0 branch from 4eda6ce to 3452f97 Compare November 22, 2020 16:03

thatch requested review from jimmylai and carljm November 22, 2020 16:14

thatch marked this pull request as draft December 2, 2020 17:17

thatch mentioned this pull request Dec 20, 2021

Implement a Python PEG parser in Rust #566

Merged

zsol mentioned this pull request Dec 20, 2021

Compatibility with older python syntax #184

Closed

thatch closed this Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support most Python 2 syntax back to 2.3 #275

Support most Python 2 syntax back to 2.3 #275

thatch commented Mar 27, 2020 •

edited

Loading

codecov-io commented Mar 27, 2020 •

edited by codecov bot

Loading

thatch commented Mar 27, 2020

carljm left a comment

carljm commented Apr 23, 2020 •

edited

Loading

jimmylai commented Apr 23, 2020

thatch commented Apr 23, 2020

jimmylai left a comment

jimmylai Apr 24, 2020

thatch Nov 14, 2020

jimmylai Apr 24, 2020

thatch commented Oct 27, 2020

thatch commented Nov 22, 2020

jimmylai commented Nov 30, 2020

jimmylai commented Nov 30, 2020

facebook-github-bot commented Dec 2, 2022

Support most Python 2 syntax back to 2.3 #275

Support most Python 2 syntax back to 2.3 #275

Conversation

thatch commented Mar 27, 2020 • edited Loading

Summary

Test Plan

codecov-io commented Mar 27, 2020 • edited by codecov bot Loading

Codecov Report

thatch commented Mar 27, 2020

carljm left a comment

Choose a reason for hiding this comment

carljm commented Apr 23, 2020 • edited Loading

jimmylai commented Apr 23, 2020

thatch commented Apr 23, 2020

jimmylai left a comment

Choose a reason for hiding this comment

jimmylai Apr 24, 2020

Choose a reason for hiding this comment

thatch Nov 14, 2020

Choose a reason for hiding this comment

jimmylai Apr 24, 2020

Choose a reason for hiding this comment

thatch commented Oct 27, 2020

thatch commented Nov 22, 2020

jimmylai commented Nov 30, 2020

jimmylai commented Nov 30, 2020

facebook-github-bot commented Dec 2, 2022

Process

thatch commented Mar 27, 2020 •

edited

Loading

codecov-io commented Mar 27, 2020 •

edited by codecov bot

Loading

carljm commented Apr 23, 2020 •

edited

Loading