From 99431c864f1dc0ef175e640185b22ea73d3fbd90 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 20 Jan 2024 18:16:50 -0500 Subject: [PATCH] PLR1702 --- .../fixtures/pylint/too_many_nested_blocks.py | 21 +++ .../src/checkers/ast/analyze/statement.rs | 15 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../pylint/rules/too_many_nested_blocks.rs | 129 ++++++++++++++++++ .../ruff_linter/src/rules/pylint/settings.rs | 2 + ...ts__PLR1702_too_many_nested_blocks.py.snap | 20 +++ crates/ruff_workspace/src/options.rs | 6 + ruff.schema.json | 10 ++ 10 files changed, 207 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/too_many_nested_blocks.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/too_many_nested_blocks.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1702_too_many_nested_blocks.py.snap 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 0000000000000..ec9b25b2b51e2 --- /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 c31f147997663..4588c658bc874 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1076,6 +1076,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { .. }, ) => { + 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_); @@ -1210,6 +1213,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); } @@ -1237,6 +1243,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)); } @@ -1260,6 +1269,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, @@ -1324,6 +1336,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 d073f0223f089..c41e7a5ba3d32 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -259,6 +259,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 9182956716f34..2dfdf6d926ad3 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -168,6 +168,7 @@ mod tests { #[test_case(Rule::UnnecessaryDunderCall, Path::new("unnecessary_dunder_call.py"))] #[test_case(Rule::NoStaticmethodDecorator, Path::new("no_method_decorator.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 ba5f2916dca13..92dee3bcd8f74 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -59,6 +59,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::*; @@ -141,6 +142,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 0000000000000..dd03da29f3391 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/too_many_nested_blocks.rs @@ -0,0 +1,129 @@ +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) { + // check that we're in a function + // if not, return + if !checker.semantic().current_scope().kind.is_function() { + return; + } + + // check if this statement has any more branching statements + // if so, return + if stmt_has_more_stmts(stmt) { + return; + } + + let max_nested_blocks = checker.settings.pylint.max_nested_blocks; + + let (count, oldest_ancestor_id) = + checker + .semantic() + .current_statement_ids() + .fold((0, None), |(count, previous_id), id| { + let stmt = checker.semantic().statement(id); + if stmt.is_with_stmt() + || stmt.is_if_stmt() + || stmt.is_try_stmt() + || stmt.is_while_stmt() + || stmt.is_for_stmt() + { + // we want to emit the diagnostic on the + // oldest nested statement + return (count + 1, Some(id)); + } + (count, previous_id) + }); + + let Some(oldest_ancestor_id) = oldest_ancestor_id else { + return; + }; + + if count <= max_nested_blocks { + return; + } + + let oldest_ancestor = checker.semantic().statement(oldest_ancestor_id); + + checker.diagnostics.push(Diagnostic::new( + TooManyNestedBlocks { + nested_blocks: count, + max_nested_blocks, + }, + oldest_ancestor.range(), + )); +} + +fn stmt_has_more_stmts(stmt: &Stmt) -> bool { + match stmt { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + body.iter().any(stmt_has_more_stmts) + || elif_else_clauses + .iter() + .any(|elif_else| elif_else.body.iter().any(stmt_has_more_stmts)) + } + Stmt::While(ast::StmtWhile { body, orelse, .. }) => { + body.iter().any(stmt_has_more_stmts) || orelse.iter().any(stmt_has_more_stmts) + } + Stmt::For(ast::StmtFor { body, orelse, .. }) => { + body.iter().any(stmt_has_more_stmts) || orelse.iter().any(stmt_has_more_stmts) + } + Stmt::Try(ast::StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }) => { + body.iter().any(stmt_has_more_stmts) + || handlers.iter().any(|handler| match handler { + ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + body, .. + }) => body.iter().any(stmt_has_more_stmts), + }) + || orelse.iter().any(stmt_has_more_stmts) + || finalbody.iter().any(stmt_has_more_stmts) + } + Stmt::With(ast::StmtWith { body, .. }) => body.iter().any(stmt_has_more_stmts), + _ => false, + } +} diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index 71ff494bf2a7c..cdd55a9e3bbbf 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 0000000000000..c3df6b5ec638d --- /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 6aaa21113b4b5..778c3eb10d96e 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 / 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 ef61102357472..b35c1a9651f12 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2419,6 +2419,15 @@ "format": "uint", "minimum": 0.0 }, + "max-nested-blocks": { + "description": "Maximum number of nested blocks allowed within a function / 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": [ @@ -3194,6 +3203,7 @@ "PLR17", "PLR170", "PLR1701", + "PLR1702", "PLR1704", "PLR1706", "PLR171",