diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_many_nested_blocks.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_nested_blocks.py new file mode 100644 index 00000000000000..ec9b25b2b51e2f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_nested_blocks.py @@ -0,0 +1,21 @@ +def correct_fruits(fruits) -> bool: + if len(fruits) > 1: # PLR1702 + if "apple" in fruits: + if "orange" in fruits: + count = fruits["orange"] + if count % 2: + if "kiwi" in fruits: + if count == 2: + return True + return False + +# Ok +def correct_fruits(fruits) -> bool: + if len(fruits) > 1: + if "apple" in fruits: + if "orange" in fruits: + count = fruits["orange"] + if count % 2: + if "kiwi" in fruits: + return True + return False diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index a613625abd3564..481e52a0083405 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1073,6 +1073,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } Stmt::If(if_ @ ast::StmtIf { test, .. }) => { + if checker.enabled(Rule::TooManyNestedBlocks) { + pylint::rules::too_many_nested_blocks(checker, stmt); + } if checker.enabled(Rule::EmptyTypeCheckingBlock) { if typing::is_type_checking_block(if_, &checker.semantic) { flake8_type_checking::rules::empty_type_checking_block(checker, if_); @@ -1205,6 +1208,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } Stmt::With(with_stmt @ ast::StmtWith { items, body, .. }) => { + if checker.enabled(Rule::TooManyNestedBlocks) { + pylint::rules::too_many_nested_blocks(checker, stmt); + } if checker.enabled(Rule::AssertRaisesException) { flake8_bugbear::rules::assert_raises_exception(checker, items); } @@ -1232,6 +1238,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } Stmt::While(while_stmt @ ast::StmtWhile { body, orelse, .. }) => { + if checker.enabled(Rule::TooManyNestedBlocks) { + pylint::rules::too_many_nested_blocks(checker, stmt); + } if checker.enabled(Rule::FunctionUsesLoopVariable) { flake8_bugbear::rules::function_uses_loop_variable(checker, &Node::Stmt(stmt)); } @@ -1255,6 +1264,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { range: _, }, ) => { + if checker.enabled(Rule::TooManyNestedBlocks) { + pylint::rules::too_many_nested_blocks(checker, stmt); + } if checker.any_enabled(&[ Rule::EnumerateForLoop, Rule::IncorrectDictIterator, @@ -1319,6 +1331,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { finalbody, .. }) => { + if checker.enabled(Rule::TooManyNestedBlocks) { + pylint::rules::too_many_nested_blocks(checker, stmt); + } if checker.enabled(Rule::JumpStatementInFinally) { flake8_bugbear::rules::jump_statement_in_finally(checker, finalbody); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f7c296a3ecc9c0..3e1a2df5591dad 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -261,6 +261,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions), (Pylint, "R0917") => (RuleGroup::Preview, rules::pylint::rules::TooManyPositional), (Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls), + (Pylint, "R1702") => (RuleGroup::Preview, rules::pylint::rules::TooManyNestedBlocks), (Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal), (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index acae7be19e9adb..5948e9d15789c1 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -171,6 +171,7 @@ mod tests { #[test_case(Rule::NoStaticmethodDecorator, Path::new("no_method_decorator.py"))] #[test_case(Rule::PotentialIndexError, Path::new("potential_index_error.py"))] #[test_case(Rule::SuperWithoutBrackets, Path::new("super_without_brackets.py"))] + #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 2845943106d667..3ba5d1061b7d24 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -61,6 +61,7 @@ pub(crate) use too_many_arguments::*; pub(crate) use too_many_boolean_expressions::*; pub(crate) use too_many_branches::*; pub(crate) use too_many_locals::*; +pub(crate) use too_many_nested_blocks::*; pub(crate) use too_many_positional::*; pub(crate) use too_many_public_methods::*; pub(crate) use too_many_return_statements::*; @@ -145,6 +146,7 @@ mod too_many_arguments; mod too_many_boolean_expressions; mod too_many_branches; mod too_many_locals; +mod too_many_nested_blocks; mod too_many_positional; mod too_many_public_methods; mod too_many_return_statements; diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_many_nested_blocks.rs b/crates/ruff_linter/src/rules/pylint/rules/too_many_nested_blocks.rs new file mode 100644 index 00000000000000..bd641f0aa9205f --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/too_many_nested_blocks.rs @@ -0,0 +1,132 @@ +use ast::ExceptHandler; +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Stmt}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for functions or methods with too many nested blocks. +/// +/// By default, this rule allows up to five nested blocks. +/// This can be configured using the [`pylint.max-nested-blocks`] option. +/// +/// ## Why is this bad? +/// Functions or methods with too many nested blocks are harder to understand +/// and maintain. +/// +/// ## Options +/// - `pylint.max-nested-blocks` +#[violation] +pub struct TooManyNestedBlocks { + nested_blocks: usize, + max_nested_blocks: usize, +} + +impl Violation for TooManyNestedBlocks { + #[derive_message_formats] + fn message(&self) -> String { + let TooManyNestedBlocks { + nested_blocks, + max_nested_blocks, + } = self; + format!("Too many nested blocks ({nested_blocks} > {max_nested_blocks})") + } +} + +/// PLR1702 +pub(crate) fn too_many_nested_blocks(checker: &mut Checker, stmt: &Stmt) { + // Only enforce nesting within functions or methods. + if !checker.semantic().current_scope().kind.is_function() { + return; + } + + // If the statement isn't a leaf node, we don't want to emit a diagnostic, since the diagnostic + // will be emitted on the leaves. + if has_nested_block(stmt) { + return; + } + + let max_nested_blocks = checker.settings.pylint.max_nested_blocks; + + // Traverse up the hierarchy, identifying the root node and counting the number of nested + // blocks between the root and this leaf. + let (count, root_id) = + checker + .semantic() + .current_statement_ids() + .fold((0, None), |(count, ancestor_id), id| { + let stmt = checker.semantic().statement(id); + if is_nested_block(stmt) { + (count + 1, Some(id)) + } else { + (count, ancestor_id) + } + }); + + let Some(root_id) = root_id else { + return; + }; + + // If the number of nested blocks is less than the maximum, we don't want to emit a diagnostic. + if count <= max_nested_blocks { + return; + } + + checker.diagnostics.push(Diagnostic::new( + TooManyNestedBlocks { + nested_blocks: count, + max_nested_blocks, + }, + checker.semantic().statement(root_id).range(), + )); +} + +/// Returns `true` if the given statement is a nested block. +fn is_nested_block(stmt: &Stmt) -> bool { + matches!( + stmt, + Stmt::If(_) | Stmt::While(_) | Stmt::For(_) | Stmt::Try(_) | Stmt::With(_) + ) +} + +/// Returns `true` if the given statement is a leaf node. +fn has_nested_block(stmt: &Stmt) -> bool { + match stmt { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + body.iter().any(is_nested_block) + || elif_else_clauses + .iter() + .any(|elif_else| elif_else.body.iter().any(is_nested_block)) + } + Stmt::While(ast::StmtWhile { body, orelse, .. }) => { + body.iter().any(is_nested_block) || orelse.iter().any(is_nested_block) + } + Stmt::For(ast::StmtFor { body, orelse, .. }) => { + body.iter().any(is_nested_block) || orelse.iter().any(is_nested_block) + } + Stmt::Try(ast::StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }) => { + body.iter().any(is_nested_block) + || handlers.iter().any(|handler| match handler { + ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + body, .. + }) => body.iter().any(is_nested_block), + }) + || orelse.iter().any(is_nested_block) + || finalbody.iter().any(is_nested_block) + } + Stmt::With(ast::StmtWith { body, .. }) => body.iter().any(is_nested_block), + _ => false, + } +} diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index 71ff494bf2a7cb..cdd55a9e3bbbf8 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -60,6 +60,7 @@ pub struct Settings { pub max_statements: usize, pub max_public_methods: usize, pub max_locals: usize, + pub max_nested_blocks: usize, } impl Default for Settings { @@ -75,6 +76,7 @@ impl Default for Settings { max_statements: 50, max_public_methods: 20, max_locals: 15, + max_nested_blocks: 5, } } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1702_too_many_nested_blocks.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1702_too_many_nested_blocks.py.snap new file mode 100644 index 00000000000000..c3df6b5ec638d7 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1702_too_many_nested_blocks.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +too_many_nested_blocks.py:2:5: PLR1702 Too many nested blocks (6 > 5) + | + 1 | def correct_fruits(fruits) -> bool: + 2 | if len(fruits) > 1: # PLR1702 + | _____^ + 3 | | if "apple" in fruits: + 4 | | if "orange" in fruits: + 5 | | count = fruits["orange"] + 6 | | if count % 2: + 7 | | if "kiwi" in fruits: + 8 | | if count == 2: + 9 | | return True + | |_______________________________________^ PLR1702 +10 | return False + | + + diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 6aaa21113b4b55..ff70baa6f45057 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2728,6 +2728,11 @@ pub struct PylintOptions { /// (see: `PLR0916`). #[option(default = r"5", value_type = "int", example = r"max-bool-expr = 5")] pub max_bool_expr: Option, + + /// Maximum number of nested blocks allowed within a function or method body + /// (see: `PLR1702`). + #[option(default = r"5", value_type = "int", example = r"max-nested-blocks = 5")] + pub max_nested_blocks: Option, } impl PylintOptions { @@ -2751,6 +2756,7 @@ impl PylintOptions { .max_public_methods .unwrap_or(defaults.max_public_methods), max_locals: self.max_locals.unwrap_or(defaults.max_locals), + max_nested_blocks: self.max_nested_blocks.unwrap_or(defaults.max_nested_blocks), } } } diff --git a/ruff.schema.json b/ruff.schema.json index 54e7656c6d7c55..818222b2d2df79 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2433,6 +2433,15 @@ "format": "uint", "minimum": 0.0 }, + "max-nested-blocks": { + "description": "Maximum number of nested blocks allowed within a function or method body (see: `PLR1702`).", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, "max-positional-args": { "description": "Maximum number of positional arguments allowed for a function or method definition (see: `PLR0917`).\n\nIf not specified, defaults to the value of `max-args`.", "type": [ @@ -3212,6 +3221,7 @@ "PLR17", "PLR170", "PLR1701", + "PLR1702", "PLR1704", "PLR1706", "PLR171",