-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 irrefutable pattern similar to if .. else
for C901
#11565
Conversation
|
@@ -96,8 +97,16 @@ fn get_complexity_number(stmts: &[Stmt]) -> usize { | |||
complexity += get_complexity_number(orelse); | |||
} | |||
Stmt::Match(ast::StmtMatch { cases, .. }) => { | |||
for case in cases { | |||
let last_index = cases.len() - 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 we use saturating_sub
here to avoid a panic for match a:
. This is not valid python but the parser might soon be able to recover from incomplete statements.
let last_index = cases.len() - 1; | |
let last_index = cases.len().saturating_sub(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.
Went with Dhurv's suggestion here.
complexity += 1; | ||
if let Pattern::MatchAs(match_as_pattern) = &case.pattern { | ||
if match_as_pattern.pattern.is_none() && i == last_index { |
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.
What's the reason that we restrict the logic to only when the _
pattern is last? Can't we always subtract the complexity in that 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.
Because an irrefutable pattern can only occur in the last case block otherwise it's a syntax error (checked by the compiler). We don't raise this currently, but we should start doing so. I'm going to make an umbrella issue with such syntax errors raised by the compiler.
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.
That makes sense. But that also implies that it would be safe to ignore the check here and simply assume it is the last?
I think we should just use if let Some(last_case) = cases.last() {
// ...
} |
if let Some(last_case) = cases.last() { | ||
if let Pattern::MatchAs(match_as_pattern) = &last_case.pattern { | ||
if match_as_pattern.pattern.is_none() { |
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.
Actually, there are more cases to consider here:
- Make sure that the
guard
field isNone
(last_case.guard.is_none()
) - Consider sequence pattern where if any pattern in the sequence is
_
or named capture, the entire sequence is considered irrefutable
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.
(Sorry for sending this review after the approval)
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.
Good catch! Done.
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! I've made is_irrefutable
a method on Pattern
.
if .. else
for C901
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.