Repository navigation
[complexity] Add unnecessary-regular-expression-compile - #25533
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| unnecessary-regular-expression-compile | 7 | 7 | 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:
|
|
Hi @ntBre, thank you so much for the re-review. I've restricted the rule to the inline |
|
Hi @ntBre, whenever you have a free moment I'd love your thoughts on the latest changes. Thanks so much! |
ntBre
left a comment
There was a problem hiding this comment.
Thank you! And sorry for the delay. This looks good to me, I just had a couple of small suggestions remaining.
ruff] Add unnecessary-regular-expression-compile (RUF078)ruff] Add `unnecessary-regular-expression-compile
ruff] Add `unnecessary-regular-expression-compileruff] Add unnecessary-regular-expression-compile
|
Hi @ntBre, thank you so much for re-review. I have pushed the changes resolving feedback's in the latest commit. Would like to request you for your re-review, whenever you get a chance. Thank you |
ruff] Add unnecessary-regular-expression-compilecomplexity] Add unnecessary-regular-expression-compile
Summary
Adds
unnecessary-regular-expression-compile(RUF078, preview). It flagsre.compile(p).match(s)and similar, where the compiled pattern is only ever used right there. In that casere.match(p, s)does the same thing without the extra object. Patterns stored in a variable are left alone, since that usually signals the pattern is meant to be reused.Closes #14691.
Test Plan
cargo test -p ruff_mdtest --test mdtest -- unnecessary-regular-expression-compile