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

Fix bug when TypeOf is one of options in OneOf / AllOf #756

Merged
merged 2 commits into from
Aug 26, 2022

Conversation

MapleCCC
Copy link
Contributor

This PR should fix #528.

I also added unit test, to prevent regression.

@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 Aug 26, 2022
@MapleCCC
Copy link
Contributor Author

The reason I choose to disallow combination of AllOf and TypeOf, is because I think this is analogous to how the combination of AllOf and OneOf is disallowed. It seems natural to think of TypeOf and OneOf having similar semantic, although on different levels. Please feel free to let me know if you prefer other design.

@MapleCCC
Copy link
Contributor Author

Static type checkers didn't catch this bug for us. I wonder if we can do something in our code to help type checkers catch these bugs for us.

The logic in the matchers sub-system are large and complicated, making it hard for human developers to manually detect all kinds of logic bugs by eyes. It would be a huge gain if we manage to have type checkers help us here.

@zsol zsol merged commit fc622ce into Instagram:main Aug 26, 2022
@zsol
Copy link
Member

zsol commented Aug 26, 2022

Thanks! I agree with your sentiment, but don't see a way to express these kinds of constraints using Python's type system atm. Tests are our only tool here unfortunately

@MapleCCC MapleCCC deleted the fix-typeof-in-oneof-allof branch August 27, 2022 03:35
@MapleCCC
Copy link
Contributor Author

Thank you for the comment.

I am more optimistic on this though. I have a hunch that we can greatly improve type-check-ability here if we designate some of the types in libcst.matchers package to be so-called sealed type [1].

This feature is actively discussed in Python typing community recently [2]. Current workaround seems to be to use union type (a.k.a. sum type) to simulate sealness.

I will play with this idea a bit more, and try to see whether it brings outweighing benefits here.

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.

OneOf causes match to fail in some cases
3 participants