-
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
Add rule PLR5601 #10103
Closed
Closed
Add rule PLR5601 #10103
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
140 changes: 140 additions & 0 deletions
140
crates/ruff_linter/resources/test/fixtures/pylint/confusing_consecutive_elif.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
def triggered_if_if_block_ends_with_elif(machine, old_conf, new_conf): | ||
"""Example code that will trigger the message | ||
|
||
Given an if-elif construct | ||
When the body of the if ends with an elif | ||
Then the message confusing-consecutive-elif must be triggered. | ||
""" | ||
if old_conf: | ||
if not new_conf: | ||
machine.disable() | ||
elif old_conf.value != new_conf.value: | ||
machine.disable() | ||
machine.enable(new_conf.value) | ||
elif new_conf: # [confusing-consecutive-elif] | ||
machine.enable(new_conf.value) | ||
|
||
|
||
def not_triggered_if_indented_block_ends_with_else(machine, old_conf, new_conf): | ||
"""Example code must not trigger the message, because the inner block ends with else. | ||
|
||
Given an if-elif construct | ||
When the body of the if ends with an else | ||
Then no message shall be triggered. | ||
""" | ||
if old_conf: | ||
if not new_conf: | ||
machine.disable() | ||
elif old_conf.value != new_conf.value: | ||
machine.disable() | ||
machine.enable(new_conf.value) | ||
else: | ||
pass | ||
elif new_conf: | ||
machine.enable(new_conf.value) | ||
|
||
|
||
def not_triggered_if_indentend_block_ends_with_call(machine, old_conf, new_conf): | ||
""" | ||
Example code must not trigger the message, | ||
|
||
Given an if-elif construct | ||
When the body of the if ends with a function call | ||
Then no message shall be triggered. | ||
|
||
Note: There is nothing special about the body ending with a function call. | ||
This is just taken as a representative value for the equivalence class of | ||
"every node class unrelated to if/elif/else". | ||
""" | ||
if old_conf: | ||
if not new_conf: | ||
machine.disable() | ||
elif old_conf.value != new_conf.value: | ||
machine.disable() | ||
machine.enable(new_conf.value) | ||
print("Processed old configuration...") | ||
elif new_conf: | ||
machine.enable(new_conf.value) | ||
|
||
|
||
def triggered_if_elif_block_ends_with_elif(machine, old_conf, new_conf, new_new_conf): | ||
"""Example code that will trigger the message | ||
|
||
Given an if-elif-elif construct | ||
When the body of the first elif ends with an elif | ||
Then the message confusing-consecutive-elif must be triggered. | ||
""" | ||
if old_conf: | ||
machine.disable() | ||
elif not new_conf: | ||
if new_new_conf: | ||
machine.disable() | ||
elif old_conf.value != new_conf.value: | ||
machine.disable() | ||
machine.enable(new_conf.value) | ||
elif new_conf: # [confusing-consecutive-elif] | ||
machine.enable(new_conf.value) | ||
|
||
|
||
def triggered_if_block_ends_with_if(machine, old_conf, new_conf, new_new_conf): | ||
"""Example code that will trigger the message | ||
|
||
Given an if-elif construct | ||
When the body of the if ends with an if | ||
Then the message confusing-consecutive-elif must be triggered. | ||
""" | ||
if old_conf: | ||
if new_new_conf: | ||
machine.disable() | ||
elif new_conf: # [confusing-consecutive-elif] | ||
machine.enable(new_conf.value) | ||
|
||
|
||
def not_triggered_if_indented_block_ends_with_ifexp(machine, old_conf, new_conf): | ||
""" | ||
Example code must not trigger the message, | ||
|
||
Given an if-elif construct | ||
When the body of the if ends with an if expression | ||
Then no message shall be triggered. | ||
""" | ||
if old_conf: | ||
if not new_conf: | ||
machine.disable() | ||
print("Processed old configuration...") | ||
elif new_conf: | ||
machine.enable(new_conf.value) | ||
|
||
|
||
def not_triggered_if_outer_block_does_not_have_elif(machine, old_conf, new_conf): | ||
"""Example code must not trigger the message | ||
|
||
Given an if construct without an elif | ||
When the body of the if ends with an if | ||
Then no message shall be triggered. | ||
""" | ||
if old_conf: | ||
if not new_conf: | ||
machine.disable() | ||
elif old_conf.value != new_conf.value: | ||
machine.disable() | ||
machine.enable(new_conf.value) | ||
else: | ||
pass | ||
|
||
|
||
def not_triggered_if_outer_block_continues_with_if(machine, old_conf, new_conf, new_new_conf): | ||
"""Example code that will trigger the message | ||
|
||
Given an if construct which continues with a new if construct | ||
When the body of the first if ends with an if expression | ||
Then no message shall be triggered. | ||
""" | ||
if old_conf: | ||
if new_new_conf: | ||
machine.disable() | ||
elif old_conf.value != new_conf.value: | ||
machine.disable() | ||
machine.enable(new_conf.value) | ||
if new_conf: | ||
machine.enable(new_conf.value) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
147 changes: 147 additions & 0 deletions
147
crates/ruff_linter/src/rules/pylint/rules/confusing_consecutive_elif.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
use ruff_diagnostics::{Diagnostic, Violation}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::{self as ast, ElifElseClause, Stmt, StmtIf}; | ||
use ruff_text_size::{Ranged, TextRange}; | ||
|
||
use crate::checkers::ast::Checker; | ||
|
||
/// ## What it does | ||
/// Checks for an `elif` statement which follows right after an indented block which itself ends with if or elif." | ||
/// | ||
/// ## Why is this bad? | ||
/// It may not be ovious if the elif statement was willingly or mistakenly unindented. | ||
/// Adding an explicit else, or extracting the indented if statement into a separate | ||
/// function might avoid confusion and prevent errors. | ||
/// | ||
/// ## Example | ||
/// ```python | ||
/// if old_conf: | ||
/// if not new_conf: | ||
/// machine.disable() | ||
/// elif old_conf.value != new_conf.value: | ||
/// machine.disable() | ||
/// machine.enable(new_conf.value) | ||
/// elif new_conf: # [confusing-consecutive-elif] | ||
/// machine.enable(new_conf.value) | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// # Option 1: add explicit else | ||
/// if old_conf: | ||
/// if not new_conf: | ||
/// machine.disable() | ||
/// elif old_conf.value != new_conf.value: | ||
/// machine.disable() | ||
/// machine.enable(new_conf.value) | ||
/// else: | ||
/// pass | ||
/// elif new_conf: # [confusing-consecutive-elif] | ||
/// machine.enable(new_conf.value) | ||
/// | ||
/// | ||
/// # Option 2: extract function | ||
/// def extracted(old_conf, new_conf, machine): | ||
/// if not new_conf: | ||
/// machine.disable() | ||
/// elif old_conf.value != new_conf.value: | ||
/// machine.disable() | ||
/// machine.enable(new_conf.value) | ||
/// | ||
/// | ||
/// if old_conf: | ||
/// extracted(old_conf, new_conf, machine) | ||
/// elif new_conf: | ||
/// machine.enable(new_conf.value) | ||
/// ``` | ||
/// | ||
/// ## References | ||
/// - [Python documentation: `if` Statements](https://docs.python.org/3/tutorial/controlflow.html#if-statements) | ||
#[violation] | ||
pub struct ConfusingConsecutiveElif; | ||
|
||
impl Violation for ConfusingConsecutiveElif { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
format!("Consecutive elif with differing indentation level, consider creating a function to separate the inner elif") | ||
} | ||
} | ||
|
||
/// PLR5601 | ||
pub(crate) fn confusing_consecutive_elif(checker: &mut Checker, stmt_if: &StmtIf) { | ||
let ast::StmtIf { | ||
body, | ||
elif_else_clauses, | ||
.. | ||
} = stmt_if; | ||
|
||
// The last clause must be an elif | ||
let Some(ElifElseClause { test: Some(_), .. }) = elif_else_clauses.last() else { | ||
return; | ||
}; | ||
|
||
// Take the second last elif, or if that does not exist, take the if | ||
let orelse = match elif_else_clauses.len() { | ||
0 => return, | ||
1 => { | ||
let Some(Stmt::If(ast::StmtIf { | ||
elif_else_clauses: orelse, | ||
.. | ||
})) = body.last() | ||
else { | ||
return; | ||
}; | ||
orelse | ||
} | ||
_ => { | ||
let [.., ElifElseClause { | ||
body: body_stmt, | ||
test: Some(_), | ||
.. | ||
}, _] = elif_else_clauses.as_slice() | ||
else { | ||
return; | ||
}; | ||
let Some(Stmt::If(ast::StmtIf { | ||
elif_else_clauses: orelse, | ||
.. | ||
})) = body_stmt.last() | ||
else { | ||
return; | ||
}; | ||
orelse | ||
} | ||
}; | ||
if !has_no_else_clause(orelse) { | ||
return; | ||
} | ||
|
||
let diagnostic = Diagnostic::new( | ||
ConfusingConsecutiveElif, | ||
TextRange::new(elif_else_clauses.last().unwrap().start(), stmt_if.end()), | ||
); | ||
|
||
checker.diagnostics.push(diagnostic); | ||
} | ||
|
||
fn has_no_else_clause(orelse: &[ElifElseClause]) -> bool { | ||
if orelse.is_empty() { | ||
return true; | ||
} | ||
let Some(ElifElseClause { | ||
body: body_stmt, | ||
test: Some(_), | ||
.. | ||
}) = orelse.last() | ||
else { | ||
return false; | ||
}; | ||
let Some(Stmt::If(ast::StmtIf { | ||
elif_else_clauses: orelse, | ||
.. | ||
})) = body_stmt.last() | ||
else { | ||
return true; | ||
}; | ||
has_no_else_clause(orelse) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
34 changes: 34 additions & 0 deletions
34
...t/snapshots/ruff_linter__rules__pylint__tests__PLR5601_confusing_consecutive_elif.py.snap
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
--- | ||
source: crates/ruff_linter/src/rules/pylint/mod.rs | ||
--- | ||
confusing_consecutive_elif.py:14:5: PLR5601 Consecutive elif with differing indentation level, consider creating a function to separate the inner elif | ||
| | ||
12 | machine.disable() | ||
13 | machine.enable(new_conf.value) | ||
14 | elif new_conf: # [confusing-consecutive-elif] | ||
| _____^ | ||
15 | | machine.enable(new_conf.value) | ||
| |______________________________________^ PLR5601 | ||
| | ||
|
||
confusing_consecutive_elif.py:75:5: PLR5601 Consecutive elif with differing indentation level, consider creating a function to separate the inner elif | ||
| | ||
73 | machine.disable() | ||
74 | machine.enable(new_conf.value) | ||
75 | elif new_conf: # [confusing-consecutive-elif] | ||
| _____^ | ||
76 | | machine.enable(new_conf.value) | ||
| |______________________________________^ PLR5601 | ||
| | ||
|
||
confusing_consecutive_elif.py:89:5: PLR5601 Consecutive elif with differing indentation level, consider creating a function to separate the inner elif | ||
| | ||
87 | if new_new_conf: | ||
88 | machine.disable() | ||
89 | elif new_conf: # [confusing-consecutive-elif] | ||
| _____^ | ||
90 | | machine.enable(new_conf.value) | ||
| |______________________________________^ PLR5601 | ||
| | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 original pylint docs mention an explicit else as alternative resolution. We should also provide that option I believe.