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

Enable pattern matching by default #2757

Closed
isidentical opened this issue Jan 10, 2022 · 5 comments · Fixed by #2758
Closed

Enable pattern matching by default #2757

isidentical opened this issue Jan 10, 2022 · 5 comments · Fixed by #2758
Labels
C: target version Related to --target-version, e.g. autodetection T: enhancement New feature or request

Comments

@isidentical
Copy link
Collaborator

Now that the speed-up PR landed, it should be safe for us to enable pattern matching by default as the last grammar case to try. This would still make us initially test the 3.7+ and 3.0-3.6 grammars first before we try to run with the backtracking.

@isidentical isidentical added the T: bug Something isn't working label Jan 10, 2022
@isidentical
Copy link
Collaborator Author

(this was actually supposed be a feature request not a bug)

@felix-hilden felix-hilden added T: enhancement New feature or request and removed T: bug Something isn't working labels Jan 10, 2022
@isidentical
Copy link
Collaborator Author

@JelleZijlstra / @ichard26 / @felix-hilden what do you think about this? I recall there was some prior discussion on discord before, but that later got dismissed due to performance worries.

@JelleZijlstra
Copy link
Collaborator

I'm fine with doing this if we think it's stable and fast enough. Perhaps @ichard26's blackbench can help make the decision.

@isidentical
Copy link
Collaborator Author

As far as I recall, blackbench did not contain anything too heavy with match/case. So it might not be a good indicator. On regular invocations, it should not be really problematic (only when the file is already broken or using 3.10 syntax, since we add it as the last choice).

For some match/case heavy non PEP 634 code (hand crafted by @ichard26, example it is almost 1.7X slower (compared to 9X before). I'll try to see whether we could improve this even more!

@ichard26 ichard26 added the C: target version Related to --target-version, e.g. autodetection label Jan 13, 2022
@ichard26
Copy link
Collaborator

Yeah blackbench honestly needs a rewrite as its current design makes it hard to use / extend. I overengineered it unfortunately. I'd argue the performance slowdown is not that important at this point, it's quite low now and it should get even lower with PR #2763.

Anyway this issue is also important as its resolution would mean we could safely go ahead with making parenthesized with a 3.10 feature without risking the world, providing a resolution path for #1948.

Stability is probably the biggest question mark right now, but I'm fairly confident in @isidentical's implementation as it has passed their own suite of testing and also diff-shades. Short of doing some case crafting by hand, there's not much to explore (though we could always throw more code at it I suppose, in particular buggy code?). A rerun of diff-shades against the most recent implementation wouldn't hurt tho. I'd suspect remaining issues are only edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: target version Related to --target-version, e.g. autodetection T: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants