diff --git a/README.md b/README.md index 71131843a3..38582ee309 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,7 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | | B002 | UnaryPrefixIncrement | Python does not support the unary prefix increment. | | +| B007 | UnusedLoopControlVariable | Loop control variable `i` not used within the loop body. | 🛠 | | B011 | DoNotAssertFalse | Do not `assert False` (`python -O` removes these calls), raise `AssertionError()` | 🛠 | | B014 | DuplicateHandlerException | Exception handler with duplicate exception: `ValueError` | 🛠 | | B017 | NoAssertRaisesException | `assertRaises(Exception):` should be considered evil. | | @@ -473,7 +474,7 @@ including: - [`flake8-super`](https://pypi.org/project/flake8-super/) - [`flake8-print`](https://pypi.org/project/flake8-print/) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (8/32) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (9/32) - [`pyupgrade`](https://pypi.org/project/pyupgrade/) (8/34) - [`autoflake`](https://pypi.org/project/autoflake/) (1/7) @@ -493,7 +494,7 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl - [`flake8-super`](https://pypi.org/project/flake8-super/) - [`flake8-print`](https://pypi.org/project/flake8-print/) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (8/32) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (9/32) Ruff also implements the functionality that you get from [`yesqa`](https://github.com/asottile/yesqa), and a subset of the rules implemented in [`pyupgrade`](https://pypi.org/project/pyupgrade/) (8/34). diff --git a/resources/test/fixtures/B007.py b/resources/test/fixtures/B007.py new file mode 100644 index 0000000000..be623afa8e --- /dev/null +++ b/resources/test/fixtures/B007.py @@ -0,0 +1,31 @@ +for i in range(10): + print(i) + +print(i) # name no longer defined on Python 3; no warning yet + +for i in range(10): # name not used within the loop; B007 + print(10) + +print(i) # name no longer defined on Python 3; no warning yet + + +for _ in range(10): # _ is okay for a throw-away variable + print(10) + + +for i in range(10): + for j in range(10): + for k in range(10): # k not used, i and j used transitively + print(i + j) + + +def strange_generator(): + for i in range(10): + for j in range(10): + for k in range(10): + for l in range(10): + yield i, (j, (k, l)) + + +for i, (j, (k, l)) in strange_generator(): # i, k not used + print(j, l) diff --git a/src/check_ast.rs b/src/check_ast.rs index b3aca6acde..7a92234360 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -687,6 +687,11 @@ where flake8_bugbear::plugins::assert_raises_exception(self, stmt, items); } } + StmtKind::For { target, body, .. } => { + if self.settings.enabled.contains(&CheckCode::B007) { + flake8_bugbear::plugins::unused_loop_control_variable(self, target, body); + } + } StmtKind::Try { handlers, .. } => { if self.settings.enabled.contains(&CheckCode::F707) { if let Some(check) = pyflakes::checks::default_except_not_last(handlers) { diff --git a/src/checks.rs b/src/checks.rs index 8ff18f335f..07ab36b46e 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -76,6 +76,7 @@ pub enum CheckCode { A003, // flake8-bugbear B002, + B007, B011, B014, B017, @@ -268,6 +269,7 @@ pub enum CheckKind { BuiltinAttributeShadowing(String), // flake8-bugbear UnaryPrefixIncrement, + UnusedLoopControlVariable(String), DoNotAssertFalse, DuplicateHandlerException(Vec), NoAssertRaisesException, @@ -429,6 +431,7 @@ impl CheckCode { CheckCode::A003 => CheckKind::BuiltinAttributeShadowing("...".to_string()), // flake8-bugbear CheckCode::B002 => CheckKind::UnaryPrefixIncrement, + CheckCode::B007 => CheckKind::UnusedLoopControlVariable("i".to_string()), CheckCode::B011 => CheckKind::DoNotAssertFalse, CheckCode::B014 => CheckKind::DuplicateHandlerException(vec!["ValueError".to_string()]), CheckCode::B017 => CheckKind::NoAssertRaisesException, @@ -605,6 +608,7 @@ impl CheckCode { CheckCode::A002 => CheckCategory::Flake8Builtins, CheckCode::A003 => CheckCategory::Flake8Builtins, CheckCode::B002 => CheckCategory::Flake8Bugbear, + CheckCode::B007 => CheckCategory::Flake8Bugbear, CheckCode::B011 => CheckCategory::Flake8Bugbear, CheckCode::B014 => CheckCategory::Flake8Bugbear, CheckCode::B017 => CheckCategory::Flake8Bugbear, @@ -750,6 +754,7 @@ impl CheckKind { CheckKind::BuiltinAttributeShadowing(_) => &CheckCode::A003, // flake8-bugbear CheckKind::UnaryPrefixIncrement => &CheckCode::B002, + CheckKind::UnusedLoopControlVariable(_) => &CheckCode::B007, CheckKind::DoNotAssertFalse => &CheckCode::B011, CheckKind::DuplicateHandlerException(_) => &CheckCode::B014, CheckKind::NoAssertRaisesException => &CheckCode::B017, @@ -990,6 +995,7 @@ impl CheckKind { } // flake8-bugbear CheckKind::UnaryPrefixIncrement => "Python does not support the unary prefix increment. Writing `++n` is equivalent to `+(+(n))`, which equals `n`. You meant `n += 1`.".to_string(), + CheckKind::UnusedLoopControlVariable(name) => format!("Loop control variable `{name}` not used within the loop body. If this is intended, start the name with an underscore."), CheckKind::DoNotAssertFalse => { "Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`" .to_string() @@ -1278,6 +1284,9 @@ impl CheckKind { CheckKind::UnaryPrefixIncrement => { "Python does not support the unary prefix increment.".to_string() } + CheckKind::UnusedLoopControlVariable(name) => { + format!("Loop control variable `{name}` not used within the loop body.") + } CheckKind::NoAssertRaisesException => { "`assertRaises(Exception):` should be considered evil.".to_string() } @@ -1320,6 +1329,7 @@ impl CheckKind { | CheckKind::TypeOfPrimitive(_) | CheckKind::UnnecessaryAbspath | CheckKind::UnusedImport(_) + | CheckKind::UnusedLoopControlVariable(_) | CheckKind::UnusedNOQA(_) | CheckKind::UsePEP585Annotation(_) | CheckKind::UsePEP604Annotation diff --git a/src/flake8_bugbear/plugins/mod.rs b/src/flake8_bugbear/plugins/mod.rs index 6d300405a1..783e0379f2 100644 --- a/src/flake8_bugbear/plugins/mod.rs +++ b/src/flake8_bugbear/plugins/mod.rs @@ -3,8 +3,10 @@ pub use assert_raises_exception::assert_raises_exception; pub use duplicate_exceptions::duplicate_exceptions; pub use duplicate_exceptions::duplicate_handler_exceptions; pub use unary_prefix_increment::unary_prefix_increment; +pub use unused_loop_control_variable::unused_loop_control_variable; mod assert_false; mod assert_raises_exception; mod duplicate_exceptions; mod unary_prefix_increment; +mod unused_loop_control_variable; diff --git a/src/flake8_bugbear/plugins/unused_loop_control_variable.rs b/src/flake8_bugbear/plugins/unused_loop_control_variable.rs new file mode 100644 index 0000000000..9eb0cd84dd --- /dev/null +++ b/src/flake8_bugbear/plugins/unused_loop_control_variable.rs @@ -0,0 +1,79 @@ +use std::collections::BTreeMap; + +use rustpython_ast::{Expr, ExprKind, Stmt}; + +use crate::ast::types::Range; +use crate::ast::visitor; +use crate::ast::visitor::Visitor; +use crate::autofix::Fix; +use crate::check_ast::Checker; +use crate::checks::{Check, CheckKind}; + +/// Identify all `ExprKind::Name` nodes in an AST. +struct NameFinder<'a> { + /// A map from identifier to defining expression. + names: BTreeMap<&'a str, &'a Expr>, +} + +impl NameFinder<'_> { + fn new() -> Self { + NameFinder { + names: Default::default(), + } + } +} + +impl<'a, 'b> Visitor<'b> for NameFinder<'a> +where + 'b: 'a, +{ + fn visit_expr(&mut self, expr: &'a Expr) { + if let ExprKind::Name { id, .. } = &expr.node { + self.names.insert(id, expr); + } + visitor::walk_expr(self, expr); + } +} + +/// B007 +pub fn unused_loop_control_variable(checker: &mut Checker, target: &Expr, body: &[Stmt]) { + let control_names = { + let mut finder = NameFinder::new(); + finder.visit_expr(target); + finder.names + }; + + let used_names = { + let mut finder = NameFinder::new(); + for stmt in body { + finder.visit_stmt(stmt); + } + finder.names + }; + + for (name, expr) in control_names { + // Ignore names that are already underscore-prefixed. + if name.starts_with('_') { + continue; + } + + // Ignore any names that are actually used in the loop body. + if used_names.contains_key(name) { + continue; + } + + let mut check = Check::new( + CheckKind::UnusedLoopControlVariable(name.to_string()), + Range::from_located(expr), + ); + if checker.patch() { + // Prefix the variable name with an underscore. + check.amend(Fix::replacement( + format!("_{name}"), + expr.location, + expr.end_location.unwrap(), + )) + } + checker.add_check(check); + } +} diff --git a/src/linter.rs b/src/linter.rs index dfed5179c8..202c1155ae 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -246,6 +246,7 @@ mod tests { #[test_case(CheckCode::A002, Path::new("A002.py"); "A002")] #[test_case(CheckCode::A003, Path::new("A003.py"); "A003")] #[test_case(CheckCode::B002, Path::new("B002.py"); "B002")] + #[test_case(CheckCode::B007, Path::new("B007.py"); "B007")] #[test_case(CheckCode::B011, Path::new("B011.py"); "B011")] #[test_case(CheckCode::B014, Path::new("B014.py"); "B014")] #[test_case(CheckCode::B017, Path::new("B017.py"); "B017")] diff --git a/src/snapshots/ruff__linter__tests__B007_B007.py.snap b/src/snapshots/ruff__linter__tests__B007_B007.py.snap new file mode 100644 index 0000000000..1009bed657 --- /dev/null +++ b/src/snapshots/ruff__linter__tests__B007_B007.py.snap @@ -0,0 +1,77 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: + UnusedLoopControlVariable: i + location: + row: 6 + column: 5 + end_location: + row: 6 + column: 6 + fix: + patch: + content: _i + location: + row: 6 + column: 5 + end_location: + row: 6 + column: 6 + applied: false +- kind: + UnusedLoopControlVariable: k + location: + row: 18 + column: 13 + end_location: + row: 18 + column: 14 + fix: + patch: + content: _k + location: + row: 18 + column: 13 + end_location: + row: 18 + column: 14 + applied: false +- kind: + UnusedLoopControlVariable: i + location: + row: 30 + column: 5 + end_location: + row: 30 + column: 6 + fix: + patch: + content: _i + location: + row: 30 + column: 5 + end_location: + row: 30 + column: 6 + applied: false +- kind: + UnusedLoopControlVariable: k + location: + row: 30 + column: 13 + end_location: + row: 30 + column: 14 + fix: + patch: + content: _k + location: + row: 30 + column: 13 + end_location: + row: 30 + column: 14 + applied: false +