ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Support pattern matching

Open sobolevn opened this issue 2 years ago • 11 comments

As I am learning the source code, it seems to me that python's pattern matching is not supported at all.

We have a visitor ready: https://github.com/charliermarsh/ruff/blob/c7349b69c12b7511d48fc45390eac8d7282062a5/src/ast/visitor.rs#L527-L571

But, that's about it.

Things to support (feel free to update):

  • [ ] Unused variables check
  • [ ] Variable names check (MatchAs pattern)
  • [ ] Custom rules, like do not match floats

sobolevn avatar Sep 29 '22 09:09 sobolevn

Yeah it makes some appearances in the RustPython AST but it doesn't seem to be supported yet by the actual parser.

charliermarsh avatar Sep 29 '22 19:09 charliermarsh

The parser in question is rustpython-parser, correct? No promises, but I can take a look at its current state and see whether I can contribute support for the match syntax.

woodruffw avatar Jan 10 '23 04:01 woodruffw

Oh, looks like someone beat me to it :slightly_smiling_face: https://github.com/RustPython/RustPython/pull/4323

woodruffw avatar Jan 10 '23 04:01 woodruffw

Yeah that's the one! There's been a little bit of progress in that PR. There's also someone who's been looking to migrate the entire parser to rust-peg (https://github.com/RustPython/RustPython/pull/4423) which would be a much bigger change. Then @andersk had augmented the parser to support parenthesized context managers (https://github.com/RustPython/RustPython/pull/4352) and had mentioned perhaps looking into match statements next.

charliermarsh avatar Jan 10 '23 04:01 charliermarsh

I'd love to sponsor someone (financially, that is) to work on this issue (in RustPython). If you're interested, shoot me a DM on Twitter or email me at [email protected] :)

charliermarsh avatar Jan 14 '23 15:01 charliermarsh

Actually when there is a match expression in a python module, ruff fails silently? I spent an hour debugging why ruff isn't sorting my imports even though the iSort rules are enabled, when I commented out the match expression ruff was able to sort again, is that expected?

JadHADDAD92 avatar Jan 31 '23 10:01 JadHADDAD92

@JadHADDAD92 - So sorry that you lost time to this. It is the expected behavior, because syntax errors have their own error code (E999). So if you don't have that code enabled (e.g., if you do ruff foo --select I), you won't see those errors at all. It's consistent with how Ruff works, but I agree it's confusing. (Flake8 does have the same behavior.)

A few alternatives:

  1. Always log an error (separate from a lint violation) if we fail to parse a file. It's hard to imagine this being unhelpful ever.
  2. Always include E999 even if it's not explicitly enabled. This is tricky and complicates the rule-selection semantics.

charliermarsh avatar Jan 31 '23 12:01 charliermarsh

I find the first alternative to be the better behavior, because I presume all ruff rules depend on a successful parse of RustPython (right?) therefore if we lint a module which contains a syntax error (failed to parse), and ruff returns 0 as if the file is well linted, it brings no clarity of what happened.

JadHADDAD92 avatar Jan 31 '23 12:01 JadHADDAD92

Not all rules depend on a successful parse. Some can work off the token stream, or even partial stream. Some rules only look at raw lines (like the line-length violations). And some work off the filesystem (e.g., the rule to detect missing __init__.py files). The majority do, but some rules can complete without one.

We could: always log an error message if a file fails to parse + exit 1.

charliermarsh avatar Jan 31 '23 12:01 charliermarsh

@charliermarsh is there an issue for this (logging an error when failing to parse) that I can subscribe to?

JadHADDAD92 avatar Feb 02 '23 09:02 JadHADDAD92

@JadHADDAD92 - No but thank you for the reminder -- I just created #2473, hopefully I can knock it out today.

charliermarsh avatar Feb 02 '23 13:02 charliermarsh

What direction are we going in order to support this?

ofek avatar Feb 11 '23 17:02 ofek

I'm working on it here: https://github.com/RustPython/RustPython/pull/4519. There are a few problems to solve but that implementation already supports many of the variants.

charliermarsh avatar Feb 18 '23 04:02 charliermarsh

I'm making good progress on this (I think?). If anyone wants to link to an OSS codebase that makes good use of match statements (for testing), it'd be appreciated.

charliermarsh avatar Feb 19 '23 20:02 charliermarsh

@charliermarsh there are good match examples in the 2nd edition Fluent Python book. This snippet looks like it would catch some edge cases. In fact, if you search in that repo for the word “match” you’ll get examples of that word being used as a keyword as well as a normal variable.

phillipuniverse avatar Feb 19 '23 21:02 phillipuniverse

Pyright has some good examples. https://github.com/microsoft/pyright/blob/main/packages/pyright-internal/src/tests/samples/match1.py for example or any other match*.py file in that directory.

SRv6d avatar Feb 19 '23 21:02 SRv6d

Also test if match := re.search(...):

ofek avatar Feb 19 '23 22:02 ofek

Python's own test suite for pattern matching seems like a good litmus test: https://github.com/python/cpython/blob/main/Lib/test/test_patma.py

https://github.com/brandtbucher/patmaperformance also has some examples.

henribru avatar Feb 19 '23 22:02 henribru

Parser looking good so far. I got it to parse Black's fixtures which cover a lot of tricky cases.

charliermarsh avatar Feb 19 '23 23:02 charliermarsh

(It also handles soft keywords as pointed out by @ofek. I've also run it over Hatch, Airflow, and Pandas, and confirmed that the output is unchanged, so it's not incorrectly detecting any match statements from e.g. variable assignments.)

charliermarsh avatar Feb 19 '23 23:02 charliermarsh

(Oh, I guess the Black test suite is a subet of test_patma.py :))

charliermarsh avatar Feb 19 '23 23:02 charliermarsh

I expect this to go out some time this week, hopefully in the next few days. The PR is up as #3047 (plus depends on https://github.com/RustPython/RustPython/pull/4519 in RustPython).

charliermarsh avatar Feb 20 '23 04:02 charliermarsh

Thank you!

Heads up for anyone else using the VS code extension you will need to set your ruff path:

"ruff.path": ["./.venv/bin/ruff"]

Looks like the extension has it's own ruff executable, which has already been updated to use the latest ruff but hasn't been released yet.

danstewart avatar Feb 22 '23 11:02 danstewart

@danstewart most likely you want this instead:

"ruff.importStrategy": "fromEnvironment"

which will find ruff in whatever the venv associated with the vscode workspace is, without having to hardcode the path.

mikeroll avatar Feb 22 '23 11:02 mikeroll

Thanks, that's much better.

danstewart avatar Feb 22 '23 11:02 danstewart

@danstewart - I cut a new pre-release last night that includes v0.0.251, so you can pull it in if you switch to the pre-release channel. I'll probably cut a main-channel release today or tomorrow.

charliermarsh avatar Feb 22 '23 14:02 charliermarsh