-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Consider match-case stmts for C901
, PLR0912
, and PLR0915
#11521
Conversation
match a: | ||
case 30: | ||
print('foo') | ||
case _: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure about this, the equivalent else statement
has the following check:
if clause.test.is_some() {
complexity += 1;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what you're not sure about? Is it that the catch-all case (_
) acts as an else
block of an if
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it that the catch-all case (_) acts as an else block of an if statement?
Yep, complexity wise to me it's equivalent to the following if else
statement.
if x == 30:
print("foo")
else:
print("bar")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't they equivalent? We'd consider the above if else
statement with complexity 2 and similarly for the following match statement:
match subject:
case foo: ...
case _: ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if else
statement has a complexity of 1, the &clause.test.is_some()
evaluates to false for the else
.
[crates/ruff_linter/src/rules/mccabe/rules/function_is_too_complex.rs:413:9] &source = "\nif x == 3:\n print(\"foo\")\nelse:\n print(\"bar\")\n "
[crates/ruff_linter/src/rules/mccabe/rules/function_is_too_complex.rs:79:21] &clause.test.is_some() = false
[crates/ruff_linter/src/rules/mccabe/rules/function_is_too_complex.rs:415:9] &get_complexity_number(&stmts) = 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. Hmm, that's interesting. Yeah, I think we could do that i.e., check if the last case
pattern is either a _
or any variable as both act as a catch-all pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only _
acts as a wild card pattern if my understanding of the documentation is correct. In any case, I can make a follow up PR to handle this. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only
_
acts as a wild card pattern if my understanding of the documentation is correct.
Yes, although a name pattern would also act in a similar way except that it also binds the value to the variable. So, in the following example foo
will be same as subject
. It's like doing foo = subject
.
match subject:
case 2: ...
case foo: ...
You can even see that in CPython where it raises a syntax error similar to _
if the name pattern is not the last pattern:
match 1:
case x:
print(x)
case 2:
...
Running python test.py
:
$ python3.13 parser/_.py
File "/Users/dhruv/playground/ruff/parser/_.py", line 2
case x:
^
SyntaxError: name capture 'x' makes remaining patterns unreachable
In any case, I can make a follow up PR to handle this. :)
Sounds good.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
crates/ruff_linter/src/rules/mccabe/rules/function_is_too_complex.rs
Outdated
Show resolved
Hide resolved
C901
, PLR0912
, and PLR0915
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
## Summary Follow up to #11521 Removes the extra added complexity for catch all match cases. This matches the implementation of plain `else` statements. ## Test Plan Added new test cases. --------- Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Resolves #11421
Summary
Instead of counting match/case as one statement, consider each
case
as a conditional.Test Plan
cargo test