Skip to content
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

[pylint] Implement too-many-nested-blocks (PLR1702) #9172

Merged
merged 3 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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_);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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));
}
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
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 @@ -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")
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
132 changes: 132 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/too_many_nested_blocks.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -75,6 +76,7 @@ impl Default for Settings {
max_statements: 50,
max_public_methods: 20,
max_locals: 15,
max_nested_blocks: 5,
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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
|


6 changes: 6 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<usize>,

/// 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<usize>,
}

impl PylintOptions {
Expand All @@ -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),
}
}
}
Expand Down
10 changes: 10 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.