Consider irrefutable pattern similar to if .. else for C901#11565
Consider irrefutable pattern similar to if .. else for C901#11565dhruvmanila merged 6 commits intoastral-sh:mainfrom
if .. else for C901#11565Conversation
|
| } | ||
| Stmt::Match(ast::StmtMatch { cases, .. }) => { | ||
| for case in cases { | ||
| let last_index = cases.len() - 1; |
There was a problem hiding this comment.
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.
Went with Dhurv's suggestion here.
| for (i, case) in cases.iter().enumerate() { | ||
| 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.
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.
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.
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.
Actually, there are more cases to consider here:
- Make sure that the
guardfield 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.
(Sorry for sending this review after the approval)
dhruvmanila
left a comment
There was a problem hiding this comment.
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
elsestatements.Test Plan
Added new test cases.