From 9b42520a53aafb7252b59b4411de3d4b3cda9a6e Mon Sep 17 00:00:00 2001 From: Chammika Mannakkara Date: Tue, 3 Oct 2023 21:21:48 +0900 Subject: [PATCH 1/8] SIM113 added with tests --- .../test/fixtures/flake8_simplify/SIM113.py | 104 ++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../src/rules/flake8_simplify/mod.rs | 1 + .../flake8_simplify/rules/ast_bool_op.rs | 2 +- .../flake8_simplify/rules/enumerate_for.rs | 179 ++++++++++++++++++ .../src/rules/flake8_simplify/rules/mod.rs | 2 + ...ke8_simplify__tests__SIM113_SIM113.py.snap | 46 +++++ crates/ruff_python_ast/src/traversal.rs | 12 ++ ruff.schema.json | 1 + 10 files changed, 350 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py create mode 100644 crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs create mode 100644 crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py new file mode 100644 index 0000000000000..2fe1a9d21b2da --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py @@ -0,0 +1,104 @@ + +def f(): + # SIM113 + idx = 0 + for x in iterable: + g(x, idx) + idx +=1 + h(x) + + +def f(): + # SIM113 + sum = 0 + idx = 0 + for x in iterable: + if g(x): + break + idx += 1 + sum += h(x, idx) + + +def f(): + # SIM113 + idx = 0 + for x, y in iterable_tuples(): + g(x) + h(x, y) + idx += 1 + +def f(): + # Current SIM113 doesn't catch this yet because for loop + # doesn't immidiately follow index initialization + idx = 0 + sum = 0 + for x in iterable: + sum += h(x, idx) + idx += 1 + + +def f(): + # Current SIM113 doesn't catch this due to unpacking in + # in intialization + sum, idx = 0, 0 + for x in iterable: + g(x, idx) + idx +=1 + h(x) + + +def f(): + # loop doesn't start at zero + idx = 10 + for x in iterable: + g(x, idx) + idx +=1 + h(x) + +def f(): + # index doesn't increment by one + idx = 0 + for x in iterable: + g(x, idx) + idx +=2 + h(x) + +def f(): + # index increment inside condition + idx = 0 + for x in iterable: + if g(x): + idx += 1 + h(x) + +def f(): + # Continue in match-case + idx = 0 + for x in iterable: + match g(x): + case 1: h(x) + case 2: continue + case _: h(idx) + idx += 1 + +def f(): + # Continue inside with clause + idx = 0 + for x in iterable: + with context as c: + if g(x): + continue + h(x, idx, c) + idx += 1 + + +def f(): + # Continue in try block + idx = 0 + for x in iterable: + try: + g(x, idx) + except: + h(x) + continue + idx += 1 \ No newline at end of file diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index a39fd0734355c..fd7fe7be7c889 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1236,6 +1236,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::TryExceptInLoop) { perflint::rules::try_except_in_loop(checker, body); } + if checker.enabled(Rule::EnumerateForLoop) { + flake8_simplify::rules::use_enumerate_in_for_loop(checker, stmt); + } } } Stmt::Try(ast::StmtTry { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f985b9a230433..256b7d7f767ec 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -423,6 +423,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Simplify, "109") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::CompareWithTuple), (Flake8Simplify, "110") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::ReimplementedBuiltin), (Flake8Simplify, "112") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::UncapitalizedEnvironmentVariables), + (Flake8Simplify, "113") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::EnumerateForLoop), (Flake8Simplify, "114") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::IfWithSameArms), (Flake8Simplify, "115") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::OpenFileWithContextHandler), (Flake8Simplify, "116") => (RuleGroup::Unspecified, rules::flake8_simplify::rules::IfElseBlockInsteadOfDictLookup), diff --git a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs index 82a7d33c9b0c8..5652d8e40b2c3 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs @@ -26,6 +26,7 @@ mod tests { #[test_case(Rule::ReimplementedBuiltin, Path::new("SIM110.py"))] #[test_case(Rule::ReimplementedBuiltin, Path::new("SIM111.py"))] #[test_case(Rule::UncapitalizedEnvironmentVariables, Path::new("SIM112.py"))] + #[test_case(Rule::EnumerateForLoop, Path::new("SIM113.py"))] #[test_case(Rule::OpenFileWithContextHandler, Path::new("SIM115.py"))] #[test_case(Rule::MultipleWithStatements, Path::new("SIM117.py"))] #[test_case(Rule::InDictKeys, Path::new("SIM118.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs index eb41bcad7f2bb..b771c5c3af573 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs @@ -288,7 +288,7 @@ impl AlwaysFixableViolation for ExprAndFalse { } /// Return `true` if two `Expr` instances are equivalent names. -fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> { +pub(crate) fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> { if let (Expr::Name(ast::ExprName { id: a, .. }), Expr::Name(ast::ExprName { id: b, .. })) = (&a, &b) { diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs new file mode 100644 index 0000000000000..ff19647ab446a --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs @@ -0,0 +1,179 @@ +use num_traits::identities::{One, Zero}; +use ruff_python_ast::{self as ast, Constant, ExceptHandler, Expr, MatchCase, Operator, Stmt}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::rules::flake8_simplify::rules::ast_bool_op::is_same_expr; +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::traversal; + +use crate::checkers::ast::Checker; + +/// ### What it does +/// Checks for loops which has explicit loop-index variables that can be simplified by using `enumerate()`. +/// +/// ## Why this is bad ? +/// Using `enumerate()` is more readable and concise. It could lead to more efficient +/// and less error-prone code. +/// +/// ### Example +/// ```python +/// sum = 0 +/// idx = 0 +/// for item in items: +/// sum += func(item, idx) +/// idx += 1 +/// ``` +/// +/// Use `enumerate()` instead: +/// ```python +/// sum = 0 +/// for idx, item in enumerate(items): +/// sum += func(item, idx) +/// ``` +/// +/// ## References +/// - [Python documentation: enumerate()](https://docs.python.org/3/library/functions.html#enumerate) +#[violation] +pub struct EnumerateForLoop { + index_var: String, +} + +impl Violation for EnumerateForLoop { + #[derive_message_formats] + fn message(&self) -> String { + let EnumerateForLoop { index_var } = self; + format!("Use enumereate() for index variable `{index_var}` in for loop") + } +} + +/// SIM113 +pub(crate) fn use_enumerate_in_for_loop(checker: &mut Checker, stmt: &Stmt) { + if !checker.semantic().current_scope().kind.is_function() { + return; + } + let Stmt::For(ast::StmtFor { body, .. }) = stmt else { + return; + }; + + // Check if loop body contains a continue statement + if loop_possibly_continues(body) { + return; + }; + // Check if index variable is initialized to zero prior to loop + let Some((prev_stmt, index_var)) = get_candidate_loop_index(checker, stmt) else { + return; + }; + + // Check if loop body contains an index increment statement matching `index_var + if body.iter().any(|stmt| is_index_increment(stmt, index_var)) { + let diagonastic = Diagnostic::new( + EnumerateForLoop { + index_var: checker.generator().expr(index_var), + }, + TextRange::new(prev_stmt.start(), stmt.end()), + ); + checker.diagnostics.push(diagonastic); + } +} + +/// Recursively check if the `for` loop body contains a `continue` statement +fn loop_possibly_continues(body: &[Stmt]) -> bool { + body.iter().any(|stmt| match stmt { + Stmt::Continue(_) => true, + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + loop_possibly_continues(body) + || elif_else_clauses + .iter() + .any(|clause| loop_possibly_continues(&clause.body)) + } + Stmt::With(ast::StmtWith { body, .. }) => loop_possibly_continues(body), + Stmt::Match(ast::StmtMatch { cases, .. }) => cases + .iter() + .any(|MatchCase { body, .. }| loop_possibly_continues(body)), + Stmt::Try(ast::StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }) => { + loop_possibly_continues(body) + || loop_possibly_continues(orelse) + || loop_possibly_continues(finalbody) + || handlers.iter().any(|handler| match handler { + ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + body, .. + }) => loop_possibly_continues(body), + }) + } + + _ => false, + }) +} + +/// Check previous statement of `for` loop to find a possible index variable +/// which is initialized to zero +/// Ex: +/// ```python +/// idx = 0 +/// for item in items: +/// ... +/// ``` +/// Return (&Stmt, &Expr) to initialization stmt and `idx` variable +fn get_candidate_loop_index<'a>( + checker: &'a Checker, + stmt: &'a Stmt, +) -> Option<(&'a Stmt, &'a Expr)> { + let parent = checker.semantic().current_statement_parent()?; + let suite = traversal::suite(stmt, parent)?; + let prev_stmt = traversal::prev_sibling(stmt, suite)?; + // check if it's a possible index initialization i.e. idx = 0 + let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = prev_stmt else { + return None; + }; + let [Expr::Name(ast::ExprName { id: _, .. })] = targets.as_slice() else { + return None; + }; + if let Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) = value.as_ref() + { + if value.is_zero() { + return Some((prev_stmt, &targets[0])); + } + } + + None +} + +// Check if `stmt` is `index_var` += 1 +fn is_index_increment(stmt: &Stmt, index_var: &Expr) -> bool { + let Stmt::AugAssign(ast::StmtAugAssign { + target, op, value, .. + }) = stmt + else { + return false; + }; + if !matches!(op, Operator::Add) { + return false; + } + let Some(_) = is_same_expr(index_var, target) else { + return false; + }; + if let Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) = value.as_ref() + { + if value.is_one() { + return true; + } + } + false +} diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs index 0e4b3b4937686..9200c0468f6f0 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs @@ -4,6 +4,7 @@ pub(crate) use ast_if::*; pub(crate) use ast_ifexp::*; pub(crate) use ast_unary_op::*; pub(crate) use ast_with::*; +pub(crate) use enumerate_for::*; pub(crate) use key_in_dict::*; pub(crate) use open_file_with_context_handler::*; pub(crate) use reimplemented_builtin::*; @@ -17,6 +18,7 @@ mod ast_if; mod ast_ifexp; mod ast_unary_op; mod ast_with; +mod enumerate_for; mod fix_if; mod fix_with; mod key_in_dict; diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap new file mode 100644 index 0000000000000..b5082ca9456cf --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap @@ -0,0 +1,46 @@ +--- +source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs +--- +SIM113.py:4:5: SIM113 Use enumereate() for index variable `idx` in for loop + | +2 | def f(): +3 | # SIM113 +4 | idx = 0 + | _____^ +5 | | for x in iterable: +6 | | g(x, idx) +7 | | idx +=1 +8 | | h(x) + | |____________^ SIM113 + | + +SIM113.py:14:5: SIM113 Use enumereate() for index variable `idx` in for loop + | +12 | # SIM113 +13 | sum = 0 +14 | idx = 0 + | _____^ +15 | | for x in iterable: +16 | | if g(x): +17 | | break +18 | | idx += 1 +19 | | sum += h(x, idx) + | |________________________^ SIM113 + | + +SIM113.py:24:5: SIM113 Use enumereate() for index variable `idx` in for loop + | +22 | def f(): +23 | # SIM113 +24 | idx = 0 + | _____^ +25 | | for x, y in iterable_tuples(): +26 | | g(x) +27 | | h(x, y) +28 | | idx += 1 + | |________________^ SIM113 +29 | +30 | def f(): + | + + diff --git a/crates/ruff_python_ast/src/traversal.rs b/crates/ruff_python_ast/src/traversal.rs index 1e050cfa94b1e..8252da83d11d4 100644 --- a/crates/ruff_python_ast/src/traversal.rs +++ b/crates/ruff_python_ast/src/traversal.rs @@ -79,3 +79,15 @@ pub fn next_sibling<'a>(stmt: &'a Stmt, suite: &'a Suite) -> Option<&'a Stmt> { } None } + +/// Given a [`Stmt`] and its containing [`Suite`], return the previous [`Stmt`] in the [`Suite`]. +pub fn prev_sibling<'a>(stmt: &'a Stmt, suite: &'a Suite) -> Option<&'a Stmt> { + let mut prev = None; + for sibling in suite { + if sibling == stmt { + return prev; + } + prev = Some(sibling); + } + None +} diff --git a/ruff.schema.json b/ruff.schema.json index 20199a2bd9304..3df1141c34074 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3224,6 +3224,7 @@ "SIM11", "SIM110", "SIM112", + "SIM113", "SIM114", "SIM115", "SIM116", From 40a167494043aced27a89869768247ecba892280 Mon Sep 17 00:00:00 2001 From: Chammika Mannakkara Date: Tue, 3 Oct 2023 22:01:00 +0900 Subject: [PATCH 2/8] bug-fix: from rebase, remove num_traits --- .../src/rules/flake8_simplify/rules/enumerate_for.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs index ff19647ab446a..2973a767abc50 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs @@ -1,5 +1,4 @@ -use num_traits::identities::{One, Zero}; -use ruff_python_ast::{self as ast, Constant, ExceptHandler, Expr, MatchCase, Operator, Stmt}; +use ruff_python_ast::{self as ast, Constant, ExceptHandler, Expr, Int, MatchCase, Operator, Stmt}; use ruff_text_size::{Ranged, TextRange}; use crate::rules::flake8_simplify::rules::ast_bool_op::is_same_expr; @@ -144,7 +143,7 @@ fn get_candidate_loop_index<'a>( .. }) = value.as_ref() { - if value.is_zero() { + if matches!(*value, Int::ZERO) { return Some((prev_stmt, &targets[0])); } } @@ -171,7 +170,7 @@ fn is_index_increment(stmt: &Stmt, index_var: &Expr) -> bool { .. }) = value.as_ref() { - if value.is_one() { + if matches!(*value, Int::ONE) { return true; } } From b15e87572f1ce7d0c3154703b756845ff6bf15a5 Mon Sep 17 00:00:00 2001 From: Chammika Mannakkara Date: Tue, 3 Oct 2023 22:24:12 +0900 Subject: [PATCH 3/8] fix documentation --- .../src/rules/flake8_simplify/rules/enumerate_for.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs index 2973a767abc50..ea15bf80ba988 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs @@ -8,14 +8,14 @@ use ruff_python_ast::traversal; use crate::checkers::ast::Checker; -/// ### What it does +/// ## What it does /// Checks for loops which has explicit loop-index variables that can be simplified by using `enumerate()`. /// -/// ## Why this is bad ? +/// ## Why this is bad? /// Using `enumerate()` is more readable and concise. It could lead to more efficient /// and less error-prone code. /// -/// ### Example +/// ## Example /// ```python /// sum = 0 /// idx = 0 @@ -24,7 +24,7 @@ use crate::checkers::ast::Checker; /// idx += 1 /// ``` /// -/// Use `enumerate()` instead: +/// Use instead: /// ```python /// sum = 0 /// for idx, item in enumerate(items): From f957ebdcc877964651098adefd0060748abbf59b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 13 Jan 2024 22:21:23 -0500 Subject: [PATCH 4/8] Fix typos --- .../flake8_simplify/rules/enumerate_for.rs | 69 ++++++++++--------- ...ke8_simplify__tests__SIM113_SIM113.py.snap | 6 +- 2 files changed, 39 insertions(+), 36 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs index f4563a5c60f3f..269f8b5858ca9 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs @@ -8,40 +8,42 @@ use crate::checkers::ast::Checker; use crate::rules::flake8_simplify::rules::ast_bool_op::is_same_expr; /// ## What it does -/// Checks for loops which has explicit loop-index variables that can be simplified by using `enumerate()`. +/// Checks for `for` loops with explicit loop-index variables that can be replaced +/// with `enumerate()`. /// /// ## Why this is bad? -/// Using `enumerate()` is more readable and concise. It could lead to more efficient -/// and less error-prone code. +/// When iterating over a sequence, it's often desirable to keep track of the +/// index of each element alongside the element itself. Prefer the `enumerate` +/// builtin over manually incrementing a counter variable within the loop, as +/// `enumerate` is more concise and idiomatic. /// /// ## Example /// ```python -/// sum = 0 -/// idx = 0 -/// for item in items: -/// sum += func(item, idx) -/// idx += 1 +/// fruits = ["apple", "banana", "cherry"] +/// for fruit in fruits: +/// print(f"{i + 1}. {fruit}") +/// i += 1 /// ``` /// /// Use instead: /// ```python -/// sum = 0 -/// for idx, item in enumerate(items): -/// sum += func(item, idx) +/// fruits = ["apple", "banana", "cherry"] +/// for i, fruit in enumerate(fruits): +/// print(f"{i + 1}. {fruit}") /// ``` /// /// ## References -/// - [Python documentation: enumerate()](https://docs.python.org/3/library/functions.html#enumerate) +/// - [Python documentation: `enumerate`](https://docs.python.org/3/library/functions.html#enumerate) #[violation] pub struct EnumerateForLoop { - index_var: String, + index: String, } impl Violation for EnumerateForLoop { #[derive_message_formats] fn message(&self) -> String { - let EnumerateForLoop { index_var } = self; - format!("Use enumereate() for index variable `{index_var}` in for loop") + let EnumerateForLoop { index } = self; + format!("Use `enumerate()` for index variable `{index}` in `for` loop") } } @@ -54,29 +56,30 @@ pub(crate) fn use_enumerate_in_for_loop(checker: &mut Checker, stmt: &Stmt) { return; }; - // Check if loop body contains a continue statement - if loop_possibly_continues(body) { + // Check if loop body contains a continue statement. + if has_continue(body) { return; }; - // Check if index variable is initialized to zero prior to loop - let Some((prev_stmt, index_var)) = get_candidate_loop_index(checker, stmt) else { + + // Check if index variable is initialized to zero prior to loop. + let Some((prev_stmt, index)) = get_candidate_loop_index(checker, stmt) else { return; }; - // Check if loop body contains an index increment statement matching `index_var - if body.iter().any(|stmt| is_index_increment(stmt, index_var)) { - let diagonastic = Diagnostic::new( + // Check if loop body contains an index increment statement matching `index`. + if body.iter().any(|stmt| is_index_increment(stmt, index)) { + let diagnostic = Diagnostic::new( EnumerateForLoop { - index_var: checker.generator().expr(index_var), + index: checker.generator().expr(index), }, TextRange::new(prev_stmt.start(), stmt.end()), ); - checker.diagnostics.push(diagonastic); + checker.diagnostics.push(diagnostic); } } /// Recursively check if the `for` loop body contains a `continue` statement -fn loop_possibly_continues(body: &[Stmt]) -> bool { +fn has_continue(body: &[Stmt]) -> bool { body.iter().any(|stmt| match stmt { Stmt::Continue(_) => true, Stmt::If(ast::StmtIf { @@ -84,15 +87,15 @@ fn loop_possibly_continues(body: &[Stmt]) -> bool { elif_else_clauses, .. }) => { - loop_possibly_continues(body) + has_continue(body) || elif_else_clauses .iter() - .any(|clause| loop_possibly_continues(&clause.body)) + .any(|clause| has_continue(&clause.body)) } - Stmt::With(ast::StmtWith { body, .. }) => loop_possibly_continues(body), + Stmt::With(ast::StmtWith { body, .. }) => has_continue(body), Stmt::Match(ast::StmtMatch { cases, .. }) => cases .iter() - .any(|MatchCase { body, .. }| loop_possibly_continues(body)), + .any(|MatchCase { body, .. }| has_continue(body)), Stmt::Try(ast::StmtTry { body, handlers, @@ -100,13 +103,13 @@ fn loop_possibly_continues(body: &[Stmt]) -> bool { finalbody, .. }) => { - loop_possibly_continues(body) - || loop_possibly_continues(orelse) - || loop_possibly_continues(finalbody) + has_continue(body) + || has_continue(orelse) + || has_continue(finalbody) || handlers.iter().any(|handler| match handler { ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. - }) => loop_possibly_continues(body), + }) => has_continue(body), }) } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap index b5082ca9456cf..d1d57051cfb49 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs --- -SIM113.py:4:5: SIM113 Use enumereate() for index variable `idx` in for loop +SIM113.py:4:5: SIM113 Use `enumerate()` for index variable `idx` in `for` loop | 2 | def f(): 3 | # SIM113 @@ -14,7 +14,7 @@ SIM113.py:4:5: SIM113 Use enumereate() for index variable `idx` in for loop | |____________^ SIM113 | -SIM113.py:14:5: SIM113 Use enumereate() for index variable `idx` in for loop +SIM113.py:14:5: SIM113 Use `enumerate()` for index variable `idx` in `for` loop | 12 | # SIM113 13 | sum = 0 @@ -28,7 +28,7 @@ SIM113.py:14:5: SIM113 Use enumereate() for index variable `idx` in for loop | |________________________^ SIM113 | -SIM113.py:24:5: SIM113 Use enumereate() for index variable `idx` in for loop +SIM113.py:24:5: SIM113 Use `enumerate()` for index variable `idx` in `for` loop | 22 | def f(): 23 | # SIM113 From 9598f8809907a41473a8be190cbf912232eb8144 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 13 Jan 2024 23:00:36 -0500 Subject: [PATCH 5/8] Use semantic model --- .../test/fixtures/flake8_simplify/SIM113.py | 131 ++++++++++----- .../src/checkers/ast/analyze/statement.rs | 2 +- crates/ruff_linter/src/codes.rs | 2 +- ...enumerate_for.rs => enumerate_for_loop.rs} | 152 +++++++++--------- .../src/rules/flake8_simplify/rules/mod.rs | 4 +- ...ke8_simplify__tests__SIM113_SIM113.py.snap | 71 ++++---- .../src/analyze/typing.rs | 18 +++ crates/ruff_python_semantic/src/scope.rs | 5 + 8 files changed, 230 insertions(+), 155 deletions(-) rename crates/ruff_linter/src/rules/flake8_simplify/rules/{enumerate_for.rs => enumerate_for_loop.rs} (51%) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py index 2fe1a9d21b2da..9214db8c7fef6 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py @@ -1,90 +1,95 @@ - -def f(): +def func(): # SIM113 idx = 0 - for x in iterable: + for x in range(5): g(x, idx) - idx +=1 + idx += 1 h(x) -def f(): +def func(): # SIM113 sum = 0 idx = 0 - for x in iterable: + for x in range(5): if g(x): break idx += 1 sum += h(x, idx) -def f(): +def func(): # SIM113 idx = 0 - for x, y in iterable_tuples(): + for x, y in zip(range(5), range(5)): g(x) h(x, y) idx += 1 - -def f(): - # Current SIM113 doesn't catch this yet because for loop - # doesn't immidiately follow index initialization + + +def func(): + # SIM113 idx = 0 sum = 0 - for x in iterable: + for x in range(5): sum += h(x, idx) idx += 1 -def f(): - # Current SIM113 doesn't catch this due to unpacking in - # in intialization +def func(): + # SIM113 sum, idx = 0, 0 - for x in iterable: + for x in range(5): g(x, idx) - idx +=1 + idx += 1 h(x) -def f(): - # loop doesn't start at zero +def func(): + # OK (index doesn't start at 0 idx = 10 - for x in iterable: + for x in range(5): g(x, idx) - idx +=1 + idx += 1 h(x) -def f(): - # index doesn't increment by one + +def func(): + # OK (incremented by more than 1) idx = 0 - for x in iterable: + for x in range(5): g(x, idx) - idx +=2 + idx += 2 h(x) -def f(): - # index increment inside condition + +def func(): + # OK (incremented in `if`) idx = 0 - for x in iterable: + for x in range(5): if g(x): idx += 1 h(x) - -def f(): - # Continue in match-case + + +def func(): + # OK (`continue` in match-case) idx = 0 - for x in iterable: + for x in range(5): match g(x): - case 1: h(x) - case 2: continue - case _: h(idx) + case 1: + h(x) + case 2: + continue + case _: + h(idx) idx += 1 -def f(): - # Continue inside with clause + +def func(): + # OK (`continue` inside `with` clause) idx = 0 - for x in iterable: + for x in range(5): with context as c: if g(x): continue @@ -92,13 +97,53 @@ def f(): idx += 1 -def f(): - # Continue in try block +def func(): + # OK (incremented in `try` block) idx = 0 - for x in iterable: + for x in range(5): try: g(x, idx) except: h(x) continue - idx += 1 \ No newline at end of file + idx += 1 + + +def func(idx: 0): + # OK (index is an argument) + for x in range(5): + g(x, idx) + idx += 1 + + +def func(x: int): + # OK (index _may_ be non-zero) + if x > 0: + idx = 1 + else: + idx = 0 + for x in range(5): + g(x, idx) + idx += 1 + h(x) + + +def func(): + # OK + idx = 0 + for x in range(5): + g(x, idx) + idx += 1 + idx += 1 + h(x) + + +def func(): + # OK + idx = 0 + for x in range(5): + g(x, idx) + idx += 1 + if idx > 10: + idx *= 2 + h(x) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 72371aabb84c6..71b7512b41ec3 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1312,7 +1312,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { perflint::rules::try_except_in_loop(checker, body); } if checker.enabled(Rule::EnumerateForLoop) { - flake8_simplify::rules::use_enumerate_in_for_loop(checker, stmt); + flake8_simplify::rules::enumerate_for_loop(checker, for_stmt); } } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index cc861c49e1265..d417ef3af9cb5 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -454,7 +454,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Simplify, "109") => (RuleGroup::Stable, rules::flake8_simplify::rules::CompareWithTuple), (Flake8Simplify, "110") => (RuleGroup::Stable, rules::flake8_simplify::rules::ReimplementedBuiltin), (Flake8Simplify, "112") => (RuleGroup::Stable, rules::flake8_simplify::rules::UncapitalizedEnvironmentVariables), - (Flake8Simplify, "113") => (RuleGroup::Stable, rules::flake8_simplify::rules::EnumerateForLoop), + (Flake8Simplify, "113") => (RuleGroup::Preview, rules::flake8_simplify::rules::EnumerateForLoop), (Flake8Simplify, "114") => (RuleGroup::Stable, rules::flake8_simplify::rules::IfWithSameArms), (Flake8Simplify, "115") => (RuleGroup::Stable, rules::flake8_simplify::rules::OpenFileWithContextHandler), (Flake8Simplify, "116") => (RuleGroup::Stable, rules::flake8_simplify::rules::IfElseBlockInsteadOfDictLookup), diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs similarity index 51% rename from crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs rename to crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs index 269f8b5858ca9..d579d973227ab 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs @@ -1,11 +1,10 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::traversal; use ruff_python_ast::{self as ast, ExceptHandler, Expr, Int, MatchCase, Number, Operator, Stmt}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_semantic::analyze::typing; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; -use crate::rules::flake8_simplify::rules::ast_bool_op::is_same_expr; /// ## What it does /// Checks for `for` loops with explicit loop-index variables that can be replaced @@ -48,33 +47,74 @@ impl Violation for EnumerateForLoop { } /// SIM113 -pub(crate) fn use_enumerate_in_for_loop(checker: &mut Checker, stmt: &Stmt) { - if !checker.semantic().current_scope().kind.is_function() { - return; - } - let Stmt::For(ast::StmtFor { body, .. }) = stmt else { +pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) { + // If the loop contains a `continue`, abort. + if has_continue(&for_stmt.body) { return; }; - // Check if loop body contains a continue statement. - if has_continue(body) { - return; - }; + // If the loop contains an increment statement (e.g., `i += 1`)... + for stmt in &for_stmt.body { + if let Some(index) = match_index_increment(stmt) { + // Find the binding corresponding to the augmented assignment (e.g., `i += 1`). + let Some(id) = checker.semantic().resolve_name(index) else { + continue; + }; + let binding = checker.semantic().binding(id); - // Check if index variable is initialized to zero prior to loop. - let Some((prev_stmt, index)) = get_candidate_loop_index(checker, stmt) else { - return; - }; + // If it's not an assignment (e.g., it's a function argument), ignore it. + if !binding.kind.is_assignment() { + continue; + } + + // Ensure that the index variable was initialized to 0. + let Some(value) = typing::find_binding_value(&index.id, binding, checker.semantic()) + else { + continue; + }; + let Expr::NumberLiteral(ast::ExprNumberLiteral { value: num, .. }) = value else { + continue; + }; + let Some(int) = num.as_int() else { + continue; + }; + if *int != Int::ZERO { + continue; + } + + // If the binding is not at the same level as the `for` loop (e.g., it's in an `if`), + // ignore it. + let Some(for_loop_id) = checker.semantic().current_statement_id() else { + continue; + }; + let Some(assignment_id) = binding.source else { + continue; + }; + if !checker.semantic().same_branch(for_loop_id, assignment_id) { + continue; + } + + // If there are multiple assignments to this variable _within_ the loop, ignore it. + if checker + .semantic() + .current_scope() + .get_all(&index.id) + .map(|id| checker.semantic().binding(id)) + .filter(|binding| for_stmt.range().contains_range(binding.range())) + .count() + > 1 + { + continue; + } - // Check if loop body contains an index increment statement matching `index`. - if body.iter().any(|stmt| is_index_increment(stmt, index)) { - let diagnostic = Diagnostic::new( - EnumerateForLoop { - index: checker.generator().expr(index), - }, - TextRange::new(prev_stmt.start(), stmt.end()), - ); - checker.diagnostics.push(diagnostic); + let diagnostic = Diagnostic::new( + EnumerateForLoop { + index: index.id.to_string(), + }, + stmt.range(), + ); + checker.diagnostics.push(diagnostic); + } } } @@ -117,64 +157,30 @@ fn has_continue(body: &[Stmt]) -> bool { }) } -/// Check previous statement of `for` loop to find a possible index variable -/// which is initialized to zero -/// Ex: -/// ```python -/// idx = 0 -/// for item in items: -/// ... -/// ``` -/// Return (&Stmt, &Expr) to initialization stmt and `idx` variable -fn get_candidate_loop_index<'a>( - checker: &'a Checker, - stmt: &'a Stmt, -) -> Option<(&'a Stmt, &'a Expr)> { - let parent = checker.semantic().current_statement_parent()?; - let suite = traversal::suite(stmt, parent)?; - let prev_stmt = traversal::prev_sibling(stmt, suite)?; - // check if it's a possible index initialization i.e. idx = 0 - let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = prev_stmt else { - return None; - }; - let [Expr::Name(ast::ExprName { id: _, .. })] = targets.as_slice() else { - return None; - }; - if let Expr::NumberLiteral(ast::ExprNumberLiteral { - value: Number::Int(value), - .. - }) = value.as_ref() - { - if matches!(*value, Int::ZERO) { - return Some((prev_stmt, &targets[0])); - } - } - - None -} - -// Check if `stmt` is `index_var` += 1 -fn is_index_increment(stmt: &Stmt, index_var: &Expr) -> bool { +/// If the statement is an index increment statement (e.g., `i += 1`), return +/// the name of the index variable. +fn match_index_increment(stmt: &Stmt) -> Option<&ast::ExprName> { let Stmt::AugAssign(ast::StmtAugAssign { - target, op, value, .. + target, + op: Operator::Add, + value, + .. }) = stmt else { - return false; - }; - if !matches!(op, Operator::Add) { - return false; - } - let Some(_) = is_same_expr(index_var, target) else { - return false; + return None; }; + + let name = target.as_name_expr()?; + if let Expr::NumberLiteral(ast::ExprNumberLiteral { value: Number::Int(value), .. }) = value.as_ref() { if matches!(*value, Int::ONE) { - return true; + return Some(name); } } - false + + None } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs index a494f9c91b55f..b2aa7d5259732 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/mod.rs @@ -4,7 +4,7 @@ pub(crate) use ast_ifexp::*; pub(crate) use ast_unary_op::*; pub(crate) use ast_with::*; pub(crate) use collapsible_if::*; -pub(crate) use enumerate_for::*; +pub(crate) use enumerate_for_loop::*; pub(crate) use if_else_block_instead_of_dict_get::*; pub(crate) use if_else_block_instead_of_dict_lookup::*; pub(crate) use if_else_block_instead_of_if_exp::*; @@ -24,7 +24,7 @@ mod ast_ifexp; mod ast_unary_op; mod ast_with; mod collapsible_if; -mod enumerate_for; +mod enumerate_for_loop; mod fix_with; mod if_else_block_instead_of_dict_get; mod if_else_block_instead_of_dict_lookup; diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap index d1d57051cfb49..b8cc349f0e83b 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM113_SIM113.py.snap @@ -1,46 +1,47 @@ --- source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs --- -SIM113.py:4:5: SIM113 Use `enumerate()` for index variable `idx` in `for` loop +SIM113.py:6:9: SIM113 Use `enumerate()` for index variable `idx` in `for` loop | -2 | def f(): -3 | # SIM113 -4 | idx = 0 - | _____^ -5 | | for x in iterable: -6 | | g(x, idx) -7 | | idx +=1 -8 | | h(x) - | |____________^ SIM113 +4 | for x in range(5): +5 | g(x, idx) +6 | idx += 1 + | ^^^^^^^^ SIM113 +7 | h(x) | -SIM113.py:14:5: SIM113 Use `enumerate()` for index variable `idx` in `for` loop - | -12 | # SIM113 -13 | sum = 0 -14 | idx = 0 - | _____^ -15 | | for x in iterable: -16 | | if g(x): -17 | | break -18 | | idx += 1 -19 | | sum += h(x, idx) - | |________________________^ SIM113 +SIM113.py:17:9: SIM113 Use `enumerate()` for index variable `idx` in `for` loop + | +15 | if g(x): +16 | break +17 | idx += 1 + | ^^^^^^^^ SIM113 +18 | sum += h(x, idx) + | + +SIM113.py:27:9: SIM113 Use `enumerate()` for index variable `idx` in `for` loop + | +25 | g(x) +26 | h(x, y) +27 | idx += 1 + | ^^^^^^^^ SIM113 | -SIM113.py:24:5: SIM113 Use `enumerate()` for index variable `idx` in `for` loop - | -22 | def f(): -23 | # SIM113 -24 | idx = 0 - | _____^ -25 | | for x, y in iterable_tuples(): -26 | | g(x) -27 | | h(x, y) -28 | | idx += 1 - | |________________^ SIM113 -29 | -30 | def f(): +SIM113.py:36:9: SIM113 Use `enumerate()` for index variable `idx` in `for` loop + | +34 | for x in range(5): +35 | sum += h(x, idx) +36 | idx += 1 + | ^^^^^^^^ SIM113 + | + +SIM113.py:44:9: SIM113 Use `enumerate()` for index variable `idx` in `for` loop + | +42 | for x in range(5): +43 | g(x, idx) +44 | idx += 1 + | ^^^^^^^^ SIM113 +45 | h(x) | diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index f63a4f5215f28..17a9f5aa8770a 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -645,6 +645,24 @@ pub fn resolve_assignment<'a>( pub fn find_assigned_value<'a>(symbol: &str, semantic: &'a SemanticModel<'a>) -> Option<&'a Expr> { let binding_id = semantic.lookup_symbol(symbol)?; let binding = semantic.binding(binding_id); + find_binding_value(symbol, &binding, semantic) +} + +/// Find the assigned [`Expr`] for a given [`Binding`], if any. +/// +/// For example given: +/// ```python +/// foo = 42 +/// (bar, bla) = 1, "str" +/// ``` +/// +/// This function will return a `NumberLiteral` with value `Int(42)` when called with `foo` and a +/// `StringLiteral` with value `"str"` when called with `bla`. +pub fn find_binding_value<'a>( + symbol: &str, + binding: &Binding, + semantic: &'a SemanticModel, +) -> Option<&'a Expr> { match binding.kind { // Ex) `x := 1` BindingKind::NamedExprAssignment => { diff --git a/crates/ruff_python_semantic/src/scope.rs b/crates/ruff_python_semantic/src/scope.rs index 3de3b058f9192..0699d160b02fb 100644 --- a/crates/ruff_python_semantic/src/scope.rs +++ b/crates/ruff_python_semantic/src/scope.rs @@ -129,6 +129,11 @@ impl<'a> Scope<'a> { self.shadowed_bindings.get(&id).copied() } + /// Returns an iterator over all bindings that the given binding shadows, including itself. + pub fn shadowed_bindings(&self, id: BindingId) -> impl Iterator + '_ { + std::iter::successors(Some(id), |id| self.shadowed_bindings.get(id).copied()) + } + /// Adds a reference to a star import (e.g., `from sys import *`) to this scope. pub fn add_star_import(&mut self, import: StarImport<'a>) { self.star_imports.push(import); From e8b6dfbeb37ee7471a4fdd7dd1cf9699427cd2ea Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 13 Jan 2024 23:22:19 -0500 Subject: [PATCH 6/8] Use visitor --- .../rules/enumerate_for_loop.rs | 97 ++++++++----------- .../flake8_trio/rules/zero_sleep_call.rs | 17 ++-- .../src/analyze/typing.rs | 2 +- 3 files changed, 49 insertions(+), 67 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs index d579d973227ab..55d0ae78ee8ed 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast, ExceptHandler, Expr, Int, MatchCase, Number, Operator, Stmt}; +use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor}; +use ruff_python_ast::{self as ast, Expr, Int, Number, Operator, Stmt}; use ruff_python_semantic::analyze::typing; use ruff_text_size::Ranged; @@ -49,9 +50,11 @@ impl Violation for EnumerateForLoop { /// SIM113 pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) { // If the loop contains a `continue`, abort. - if has_continue(&for_stmt.body) { + let mut visitor = LoopControlFlowVisitor::default(); + visitor.visit_body(&for_stmt.body); + if visitor.has_continue { return; - }; + } // If the loop contains an increment statement (e.g., `i += 1`)... for stmt in &for_stmt.body { @@ -60,9 +63,9 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) let Some(id) = checker.semantic().resolve_name(index) else { continue; }; - let binding = checker.semantic().binding(id); // If it's not an assignment (e.g., it's a function argument), ignore it. + let binding = checker.semantic().binding(id); if !binding.kind.is_assignment() { continue; } @@ -72,13 +75,13 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) else { continue; }; - let Expr::NumberLiteral(ast::ExprNumberLiteral { value: num, .. }) = value else { - continue; - }; - let Some(int) = num.as_int() else { - continue; - }; - if *int != Int::ZERO { + if !matches!( + value, + Expr::NumberLiteral(ast::ExprNumberLiteral { + value: Number::Int(Int::ZERO), + .. + }) + ) { continue; } @@ -118,45 +121,6 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) } } -/// Recursively check if the `for` loop body contains a `continue` statement -fn has_continue(body: &[Stmt]) -> bool { - body.iter().any(|stmt| match stmt { - Stmt::Continue(_) => true, - Stmt::If(ast::StmtIf { - body, - elif_else_clauses, - .. - }) => { - has_continue(body) - || elif_else_clauses - .iter() - .any(|clause| has_continue(&clause.body)) - } - Stmt::With(ast::StmtWith { body, .. }) => has_continue(body), - Stmt::Match(ast::StmtMatch { cases, .. }) => cases - .iter() - .any(|MatchCase { body, .. }| has_continue(body)), - Stmt::Try(ast::StmtTry { - body, - handlers, - orelse, - finalbody, - .. - }) => { - has_continue(body) - || has_continue(orelse) - || has_continue(finalbody) - || handlers.iter().any(|handler| match handler { - ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { - body, .. - }) => has_continue(body), - }) - } - - _ => false, - }) -} - /// If the statement is an index increment statement (e.g., `i += 1`), return /// the name of the index variable. fn match_index_increment(stmt: &Stmt) -> Option<&ast::ExprName> { @@ -172,15 +136,32 @@ fn match_index_increment(stmt: &Stmt) -> Option<&ast::ExprName> { let name = target.as_name_expr()?; - if let Expr::NumberLiteral(ast::ExprNumberLiteral { - value: Number::Int(value), - .. - }) = value.as_ref() - { - if matches!(*value, Int::ONE) { - return Some(name); - } + if matches!( + value.as_ref(), + Expr::NumberLiteral(ast::ExprNumberLiteral { + value: Number::Int(Int::ONE), + .. + }) + ) { + return Some(name); } None } + +#[derive(Debug, Default)] +struct LoopControlFlowVisitor { + has_continue: bool, +} + +impl StatementVisitor<'_> for LoopControlFlowVisitor { + fn visit_stmt(&mut self, stmt: &Stmt) { + match stmt { + Stmt::Continue(_) => self.has_continue = true, + Stmt::FunctionDef(_) | Stmt::ClassDef(_) => { + // Don't recurse. + } + _ => walk_stmt(self, stmt), + } + } +} diff --git a/crates/ruff_linter/src/rules/flake8_trio/rules/zero_sleep_call.rs b/crates/ruff_linter/src/rules/flake8_trio/rules/zero_sleep_call.rs index 57caec4eec68c..20f7654ff5f86 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/rules/zero_sleep_call.rs +++ b/crates/ruff_linter/src/rules/flake8_trio/rules/zero_sleep_call.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast, Expr, ExprCall, Int}; +use ruff_python_ast::{self as ast, Expr, ExprCall, Int, Number}; use ruff_python_semantic::analyze::typing::find_assigned_value; use ruff_text_size::Ranged; @@ -65,8 +65,7 @@ pub(crate) fn zero_sleep_call(checker: &mut Checker, call: &ExprCall) { match arg { Expr::NumberLiteral(ast::ExprNumberLiteral { value, .. }) => { - let Some(int) = value.as_int() else { return }; - if *int != Int::ZERO { + if !matches!(value, Number::Int(Int::ZERO)) { return; } } @@ -74,11 +73,13 @@ pub(crate) fn zero_sleep_call(checker: &mut Checker, call: &ExprCall) { let Some(value) = find_assigned_value(id, checker.semantic()) else { return; }; - let Expr::NumberLiteral(ast::ExprNumberLiteral { value: num, .. }) = value else { - return; - }; - let Some(int) = num.as_int() else { return }; - if *int != Int::ZERO { + if !matches!( + value, + Expr::NumberLiteral(ast::ExprNumberLiteral { + value: Number::Int(Int::ZERO), + .. + }) + ) { return; } } diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index 17a9f5aa8770a..39eec17a53f88 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -645,7 +645,7 @@ pub fn resolve_assignment<'a>( pub fn find_assigned_value<'a>(symbol: &str, semantic: &'a SemanticModel<'a>) -> Option<&'a Expr> { let binding_id = semantic.lookup_symbol(symbol)?; let binding = semantic.binding(binding_id); - find_binding_value(symbol, &binding, semantic) + find_binding_value(symbol, binding, semantic) } /// Find the assigned [`Expr`] for a given [`Binding`], if any. From ef4dae4da4770f0af4af79fa2d0137804a97faa9 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 13 Jan 2024 23:25:48 -0500 Subject: [PATCH 7/8] Fix docs --- .../src/rules/flake8_simplify/rules/enumerate_for_loop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs index 55d0ae78ee8ed..e339596f3f2d3 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs @@ -11,7 +11,7 @@ use crate::checkers::ast::Checker; /// Checks for `for` loops with explicit loop-index variables that can be replaced /// with `enumerate()`. /// -/// ## Why this is bad? +/// ## Why is this bad? /// When iterating over a sequence, it's often desirable to keep track of the /// index of each element alongside the element itself. Prefer the `enumerate` /// builtin over manually incrementing a counter variable within the loop, as From 5c25d6817e88249726ee3e3f9f105650b0c03e64 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 14 Jan 2024 09:54:36 -0500 Subject: [PATCH 8/8] Fix false positives --- .../test/fixtures/flake8_simplify/SIM113.py | 19 +++++++ .../ast/analyze/deferred_for_loops.rs | 5 +- .../src/checkers/ast/analyze/statement.rs | 6 +-- .../rules/enumerate_for_loop.rs | 52 ++++++++++++++----- 4 files changed, 64 insertions(+), 18 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py index 9214db8c7fef6..67ef392337b3c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM113.py @@ -147,3 +147,22 @@ def func(): if idx > 10: idx *= 2 h(x) + + +def func(): + # OK (index used after loop) + idx = 0 + for x in range(5): + g(x, idx) + idx += 1 + h(x) + print(idx) + + +def func(): + # OK (index within nested loop) + idx = 0 + for x in range(5): + for y in range(5): + g(x, idx) + idx += 1 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs index 5d4aa26ed2608..c336121bfdaa1 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs @@ -2,7 +2,7 @@ use ruff_python_ast::Stmt; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::{flake8_bugbear, perflint, pyupgrade, refurb}; +use crate::rules::{flake8_bugbear, flake8_simplify, perflint, pyupgrade, refurb}; /// Run lint rules over all deferred for-loops in the [`SemanticModel`]. pub(crate) fn deferred_for_loops(checker: &mut Checker) { @@ -27,6 +27,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryEnumerate) { refurb::rules::unnecessary_enumerate(checker, stmt_for); } + if checker.enabled(Rule::EnumerateForLoop) { + flake8_simplify::rules::enumerate_for_loop(checker, stmt_for); + } } } } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 71b7512b41ec3..af634230f4709 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1258,9 +1258,10 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { }, ) => { if checker.any_enabled(&[ - Rule::UnusedLoopControlVariable, + Rule::EnumerateForLoop, Rule::IncorrectDictIterator, Rule::UnnecessaryEnumerate, + Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, ]) { checker.deferred.for_loops.push(checker.semantic.snapshot()); @@ -1311,9 +1312,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::TryExceptInLoop) { perflint::rules::try_except_in_loop(checker, body); } - if checker.enabled(Rule::EnumerateForLoop) { - flake8_simplify::rules::enumerate_for_loop(checker, for_stmt); - } } } Stmt::Try(ast::StmtTry { diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs index e339596f3f2d3..95f2237b01952 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/enumerate_for_loop.rs @@ -56,10 +56,10 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) return; } - // If the loop contains an increment statement (e.g., `i += 1`)... for stmt in &for_stmt.body { + // Find the augmented assignment expression (e.g., `i += 1`). if let Some(index) = match_index_increment(stmt) { - // Find the binding corresponding to the augmented assignment (e.g., `i += 1`). + // Find the binding corresponding to the initialization (e.g., `i = 1`). let Some(id) = checker.semantic().resolve_name(index) else { continue; }; @@ -70,6 +70,11 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) continue; } + // If the variable is global or nonlocal, ignore it. + if binding.is_global() || binding.is_nonlocal() { + continue; + } + // Ensure that the index variable was initialized to 0. let Some(value) = typing::find_binding_value(&index.id, binding, checker.semantic()) else { @@ -93,20 +98,41 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor) let Some(assignment_id) = binding.source else { continue; }; - if !checker.semantic().same_branch(for_loop_id, assignment_id) { + if checker.semantic().parent_statement_id(for_loop_id) + != checker.semantic().parent_statement_id(assignment_id) + { continue; } - // If there are multiple assignments to this variable _within_ the loop, ignore it. - if checker - .semantic() - .current_scope() - .get_all(&index.id) - .map(|id| checker.semantic().binding(id)) - .filter(|binding| for_stmt.range().contains_range(binding.range())) - .count() - > 1 - { + // Identify the binding created by the augmented assignment. + // TODO(charlie): There should be a way to go from `ExprName` to `BindingId` (like + // `resolve_name`, but for bindings rather than references). + let binding = { + let mut bindings = checker + .semantic() + .current_scope() + .get_all(&index.id) + .map(|id| checker.semantic().binding(id)) + .filter(|binding| for_stmt.range().contains_range(binding.range())); + + let Some(binding) = bindings.next() else { + continue; + }; + + // If there are multiple assignments to this variable _within_ the loop, ignore it. + if bindings.next().is_some() { + continue; + } + + binding + }; + + // If the variable is used _after_ the loop, ignore it. + // Find the binding for the augmented assignment. + if binding.references.iter().any(|id| { + let reference = checker.semantic().reference(*id); + reference.start() > for_stmt.end() + }) { continue; }