Skip to content

Commit

Permalink
[pylint] Add fix for collapsible-else-if (PLR5501) (#9594)
Browse files Browse the repository at this point in the history
## Summary

adds a fix for `collapsible-else-if` / `PLR5501`

## Test Plan

`cargo test`
  • Loading branch information
diceroll123 authored Jan 22, 2024
1 parent 1e4b421 commit e54ed28
Show file tree
Hide file tree
Showing 6 changed files with 377 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,17 @@ def not_ok1():
pass


def not_ok1_with_comments():
if 1:
pass
else:
# inner comment
if 2:
pass
else:
pass # final pass comment


# Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737
def not_ok2():
if True:
Expand All @@ -61,3 +72,28 @@ def not_ok2():
else:
print(4)


def not_ok3():
if 1:
pass
else:
if 2: pass
else: pass


def not_ok4():
if 1:
pass
else:
if 2: pass
else:
pass


def not_ok5():
if 1:
pass
else:
if 2:
pass
else: pass
12 changes: 2 additions & 10 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1069,13 +1069,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
ruff::rules::sort_dunder_all_aug_assign(checker, aug_assign);
}
}
Stmt::If(
if_ @ ast::StmtIf {
test,
elif_else_clauses,
..
},
) => {
Stmt::If(if_ @ ast::StmtIf { test, .. }) => {
if checker.enabled(Rule::EmptyTypeCheckingBlock) {
if typing::is_type_checking_block(if_, &checker.semantic) {
flake8_type_checking::rules::empty_type_checking_block(checker, if_);
Expand Down Expand Up @@ -1117,9 +1111,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
pyupgrade::rules::outdated_version_block(checker, if_);
}
if checker.enabled(Rule::CollapsibleElseIf) {
if let Some(diagnostic) = pylint::rules::collapsible_else_if(elif_else_clauses) {
checker.diagnostics.push(diagnostic);
}
pylint::rules::collapsible_else_if(checker, stmt);
}
if checker.enabled(Rule::CheckAndRemoveFromSet) {
refurb::rules::check_and_remove_from_set(checker, if_);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ mod tests {
}

#[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"))]
#[test_case(Rule::CollapsibleElseIf, Path::new("collapsible_else_if.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
Expand Down
97 changes: 81 additions & 16 deletions crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
use ruff_python_ast::{ElifElseClause, Stmt};
use ruff_text_size::{Ranged, TextRange};
use anyhow::Result;

use ruff_diagnostics::{Diagnostic, Violation};
use ast::whitespace::indentation;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, ElifElseClause, Stmt};
use ruff_python_codegen::Stylist;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::rules::pyupgrade::fixes::adjust_indentation;

/// ## What it does
/// Checks for `else` blocks that consist of a single `if` statement.
Expand Down Expand Up @@ -40,27 +47,85 @@ use ruff_macros::{derive_message_formats, violation};
pub struct CollapsibleElseIf;

impl Violation for CollapsibleElseIf {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Use `elif` instead of `else` then `if`, to reduce indentation")
}

fn fix_title(&self) -> Option<String> {
Some("Convert to `elif`".to_string())
}
}

/// PLR5501
pub(crate) fn collapsible_else_if(elif_else_clauses: &[ElifElseClause]) -> Option<Diagnostic> {
let Some(ElifElseClause {
body,
test: None,
range,
}) = elif_else_clauses.last()
pub(crate) fn collapsible_else_if(checker: &mut Checker, stmt: &Stmt) {
let Stmt::If(ast::StmtIf {
elif_else_clauses, ..
}) = stmt
else {
return None;
return;
};
if let [first @ Stmt::If(_)] = body.as_slice() {
return Some(Diagnostic::new(
CollapsibleElseIf,
TextRange::new(range.start(), first.start()),
));

let Some(
else_clause @ ElifElseClause {
body, test: None, ..
},
) = elif_else_clauses.last()
else {
return;
};
let [first @ Stmt::If(ast::StmtIf { .. })] = body.as_slice() else {
return;
};

let mut diagnostic = Diagnostic::new(
CollapsibleElseIf,
TextRange::new(else_clause.start(), first.start()),
);

if checker.settings.preview.is_enabled() {
diagnostic.try_set_fix(|| {
convert_to_elif(first, else_clause, checker.locator(), checker.stylist())
});
}
None

checker.diagnostics.push(diagnostic);
}

/// Generate [`Fix`] to convert an `else` block to an `elif` block.
fn convert_to_elif(
first: &Stmt,
else_clause: &ElifElseClause,
locator: &Locator,
stylist: &Stylist,
) -> Result<Fix> {
let inner_if_line_start = locator.line_start(first.start());
let inner_if_line_end = locator.line_end(first.end());

// Identify the indentation of the loop itself (e.g., the `while` or `for`).
let Some(indentation) = indentation(locator, else_clause) else {
return Err(anyhow::anyhow!("`else` is expected to be on its own line"));
};

// Dedent the content from the end of the `else` to the end of the `if`.
let indented = adjust_indentation(
TextRange::new(inner_if_line_start, inner_if_line_end),
indentation,
locator,
stylist,
)?;

// Strip the indent from the first line of the `if` statement, and add `el` to the start.
let Some(unindented) = indented.strip_prefix(indentation) else {
return Err(anyhow::anyhow!("indented block to start with indentation"));
};
let indented = format!("{indentation}el{unindented}");

Ok(Fix::safe_edit(Edit::replacement(
indented,
locator.line_start(else_clause.start()),
inner_if_line_end,
)))
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ collapsible_else_if.py:37:5: PLR5501 Use `elif` instead of `else` then `if`, to
| |________^ PLR5501
39 | pass
|
= help: Convert to `elif`

collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
|
Expand All @@ -23,17 +24,71 @@ collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to
47 | pass
48 | else:
|
= help: Convert to `elif`

collapsible_else_if.py:58:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
collapsible_else_if.py:55:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
|
56 | elif True:
57 | print(2)
58 | else:
53 | if 1:
54 | pass
55 | else:
| _____^
59 | | if True:
56 | | # inner comment
57 | | if 2:
| |________^ PLR5501
60 | print(3)
61 | else:
58 | pass
59 | else:
|
= help: Convert to `elif`

collapsible_else_if.py:69:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
|
67 | elif True:
68 | print(2)
69 | else:
| _____^
70 | | if True:
| |________^ PLR5501
71 | print(3)
72 | else:
|
= help: Convert to `elif`

collapsible_else_if.py:79:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
|
77 | if 1:
78 | pass
79 | else:
| _____^
80 | | if 2: pass
| |________^ PLR5501
81 | else: pass
|
= help: Convert to `elif`

collapsible_else_if.py:87:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
|
85 | if 1:
86 | pass
87 | else:
| _____^
88 | | if 2: pass
| |________^ PLR5501
89 | else:
90 | pass
|
= help: Convert to `elif`

collapsible_else_if.py:96:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation
|
94 | if 1:
95 | pass
96 | else:
| _____^
97 | | if 2:
| |________^ PLR5501
98 | pass
99 | else: pass
|
= help: Convert to `elif`


Loading

0 comments on commit e54ed28

Please sign in to comment.