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

Implement PEP-634 - Match statement #568

Merged
merged 11 commits into from
Dec 30, 2021
Merged

Implement PEP-634 - Match statement #568

merged 11 commits into from
Dec 30, 2021

Conversation

zsol
Copy link
Member

@zsol zsol commented Dec 23, 2021

This PR implements the match statement in LibCST.

  • Python CST nodes to represent the new syntax
  • Code generation from Python CST nodes
  • Validation for Python CST nodes
  • Documentation for Python CST Nodes
  • Extend the Rust grammar to parse new syntax
  • Rust CST nodes to represent the new syntax
  • Code generation from Rust CST nodes
  • Round trip tests
  • Unit tests
  • Generate new matchers and types

@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 Dec 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2021

Codecov Report

Merging #568 (ab1a5da) into main (67db039) will increase coverage by 0.06%.
The diff coverage is 96.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
+ Coverage   94.65%   94.72%   +0.06%     
==========================================
  Files         240      241       +1     
  Lines       23579    24635    +1056     
==========================================
+ Hits        22319    23335    +1016     
- Misses       1260     1300      +40     
Impacted Files Coverage Δ
libcst/__init__.py 91.30% <ø> (ø)
libcst/matchers/_return_types.py 100.00% <ø> (ø)
libcst/_nodes/statement.py 95.25% <92.47%> (-0.90%) ⬇️
libcst/_typed_visitor.py 97.03% <97.68%> (+0.08%) ⬆️
libcst/_nodes/tests/test_match.py 100.00% <100.00%> (ø)
libcst/matchers/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67db039...ab1a5da. Read the comment docs.

@stroxler
Copy link
Contributor

@zsol did you want me to review this as well?

@zsol
Copy link
Member Author

zsol commented Dec 29, 2021 via email

@tushar-deepsource
Copy link

tushar-deepsource commented Dec 29, 2021

Hey! First of all, amazing work, pulling this off in such a short time.

I was testing this branch on my own test cases, and I've found a really peculiar bug:

# pylint: for some reason, having a comment starting with `pylint: ` is important for this bug to show up

match x:
    case y.z:
        pass

This is what happens when I try to parse this:

[...]
E           libcst._exceptions.ParserSyntaxError: Syntax Error @ 4:14.
E           parser error: error at 4:13: expected one of (, .
E           
E               case y.z:
E                        ^

venv3/lib/python3.10/site-packages/libcst/_parser/entrypoints.py:55: ParserSyntaxError

If I remove the comment on top, change y.z, or change the pylint: part to anything else, the error goes away.

@zsol
Copy link
Member Author

zsol commented Dec 29, 2021

Hey @tushar-deepsource thanks for trying this out! Can confirm I can repro the problem, but AFAICT the issue is with the y.z case matcher, not the pylint comment. The error doesn't go away even if I tinker (or completely remove) the pylint: comment. I'll fix this

@tushar-deepsource
Copy link

That seems to have fixed every edge case that I have in my codebase. Just one thing remains: something to do with trailing indent whitespace:

Python 3.10.0
>>> import libcst
>>> code = '''\
... if x == type(y):
...     pass
...     ''' # This indent whitespace is important
>>> libcst.parse_statement(code)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../libcst/_parser/entrypoints.py", line 138, in parse_statement
    result = _parse(
  File ".../libcst/_parser/entrypoints.py", line 55, in _parse
    return parse(source_str)
libcst._exceptions.ParserSyntaxError: Syntax Error @ 3:5.
parser error: error at 3:4: expected one of (, *, +, -, ..., AWAIT, DEDENT, False, NAME, NUMBER, None, True, [, break, continue, lambda, match, not, pass, ~

    pass
       ^
>>>

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Overall LGTM

I have a few nits on ordering that you could ignore, and I think one of the test fixtures could use more cases.

Match,
MatchCase,
MatchAs,
MatchClass,
Copy link
Contributor

Choose a reason for hiding this comment

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

notes to self:

  • Match is Match from Python.asdl, a variant of statement
  • MatchCase is match_case from Python.asdl
  • MatchPattern is presumably pattern type from Python.asdl
  • The following types don't appear to exist in Python.asdl, they are LibCST-only:
    • MatchClass, MatchKeywordElement, MatchList, MatchMappingElement, MatchTuple
  • Everything else is a variant of MatchPattern

Copy link
Member Author

Choose a reason for hiding this comment

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

Just between us: I mostly used the ast module as a reference instead of Python.asdl, the latter is not typed strongly enough for my taste

libcst/_nodes/statement.py Show resolved Hide resolved
libcst/_nodes/statement.py Outdated Show resolved Hide resolved
patterns: Sequence[Union["MatchSequenceElement", "MatchStar"]]

#: Parenthesis at the beginning of the node
lpar: Sequence[LeftParen] = field(default_factory=lambda: (LeftParen(),))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the factory really needed here?

I thought they were generally necessary only for mutable containers, which tuple isn't

libcst/_nodes/statement.py Outdated Show resolved Hide resolved
native/libcst/src/parser/grammar.rs Outdated Show resolved Hide resolved
native/libcst/src/parser/grammar.rs Show resolved Hide resolved
@zsol zsol merged commit 9932a6d into main Dec 30, 2021
@zsol zsol deleted the match-statement branch December 30, 2021 10:00
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.

5 participants