[ruff] Add unnecessary-regular-expression-compile (RUF078) - #25533
anishgirianish wants to merge 9 commits into
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| unnecessary-regular-expression-compile | 81 | 81 | 0 | 0 | 0 |
| RUF078 | 2 | 2 | 0 | 0 | 0 |
…gex-compile # Conflicts: # crates/ruff_linter/src/codes.rs
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me overall! As usual, Codex came up with some edge cases that might be worth looking into.
First, I believe the binding.references() count is only counting textual references, so a case like this:
p = re.compile(...)
for x in range(100):
p.match(...)
only looks like one reference. This occurs in a couple of ecosystem projects:
- https://github.com/pandas-dev/pandas/blob/db9787c75fa4adba75db34f58f05542ec1a640a4/pandas/io/parsers/python_parser.py#L1471-L1480
- https://github.com/zulip/zulip/blob/d101bdb28b66aaa65bf7dfc351d807c948c88672/scripts/lib/check_rabbitmq_queue.py#L147-L159
Somewhat similarly, I don't think the rule is accounting for rebindings currently. So a conditional case like this can be incorrect:
pattern = Matcher()
if condition:
pattern = re.compile("a")
return pattern.match(value)This one is the least problematic, or at least seems the most rare, but we might also want to check for side effects in the re.compile arguments because these will only be evaluated once if the rule is followed. Something like this:
re.compile(get_pattern()).match(get_string())…gex-compile # Conflicts: # ruff.schema.json
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this is looking good. I found a few more edge cases and a possible small simplification.
ruff] Add unnecessary-regular-expression-compile (RUF077)
…gex-compile # Conflicts: # crates/ruff_linter/src/codes.rs
ruff] Add unnecessary-regular-expression-compile (RUF077)ruff] Add unnecessary-regular-expression-compile (RUF078)
Hi @ntBre , thank you so much for the review, I have updated pr resolving the feedbacks. Would like to request you for re-review, when ever you get a chance. Thank you |
|
Thanks for working on this and dealing with all my comments! I feel bad mentioning this in light of all the work you've put in, but what do you think about restricting the rule to the inline case rather than trying to track assignments and loops and such? This occurred to me today when reviewing again for a few reasons:
|
Summary
Adds
unnecessary-regular-expression-compile(RUF076, preview): flags are.compile()used exactly once inline (re.compile(p).match(s)) or via a single-use local, since the top-levelrefunctions are equivalent and skipthe throwaway object. Reused patterns (module/class-level, or locals read more than once) are left alone.
Closes #14691.
Test Plan
cargo test -p ruff_mdtest --test mdtest -- unnecessary-regular-expression-compile