From 0b7d6b9097b912c457eaa07ef11214c54fa93c33 Mon Sep 17 00:00:00 2001 From: Chris Chan Date: Sun, 26 Feb 2023 22:42:33 +0000 Subject: [PATCH] Implement pylint's `else-if-used` rule (`PLR5501`) (#3231) Attempt to implement else-if-used https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/else-if-used.html Issue #970 --- .../fixtures/pylint/collapsible_else_if.py | 49 +++++++++++++++++++ crates/ruff/src/checkers/ast.rs | 7 +++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/collapsible_else_if.rs | 42 ++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ...tests__PLR5501_collapsible_else_if.py.snap | 25 ++++++++++ ruff.schema.json | 4 ++ 9 files changed, 132 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py create mode 100644 crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py b/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py new file mode 100644 index 0000000000000..2d12216144244 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py @@ -0,0 +1,49 @@ +""" +Test for else-if-used +""" + + +def ok0(): + """Should not trigger on elif""" + if 1: + pass + elif 2: + pass + + +def ok1(): + """If the orelse has more than 1 item in it, shouldn't trigger""" + if 1: + pass + else: + print() + if 1: + pass + + +def ok2(): + """If the orelse has more than 1 item in it, shouldn't trigger""" + if 1: + pass + else: + if 1: + pass + print() + + +def not_ok0(): + if 1: + pass + else: + if 2: + pass + + +def not_ok1(): + if 1: + pass + else: + if 2: + pass + else: + pass diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index a989c3ba2f204..4141839b5e4bf 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -1633,6 +1633,13 @@ where if self.settings.rules.enabled(&Rule::OutdatedVersionBlock) { pyupgrade::rules::outdated_version_block(self, stmt, test, body, orelse); } + if self.settings.rules.enabled(&Rule::CollapsibleElseIf) { + if let Some(diagnostic) = + pylint::rules::collapsible_else_if(orelse, self.locator) + { + self.diagnostics.push(diagnostic); + } + } } StmtKind::Assert { test, msg } => { if self.settings.rules.enabled(&Rule::AssertTuple) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index c68836bb0876a..3cbfd78cfb385 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -145,6 +145,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "R0133") => Rule::ComparisonOfConstant, (Pylint, "R1701") => Rule::ConsiderMergingIsinstance, (Pylint, "R1722") => Rule::ConsiderUsingSysExit, + (Pylint, "R5501") => Rule::CollapsibleElseIf, (Pylint, "R2004") => Rule::MagicValueComparison, (Pylint, "W0120") => Rule::UselessElseOnLoop, (Pylint, "W0602") => Rule::GlobalVariableNotAssigned, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 7b91f1098c16c..ea1202f4d2769 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -135,6 +135,7 @@ ruff_macros::register_rules!( rules::pylint::rules::BadStringFormatType, rules::pylint::rules::BidirectionalUnicode, rules::pylint::rules::BadStrStripCall, + rules::pylint::rules::CollapsibleElseIf, rules::pylint::rules::UselessImportAlias, rules::pylint::rules::UnnecessaryDirectLambdaCall, rules::pylint::rules::NonlocalWithoutBinding, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index 6a9b341552d9e..f7cf12679dac4 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -45,6 +45,7 @@ mod tests { #[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"); "PLR0913")] #[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"); "PLR0912")] #[test_case(Rule::TooManyStatements, Path::new("too_many_statements.py"); "PLR0915")] + #[test_case(Rule::CollapsibleElseIf, Path::new("collapsible_else_if.py"); "PLR5501")] #[test_case(Rule::BadStringFormatType, Path::new("bad_string_format_type.py"); "PLE1307")] #[test_case(Rule::BidirectionalUnicode, Path::new("bidirectional_unicode.py"); "PLE2502")] #[test_case(Rule::BadStrStripCall, Path::new("bad_str_strip_call.py"); "PLE01310")] diff --git a/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs b/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs new file mode 100644 index 0000000000000..4328bddaee183 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs @@ -0,0 +1,42 @@ +use rustpython_parser::ast::{Stmt, StmtKind}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::types::Range; +use crate::registry::Diagnostic; +use crate::source_code::Locator; +use crate::violation::Violation; + +define_violation!( + pub struct CollapsibleElseIf; +); + +impl Violation for CollapsibleElseIf { + #[derive_message_formats] + fn message(&self) -> String { + format!("Consider using `elif` instead of `else` then `if` to remove one indentation level") + } +} + +/// PLR5501 +pub fn collapsible_else_if(orelse: &[Stmt], locator: &Locator) -> Option { + if orelse.len() == 1 { + let first = &orelse[0]; + if matches!(first.node, StmtKind::If { .. }) { + // Determine whether this is an `elif`, or an `if` in an `else` block. + if locator + .slice(&Range { + location: first.location, + end_location: first.end_location.unwrap(), + }) + .starts_with("if") + { + return Some(Diagnostic::new( + CollapsibleElseIf, + Range::from_located(first), + )); + } + } + } + None +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 3c150358faef8..f8996485da95b 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -2,6 +2,7 @@ pub use await_outside_async::{await_outside_async, AwaitOutsideAsync}; pub use bad_str_strip_call::{bad_str_strip_call, BadStrStripCall}; pub use bad_string_format_type::{bad_string_format_type, BadStringFormatType}; pub use bidirectional_unicode::{bidirectional_unicode, BidirectionalUnicode}; +pub use collapsible_else_if::{collapsible_else_if, CollapsibleElseIf}; pub use comparison_of_constant::{comparison_of_constant, ComparisonOfConstant}; pub use consider_using_sys_exit::{consider_using_sys_exit, ConsiderUsingSysExit}; pub use global_variable_not_assigned::GlobalVariableNotAssigned; @@ -33,6 +34,7 @@ mod await_outside_async; mod bad_str_strip_call; mod bad_string_format_type; mod bidirectional_unicode; +mod collapsible_else_if; mod comparison_of_constant; mod consider_using_sys_exit; mod global_variable_not_assigned; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap new file mode 100644 index 0000000000000..7011e0396344c --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap @@ -0,0 +1,25 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + CollapsibleElseIf: ~ + location: + row: 38 + column: 8 + end_location: + row: 39 + column: 16 + fix: ~ + parent: ~ +- kind: + CollapsibleElseIf: ~ + location: + row: 46 + column: 8 + end_location: + row: 49 + column: 16 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index 99096119f17b2..22319d61471d8 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1847,6 +1847,10 @@ "PLR20", "PLR200", "PLR2004", + "PLR5", + "PLR55", + "PLR550", + "PLR5501", "PLW", "PLW0", "PLW01",