From 511cc25fc41dc19ef8815c9e133e7779650a3c9e Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Tue, 19 Sep 2023 21:19:28 +0100 Subject: [PATCH] [`refurb`] Implement `unnecessary-enumerate` (`FURB148`) (#7454) ## Summary Implement [`no-ignored-enumerate-items`](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/no_ignored_enumerate.py) as `unnecessary-enumerate` (`FURB148`). The auto-fix considers if a `start` argument is passed to the `enumerate()` function. If only the index is used, then the suggested fix is to pass the `start` value to the `range()` function. So, ```python for i, _ in enumerate(xs, 1): ... ``` becomes ```python for i in range(1, len(xs)): ... ``` If the index is ignored and only the value is ignored, and if a start value greater than zero is passed to `enumerate()`, the rule doesn't produce a suggestion. I couldn't find a unanimously accepted best way to iterate over a collection whilst skipping the first n elements. The rule still triggers, however. Related to #1348. ## Test Plan `cargo test` --------- Co-authored-by: Dhruv Manilawala Co-authored-by: Charlie Marsh --- .../resources/test/fixtures/refurb/FURB148.py | 66 ++++ .../ast/analyze/deferred_for_loops.rs | 5 +- .../src/checkers/ast/analyze/statement.rs | 1 + crates/ruff/src/codes.rs | 1 + .../perflint/rules/incorrect_dict_iterator.rs | 38 +-- crates/ruff/src/rules/refurb/mod.rs | 1 + crates/ruff/src/rules/refurb/rules/mod.rs | 2 + .../refurb/rules/unnecessary_enumerate.rs | 261 ++++++++++++++ ...es__refurb__tests__FURB148_FURB148.py.snap | 323 ++++++++++++++++++ crates/ruff_python_semantic/src/model.rs | 35 +- crates/ruff_workspace/src/configuration.rs | 1 + ruff.schema.json | 1 + 12 files changed, 695 insertions(+), 40 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/refurb/FURB148.py create mode 100644 crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs create mode 100644 crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap diff --git a/crates/ruff/resources/test/fixtures/refurb/FURB148.py b/crates/ruff/resources/test/fixtures/refurb/FURB148.py new file mode 100644 index 0000000000000..75f2c8a900603 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/refurb/FURB148.py @@ -0,0 +1,66 @@ +books = ["Dune", "Foundation", "Neuromancer"] + +# Errors +for index, _ in enumerate(books): + print(index) + +for index, _ in enumerate(books, start=0): + print(index) + +for index, _ in enumerate(books, 0): + print(index) + +for index, _ in enumerate(books, start=1): + print(index) + +for index, _ in enumerate(books, 1): + print(index) + +for index, _ in enumerate(books, start=x): + print(book) + +for index, _ in enumerate(books, x): + print(book) + +for _, book in enumerate(books): + print(book) + +for _, book in enumerate(books, start=0): + print(book) + +for _, book in enumerate(books, 0): + print(book) + +for _, book in enumerate(books, start=1): + print(book) + +for _, book in enumerate(books, 1): + print(book) + +for _, book in enumerate(books, start=x): + print(book) + +for _, book in enumerate(books, x): + print(book) + +for index, (_, _) in enumerate(books): + print(index) + +for (_, _), book in enumerate(books): + print(book) + +for(index, _)in enumerate(books): + print(index) + +for(index), _ in enumerate(books): + print(index) + +# OK +for index, book in enumerate(books): + print(index, book) + +for index in range(len(books)): + print(index) + +for book in books: + print(book) diff --git a/crates/ruff/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff/src/checkers/ast/analyze/deferred_for_loops.rs index bb1805ba7790e..5d4aa26ed2608 100644 --- a/crates/ruff/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/crates/ruff/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}; +use crate::rules::{flake8_bugbear, perflint, pyupgrade, refurb}; /// Run lint rules over all deferred for-loops in the [`SemanticModel`]. pub(crate) fn deferred_for_loops(checker: &mut Checker) { @@ -24,6 +24,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) { if checker.enabled(Rule::YieldInForLoop) { pyupgrade::rules::yield_in_for_loop(checker, stmt_for); } + if checker.enabled(Rule::UnnecessaryEnumerate) { + refurb::rules::unnecessary_enumerate(checker, stmt_for); + } } } } diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 9b1bbddee4775..2205632ec1e98 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -1190,6 +1190,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.any_enabled(&[ Rule::UnusedLoopControlVariable, Rule::IncorrectDictIterator, + Rule::UnnecessaryEnumerate, Rule::YieldInForLoop, ]) { checker.deferred.for_loops.push(checker.semantic.snapshot()); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 8a4b6e3353e85..cddcc5d0a7697 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -921,6 +921,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "132") => (RuleGroup::Nursery, rules::refurb::rules::CheckAndRemoveFromSet), (Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap), (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy), + (Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate), // flake8-logging (Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation), diff --git a/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs b/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs index 4858b1a792c67..4ad4b24cd086b 100644 --- a/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs +++ b/crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs @@ -4,7 +4,6 @@ use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::{Arguments, Expr}; -use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -83,8 +82,8 @@ pub(crate) fn incorrect_dict_iterator(checker: &mut Checker, stmt_for: &ast::Stm } match ( - is_unused(key, checker.semantic()), - is_unused(value, checker.semantic()), + checker.semantic().is_unused(key), + checker.semantic().is_unused(value), ) { (true, true) => { // Both the key and the value are unused. @@ -145,36 +144,3 @@ impl fmt::Display for DictSubset { } } } - -/// Returns `true` if the given expression is either an unused value or a tuple of unused values. -fn is_unused(expr: &Expr, semantic: &SemanticModel) -> bool { - match expr { - Expr::Tuple(ast::ExprTuple { elts, .. }) => { - elts.iter().all(|expr| is_unused(expr, semantic)) - } - Expr::Name(ast::ExprName { id, .. }) => { - // Treat a variable as used if it has any usages, _or_ it's shadowed by another variable - // with usages. - // - // If we don't respect shadowing, we'll incorrectly flag `bar` as unused in: - // ```python - // from random import random - // - // for bar in range(10): - // if random() > 0.5: - // break - // else: - // bar = 1 - // - // print(bar) - // ``` - let scope = semantic.current_scope(); - scope - .get_all(id) - .map(|binding_id| semantic.binding(binding_id)) - .filter(|binding| binding.start() >= expr.start()) - .all(|binding| !binding.is_used()) - } - _ => false, - } -} diff --git a/crates/ruff/src/rules/refurb/mod.rs b/crates/ruff/src/rules/refurb/mod.rs index 27b1c59e9f053..73436726c31f9 100644 --- a/crates/ruff/src/rules/refurb/mod.rs +++ b/crates/ruff/src/rules/refurb/mod.rs @@ -19,6 +19,7 @@ mod tests { #[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))] #[test_case(Rule::ReimplementedStarmap, Path::new("FURB140.py"))] #[test_case(Rule::SliceCopy, Path::new("FURB145.py"))] + #[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/refurb/rules/mod.rs b/crates/ruff/src/rules/refurb/rules/mod.rs index 55a5095e251b2..367e357166eb2 100644 --- a/crates/ruff/src/rules/refurb/rules/mod.rs +++ b/crates/ruff/src/rules/refurb/rules/mod.rs @@ -3,9 +3,11 @@ pub(crate) use delete_full_slice::*; pub(crate) use reimplemented_starmap::*; pub(crate) use repeated_append::*; pub(crate) use slice_copy::*; +pub(crate) use unnecessary_enumerate::*; mod check_and_remove_from_set; mod delete_full_slice; mod reimplemented_starmap; mod repeated_append; mod slice_copy; +mod unnecessary_enumerate; diff --git a/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs b/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs new file mode 100644 index 0000000000000..a8a6fceffd198 --- /dev/null +++ b/crates/ruff/src/rules/refurb/rules/unnecessary_enumerate.rs @@ -0,0 +1,261 @@ +use std::fmt; + +use num_traits::Zero; + +use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_ast::{Arguments, Constant, Expr}; +use ruff_python_codegen::Generator; +use ruff_text_size::{Ranged, TextRange}; + +use crate::autofix::edits::pad; +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for uses of `enumerate` that discard either the index or the value +/// when iterating over a sequence. +/// +/// ## Why is this bad? +/// The built-in `enumerate` function is useful when you need both the index and +/// value of a sequence. +/// +/// If you only need the index or values of a sequence, you should iterate over +/// `range(len(...))` or the sequence itself, respectively, instead. This is +/// more efficient and communicates the intent of the code more clearly. +/// +/// ## Example +/// ```python +/// for index, _ in enumerate(sequence): +/// print(index) +/// +/// for _, value in enumerate(sequence): +/// print(value) +/// ``` +/// +/// Use instead: +/// ```python +/// for index in range(len(sequence)): +/// print(index) +/// +/// for value in sequence: +/// print(value) +/// ``` +/// +/// ## References +/// - [Python documentation: `enumerate`](https://docs.python.org/3/library/functions.html#enumerate) +/// - [Python documentation: `range`](https://docs.python.org/3/library/stdtypes.html#range) +/// - [Python documentation: `len`](https://docs.python.org/3/library/functions.html#len) +#[violation] +pub struct UnnecessaryEnumerate { + subset: EnumerateSubset, +} + +impl Violation for UnnecessaryEnumerate { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let UnnecessaryEnumerate { subset } = self; + match subset { + EnumerateSubset::Indices => { + format!("`enumerate` value is unused, use `for x in range(len(y))` instead") + } + EnumerateSubset::Values => { + format!("`enumerate` index is unused, use `for x in y` instead") + } + } + } + + fn autofix_title(&self) -> Option { + let UnnecessaryEnumerate { subset } = self; + match subset { + EnumerateSubset::Indices => Some("Replace with `range(len(...))`".to_string()), + EnumerateSubset::Values => Some("Remove `enumerate`".to_string()), + } + } +} + +/// FURB148 +pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtFor) { + // Check the for statement is of the form `for x, y in func(...)`. + let Expr::Tuple(ast::ExprTuple { elts, .. }) = stmt_for.target.as_ref() else { + return; + }; + let [index, value] = elts.as_slice() else { + return; + }; + let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = stmt_for.iter.as_ref() + else { + return; + }; + + // Check that the function is the `enumerate` builtin. + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return; + }; + if id != "enumerate" { + return; + }; + if !checker.semantic().is_builtin("enumerate") { + return; + }; + + // Get the first argument, which is the sequence to iterate over. + let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { + return; + }; + + // Check if the index and value are used. + match ( + checker.semantic().is_unused(index), + checker.semantic().is_unused(value), + ) { + (true, true) => { + // Both the index and the value are unused. + } + (false, false) => { + // Both the index and the value are used. + } + (true, false) => { + let mut diagnostic = Diagnostic::new( + UnnecessaryEnumerate { + subset: EnumerateSubset::Values, + }, + func.range(), + ); + + // The index is unused, so replace with `for value in sequence`. + if checker.patch(diagnostic.kind.rule()) { + let replace_iter = Edit::range_replacement(sequence.into(), stmt_for.iter.range()); + let replace_target = Edit::range_replacement( + pad( + checker.locator().slice(value).to_string(), + stmt_for.target.range(), + checker.locator(), + ), + stmt_for.target.range(), + ); + diagnostic.set_fix(Fix::suggested_edits(replace_iter, [replace_target])); + } + + checker.diagnostics.push(diagnostic); + } + (false, true) => { + // The value is unused, so replace with `for index in range(len(sequence))`. + let mut diagnostic = Diagnostic::new( + UnnecessaryEnumerate { + subset: EnumerateSubset::Indices, + }, + func.range(), + ); + if checker.patch(diagnostic.kind.rule()) + && checker.semantic().is_builtin("range") + && checker.semantic().is_builtin("len") + { + // If the `start` argument is set to something other than the `range` default, + // there's no clear fix. + let start = arguments.find_argument("start", 1); + if start.map_or(true, |start| { + if let Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) = start + { + value.is_zero() + } else { + false + } + }) { + let replace_iter = Edit::range_replacement( + generate_range_len_call(sequence, checker.generator()), + stmt_for.iter.range(), + ); + + let replace_target = Edit::range_replacement( + pad( + checker.locator().slice(index).to_string(), + stmt_for.target.range(), + checker.locator(), + ), + stmt_for.target.range(), + ); + + diagnostic.set_fix(Fix::suggested_edits(replace_iter, [replace_target])); + } + } + checker.diagnostics.push(diagnostic); + } + } +} + +#[derive(Debug, PartialEq, Eq)] +enum EnumerateSubset { + /// E.g., `for _, value in enumerate(sequence):`. + Indices, + /// E.g., `for index, _ in enumerate(sequence):`. + Values, +} + +impl fmt::Display for EnumerateSubset { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + EnumerateSubset::Indices => write!(f, "indices"), + EnumerateSubset::Values => write!(f, "values"), + } + } +} + +/// Format a code snippet to call `range(len(name))`, where `name` is the given +/// sequence name. +fn generate_range_len_call(name: &str, generator: Generator) -> String { + // Construct `name`. + let var = ast::ExprName { + id: name.to_string(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + // Construct `len(name)`. + let len = ast::ExprCall { + func: Box::new( + ast::ExprName { + id: "len".to_string(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + } + .into(), + ), + arguments: Arguments { + args: vec![var.into()], + keywords: vec![], + range: TextRange::default(), + }, + range: TextRange::default(), + }; + // Construct `range(len(name))`. + let range = ast::ExprCall { + func: Box::new( + ast::ExprName { + id: "range".to_string(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + } + .into(), + ), + arguments: Arguments { + args: vec![len.into()], + keywords: vec![], + range: TextRange::default(), + }, + range: TextRange::default(), + }; + // And finally, turn it into a statement. + let stmt = ast::StmtExpr { + value: Box::new(range.into()), + range: TextRange::default(), + }; + generator.stmt(&stmt.into()) +} diff --git a/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap new file mode 100644 index 0000000000000..1c4f0e78f1e8a --- /dev/null +++ b/crates/ruff/src/rules/refurb/snapshots/ruff__rules__refurb__tests__FURB148_FURB148.py.snap @@ -0,0 +1,323 @@ +--- +source: crates/ruff/src/rules/refurb/mod.rs +--- +FURB148.py:4:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +3 | # Errors +4 | for index, _ in enumerate(books): + | ^^^^^^^^^ FURB148 +5 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +1 1 | books = ["Dune", "Foundation", "Neuromancer"] +2 2 | +3 3 | # Errors +4 |-for index, _ in enumerate(books): + 4 |+for index in range(len(books)): +5 5 | print(index) +6 6 | +7 7 | for index, _ in enumerate(books, start=0): + +FURB148.py:7:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +5 | print(index) +6 | +7 | for index, _ in enumerate(books, start=0): + | ^^^^^^^^^ FURB148 +8 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +4 4 | for index, _ in enumerate(books): +5 5 | print(index) +6 6 | +7 |-for index, _ in enumerate(books, start=0): + 7 |+for index in range(len(books)): +8 8 | print(index) +9 9 | +10 10 | for index, _ in enumerate(books, 0): + +FURB148.py:10:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | + 8 | print(index) + 9 | +10 | for index, _ in enumerate(books, 0): + | ^^^^^^^^^ FURB148 +11 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +7 7 | for index, _ in enumerate(books, start=0): +8 8 | print(index) +9 9 | +10 |-for index, _ in enumerate(books, 0): + 10 |+for index in range(len(books)): +11 11 | print(index) +12 12 | +13 13 | for index, _ in enumerate(books, start=1): + +FURB148.py:13:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead + | +11 | print(index) +12 | +13 | for index, _ in enumerate(books, start=1): + | ^^^^^^^^^ FURB148 +14 | print(index) + | + = help: Replace with `range(len(...))` + +FURB148.py:16:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead + | +14 | print(index) +15 | +16 | for index, _ in enumerate(books, 1): + | ^^^^^^^^^ FURB148 +17 | print(index) + | + = help: Replace with `range(len(...))` + +FURB148.py:19:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead + | +17 | print(index) +18 | +19 | for index, _ in enumerate(books, start=x): + | ^^^^^^^^^ FURB148 +20 | print(book) + | + = help: Replace with `range(len(...))` + +FURB148.py:22:17: FURB148 `enumerate` value is unused, use `for x in range(len(y))` instead + | +20 | print(book) +21 | +22 | for index, _ in enumerate(books, x): + | ^^^^^^^^^ FURB148 +23 | print(book) + | + = help: Replace with `range(len(...))` + +FURB148.py:25:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +23 | print(book) +24 | +25 | for _, book in enumerate(books): + | ^^^^^^^^^ FURB148 +26 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +22 22 | for index, _ in enumerate(books, x): +23 23 | print(book) +24 24 | +25 |-for _, book in enumerate(books): + 25 |+for book in books: +26 26 | print(book) +27 27 | +28 28 | for _, book in enumerate(books, start=0): + +FURB148.py:28:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +26 | print(book) +27 | +28 | for _, book in enumerate(books, start=0): + | ^^^^^^^^^ FURB148 +29 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +25 25 | for _, book in enumerate(books): +26 26 | print(book) +27 27 | +28 |-for _, book in enumerate(books, start=0): + 28 |+for book in books: +29 29 | print(book) +30 30 | +31 31 | for _, book in enumerate(books, 0): + +FURB148.py:31:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +29 | print(book) +30 | +31 | for _, book in enumerate(books, 0): + | ^^^^^^^^^ FURB148 +32 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +28 28 | for _, book in enumerate(books, start=0): +29 29 | print(book) +30 30 | +31 |-for _, book in enumerate(books, 0): + 31 |+for book in books: +32 32 | print(book) +33 33 | +34 34 | for _, book in enumerate(books, start=1): + +FURB148.py:34:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +32 | print(book) +33 | +34 | for _, book in enumerate(books, start=1): + | ^^^^^^^^^ FURB148 +35 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +31 31 | for _, book in enumerate(books, 0): +32 32 | print(book) +33 33 | +34 |-for _, book in enumerate(books, start=1): + 34 |+for book in books: +35 35 | print(book) +36 36 | +37 37 | for _, book in enumerate(books, 1): + +FURB148.py:37:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +35 | print(book) +36 | +37 | for _, book in enumerate(books, 1): + | ^^^^^^^^^ FURB148 +38 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +34 34 | for _, book in enumerate(books, start=1): +35 35 | print(book) +36 36 | +37 |-for _, book in enumerate(books, 1): + 37 |+for book in books: +38 38 | print(book) +39 39 | +40 40 | for _, book in enumerate(books, start=x): + +FURB148.py:40:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +38 | print(book) +39 | +40 | for _, book in enumerate(books, start=x): + | ^^^^^^^^^ FURB148 +41 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +37 37 | for _, book in enumerate(books, 1): +38 38 | print(book) +39 39 | +40 |-for _, book in enumerate(books, start=x): + 40 |+for book in books: +41 41 | print(book) +42 42 | +43 43 | for _, book in enumerate(books, x): + +FURB148.py:43:16: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +41 | print(book) +42 | +43 | for _, book in enumerate(books, x): + | ^^^^^^^^^ FURB148 +44 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +40 40 | for _, book in enumerate(books, start=x): +41 41 | print(book) +42 42 | +43 |-for _, book in enumerate(books, x): + 43 |+for book in books: +44 44 | print(book) +45 45 | +46 46 | for index, (_, _) in enumerate(books): + +FURB148.py:46:22: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +44 | print(book) +45 | +46 | for index, (_, _) in enumerate(books): + | ^^^^^^^^^ FURB148 +47 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +43 43 | for _, book in enumerate(books, x): +44 44 | print(book) +45 45 | +46 |-for index, (_, _) in enumerate(books): + 46 |+for index in range(len(books)): +47 47 | print(index) +48 48 | +49 49 | for (_, _), book in enumerate(books): + +FURB148.py:49:21: FURB148 [*] `enumerate` index is unused, use `for x in y` instead + | +47 | print(index) +48 | +49 | for (_, _), book in enumerate(books): + | ^^^^^^^^^ FURB148 +50 | print(book) + | + = help: Remove `enumerate` + +ℹ Suggested fix +46 46 | for index, (_, _) in enumerate(books): +47 47 | print(index) +48 48 | +49 |-for (_, _), book in enumerate(books): + 49 |+for book in books: +50 50 | print(book) +51 51 | +52 52 | for(index, _)in enumerate(books): + +FURB148.py:52:17: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +50 | print(book) +51 | +52 | for(index, _)in enumerate(books): + | ^^^^^^^^^ FURB148 +53 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +49 49 | for (_, _), book in enumerate(books): +50 50 | print(book) +51 51 | +52 |-for(index, _)in enumerate(books): + 52 |+for index in range(len(books)): +53 53 | print(index) +54 54 | +55 55 | for(index), _ in enumerate(books): + +FURB148.py:55:18: FURB148 [*] `enumerate` value is unused, use `for x in range(len(y))` instead + | +53 | print(index) +54 | +55 | for(index), _ in enumerate(books): + | ^^^^^^^^^ FURB148 +56 | print(index) + | + = help: Replace with `range(len(...))` + +ℹ Suggested fix +52 52 | for(index, _)in enumerate(books): +53 53 | print(index) +54 54 | +55 |-for(index), _ in enumerate(books): + 55 |+for index in range(len(books)): +56 56 | print(index) +57 57 | +58 58 | # OK + + diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index eb353c736d851..f8e3f8438661c 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1107,9 +1107,38 @@ impl<'a> SemanticModel<'a> { .all(|(left, right)| left == right) } - /// Returns `true` if the given [`BindingId`] is used. - pub fn is_used(&self, binding_id: BindingId) -> bool { - self.bindings[binding_id].is_used() + /// Returns `true` if the given expression is an unused variable, or consists solely of + /// references to other unused variables. This method is conservative in that it considers a + /// variable to be "used" if it's shadowed by another variable with usages. + pub fn is_unused(&self, expr: &Expr) -> bool { + match expr { + Expr::Tuple(ast::ExprTuple { elts, .. }) => { + elts.iter().all(|expr| self.is_unused(expr)) + } + Expr::Name(ast::ExprName { id, .. }) => { + // Treat a variable as used if it has any usages, _or_ it's shadowed by another variable + // with usages. + // + // If we don't respect shadowing, we'll incorrectly flag `bar` as unused in: + // ```python + // from random import random + // + // for bar in range(10): + // if random() > 0.5: + // break + // else: + // bar = 1 + // + // print(bar) + // ``` + self.current_scope() + .get_all(id) + .map(|binding_id| self.binding(binding_id)) + .filter(|binding| binding.start() >= expr.start()) + .all(|binding| !binding.is_used()) + } + _ => false, + } } /// Add a reference to the given [`BindingId`] in the local scope. diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index e07b92ad23293..82ffa2ed49ee6 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -861,6 +861,7 @@ mod tests { Rule::TooManyPublicMethods, Rule::TooManyPublicMethods, Rule::UndocumentedWarn, + Rule::UnnecessaryEnumerate, ]; #[allow(clippy::needless_pass_by_value)] diff --git a/ruff.schema.json b/ruff.schema.json index 3d32bf5eeec6a..16ef5d54ca28c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2093,6 +2093,7 @@ "FURB14", "FURB140", "FURB145", + "FURB148", "G", "G0", "G00",