From 76e178a3a8dc037386442842d2299fcee6c56a22 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 16 Oct 2023 21:49:11 -0400 Subject: [PATCH 01/19] add PLR1736 and autofix --- .../pylint/unnecessary_list_index_lookup.py | 5 + .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 4 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../rules/unnecessary_list_index_lookup.rs | 142 ++++++++++++++++++ ...1736_unnecessary_list_index_lookup.py.snap | 37 +++++ ruff.schema.json | 2 + 8 files changed, 196 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py new file mode 100644 index 0000000000000..4ee655227c08f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -0,0 +1,5 @@ +letters = ["a", "b", "c"] + +for index, letter in enumerate(letters): + print(letters[index]) # PLR1736 + blah = letters[index] # PLR1736 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 48343b3825c7c..497408e3ca37e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1271,6 +1271,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListCast) { perflint::rules::unnecessary_list_cast(checker, iter, body); } + if checker.enabled(Rule::UnnecessaryListIndexLookup) { + pylint::rules::unnecessary_list_index_lookup(checker, for_stmt); + } if !is_async { if checker.enabled(Rule::ReimplementedBuiltin) { flake8_simplify::rules::convert_for_loop_to_any_all(checker, stmt); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 1e04278e0f15b..76f5e78c143cc 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -257,6 +257,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary), + (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison), (Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index e2f272619b2ce..c127d09b2d0f9 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -154,6 +154,10 @@ mod tests { Rule::RepeatedKeywordArgument, Path::new("repeated_keyword_argument.py") )] + #[test_case( + Rule::UnnecessaryListIndexLookup, + Path::new("unnecessary_list_index_lookup.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_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index b130d36260182..c18250188e2b7 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -63,6 +63,7 @@ pub(crate) use type_param_name_mismatch::*; pub(crate) use unexpected_special_method_signature::*; pub(crate) use unnecessary_direct_lambda_call::*; pub(crate) use unnecessary_lambda::*; +pub(crate) use unnecessary_list_index_lookup::*; pub(crate) use unspecified_encoding::*; pub(crate) use useless_else_on_loop::*; pub(crate) use useless_import_alias::*; @@ -136,6 +137,7 @@ mod type_param_name_mismatch; mod unexpected_special_method_signature; mod unnecessary_direct_lambda_call; mod unnecessary_lambda; +mod unnecessary_list_index_lookup; mod unspecified_encoding; mod useless_else_on_loop; mod useless_import_alias; diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs new file mode 100644 index 0000000000000..58d32d87839f8 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -0,0 +1,142 @@ +use ruff_python_ast::{self as ast, Expr, StmtFor}; + +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::visitor; +use ruff_python_ast::visitor::Visitor; +use ruff_text_size::TextRange; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for uses of enumeration and accessing the value by index lookup. +/// +/// ## Why is this bad? +/// The value is already accessible by the 2nd variable from the enumeration. +/// +/// ## Example +/// ```python +/// letters = ["a", "b", "c"] +/// +/// for index, letter in enumerate(letters): +/// print(letters[index]) +/// ``` +/// +/// Use instead: +/// ```python +/// letters = ["a", "b", "c"] +/// +/// for index, letter in enumerate(letters): +/// print(letter) +/// ``` +#[violation] +pub struct UnnecessaryListIndexLookup; + +impl AlwaysFixableViolation for UnnecessaryListIndexLookup { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unnecessary list index lookup") + } + + fn fix_title(&self) -> String { + format!("Remove unnecessary list index lookup") + } +} + +struct SubscriptVisitor<'a> { + sequence_name: &'a str, + index_name: &'a str, + diagnostic_ranges: Vec, +} + +impl<'a> SubscriptVisitor<'a> { + fn new(sequence_name: &'a str, index_name: &'a str) -> Self { + Self { + sequence_name, + index_name, + diagnostic_ranges: Vec::new(), + } + } +} + +impl<'a> Visitor<'_> for SubscriptVisitor<'a> { + fn visit_expr(&mut self, expr: &Expr) { + match expr { + Expr::Subscript(ast::ExprSubscript { + value, + slice, + range, + .. + }) => { + if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { + if id == self.sequence_name { + if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { + if id == self.index_name { + self.diagnostic_ranges.push(*range); + } + } + } + } + } + _ => visitor::walk_expr(self, expr), + } + } +} + +/// PLR1736 +pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &StmtFor) { + 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; + }; + + let Expr::Tuple(ast::ExprTuple { elts, .. }) = stmt_for.target.as_ref() else { + return; + }; + let [index, value] = elts.as_slice() else { + return; + }; + + // Grab the variable names + let Expr::Name(ast::ExprName { id: index_name, .. }) = index else { + return; + }; + + let Expr::Name(ast::ExprName { id: value_name, .. }) = value else { + return; + }; + + // Get the first argument of the enumerate call + let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { + return; + }; + + let mut visitor = SubscriptVisitor::new(sequence, index_name); + + visitor.visit_body(&stmt_for.body); + visitor.visit_body(&stmt_for.orelse); + + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); + + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap new file mode 100644 index 0000000000000..c898cf48628e2 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -0,0 +1,37 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +unnecessary_list_index_lookup.py:4:11: PLR1736 [*] Unnecessary list index lookup + | +3 | for index, letter in enumerate(letters): +4 | print(letters[index]) # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +5 | blah = letters[index] # PLR1736 + | + = help: Remove unnecessary list index lookup + +ℹ Fix +1 1 | letters = ["a", "b", "c"] +2 2 | +3 3 | for index, letter in enumerate(letters): +4 |- print(letters[index]) # PLR1736 + 4 |+ print(letter) # PLR1736 +5 5 | blah = letters[index] # PLR1736 + +unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup + | +3 | for index, letter in enumerate(letters): +4 | print(letters[index]) # PLR1736 +5 | blah = letters[index] # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 + | + = help: Remove unnecessary list index lookup + +ℹ Fix +2 2 | +3 3 | for index, letter in enumerate(letters): +4 4 | print(letters[index]) # PLR1736 +5 |- blah = letters[index] # PLR1736 + 5 |+ blah = letter # PLR1736 + + diff --git a/ruff.schema.json b/ruff.schema.json index 8ae7c3fe37fb2..0e50012c9971e 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3111,6 +3111,8 @@ "PLR1714", "PLR172", "PLR1722", + "PLR173", + "PLR1736", "PLR2", "PLR20", "PLR200", From a0c3a5716ea1c3d1556d190b6a31d700dcc472d3 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 16 Oct 2023 23:19:07 -0400 Subject: [PATCH 02/19] fix assignment case --- .../pylint/unnecessary_list_index_lookup.py | 2 ++ .../rules/unnecessary_list_index_lookup.rs | 11 +++++++- ...1736_unnecessary_list_index_lookup.py.snap | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index 4ee655227c08f..e24d59f1d0756 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -3,3 +3,5 @@ for index, letter in enumerate(letters): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 + letters[index] = letters[index] # PLR1736 (on the right hand) + letters[index] = "d" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 58d32d87839f8..0fdd748d8f818 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::{self as ast, Expr, StmtFor}; +use ruff_python_ast::{self as ast, Expr, Stmt, StmtFor}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; @@ -81,6 +81,15 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { _ => visitor::walk_expr(self, expr), } } + + fn visit_stmt(&mut self, stmt: &Stmt) { + match stmt { + Stmt::Assign(ast::StmtAssign { value, .. }) => { + self.visit_expr(value); + } + _ => visitor::walk_stmt(self, stmt), + } + } } /// PLR1736 diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index c898cf48628e2..9cef9a6643bc8 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -7,6 +7,7 @@ unnecessary_list_index_lookup.py:4:11: PLR1736 [*] Unnecessary list index lookup 4 | print(letters[index]) # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 5 | blah = letters[index] # PLR1736 +6 | letters[index] = letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup @@ -17,6 +18,8 @@ unnecessary_list_index_lookup.py:4:11: PLR1736 [*] Unnecessary list index lookup 4 |- print(letters[index]) # PLR1736 4 |+ print(letter) # PLR1736 5 5 | blah = letters[index] # PLR1736 +6 6 | letters[index] = letters[index] # PLR1736 (on the right hand) +7 7 | letters[index] = "d" # Ok unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup | @@ -24,6 +27,8 @@ unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup 4 | print(letters[index]) # PLR1736 5 | blah = letters[index] # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 +6 | letters[index] = letters[index] # PLR1736 (on the right hand) +7 | letters[index] = "d" # Ok | = help: Remove unnecessary list index lookup @@ -33,5 +38,25 @@ unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup 4 4 | print(letters[index]) # PLR1736 5 |- blah = letters[index] # PLR1736 5 |+ blah = letter # PLR1736 +6 6 | letters[index] = letters[index] # PLR1736 (on the right hand) +7 7 | letters[index] = "d" # Ok + +unnecessary_list_index_lookup.py:6:22: PLR1736 [*] Unnecessary list index lookup + | +4 | print(letters[index]) # PLR1736 +5 | blah = letters[index] # PLR1736 +6 | letters[index] = letters[index] # PLR1736 (on the right hand) + | ^^^^^^^^^^^^^^ PLR1736 +7 | letters[index] = "d" # Ok + | + = help: Remove unnecessary list index lookup + +ℹ Fix +3 3 | for index, letter in enumerate(letters): +4 4 | print(letters[index]) # PLR1736 +5 5 | blah = letters[index] # PLR1736 +6 |- letters[index] = letters[index] # PLR1736 (on the right hand) + 6 |+ letters[index] = letter # PLR1736 (on the right hand) +7 7 | letters[index] = "d" # Ok From f24c92d8a04c36687093bad90cf4fa1dfab69331 Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 17 Oct 2023 18:08:13 -0400 Subject: [PATCH 03/19] ignore rule when underscore is used as a variable --- .../pylint/unnecessary_list_index_lookup.py | 24 ++++-- .../rules/unnecessary_list_index_lookup.rs | 5 ++ ...1736_unnecessary_list_index_lookup.py.snap | 82 ++++++++++--------- 3 files changed, 66 insertions(+), 45 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index e24d59f1d0756..7227014c6aec8 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -1,7 +1,21 @@ letters = ["a", "b", "c"] -for index, letter in enumerate(letters): - print(letters[index]) # PLR1736 - blah = letters[index] # PLR1736 - letters[index] = letters[index] # PLR1736 (on the right hand) - letters[index] = "d" # Ok + +def fix_these(): + for index, letter in enumerate(letters): + print(letters[index]) # PLR1736 + blah = letters[index] # PLR1736 + letters[index] = letters[index] # PLR1736 (on the right hand) + + +def dont_fix_these(): + for index, letter in enumerate(letters): + letters[index] = "d" # Ok + + +def value_intentionally_unused(): + for index, _ in enumerate(letters): + print(letters[index]) # Ok + blah = letters[index] # Ok + letters[index] = letters[index] # Ok + letters[index] = "d" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 0fdd748d8f818..360c6e3189ffc 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -128,6 +128,11 @@ pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &St return; }; + // If either of the variable names are intentionally ignored by naming them `_`, then don't emit + if index_name == "_" || value_name == "_" { + return; + } + // Get the first argument of the enumerate call let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { return; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index 9cef9a6643bc8..4c877419ec22c 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -1,62 +1,64 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -unnecessary_list_index_lookup.py:4:11: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:6:15: PLR1736 [*] Unnecessary list index lookup | -3 | for index, letter in enumerate(letters): -4 | print(letters[index]) # PLR1736 - | ^^^^^^^^^^^^^^ PLR1736 -5 | blah = letters[index] # PLR1736 -6 | letters[index] = letters[index] # PLR1736 (on the right hand) +4 | def fix_these(): +5 | for index, letter in enumerate(letters): +6 | print(letters[index]) # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +7 | blah = letters[index] # PLR1736 +8 | letters[index] = letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup ℹ Fix -1 1 | letters = ["a", "b", "c"] -2 2 | -3 3 | for index, letter in enumerate(letters): -4 |- print(letters[index]) # PLR1736 - 4 |+ print(letter) # PLR1736 -5 5 | blah = letters[index] # PLR1736 -6 6 | letters[index] = letters[index] # PLR1736 (on the right hand) -7 7 | letters[index] = "d" # Ok +3 3 | +4 4 | def fix_these(): +5 5 | for index, letter in enumerate(letters): +6 |- print(letters[index]) # PLR1736 + 6 |+ print(letter) # PLR1736 +7 7 | blah = letters[index] # PLR1736 +8 8 | letters[index] = letters[index] # PLR1736 (on the right hand) +9 9 | -unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:7:16: PLR1736 [*] Unnecessary list index lookup | -3 | for index, letter in enumerate(letters): -4 | print(letters[index]) # PLR1736 -5 | blah = letters[index] # PLR1736 - | ^^^^^^^^^^^^^^ PLR1736 -6 | letters[index] = letters[index] # PLR1736 (on the right hand) -7 | letters[index] = "d" # Ok +5 | for index, letter in enumerate(letters): +6 | print(letters[index]) # PLR1736 +7 | blah = letters[index] # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +8 | letters[index] = letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup ℹ Fix -2 2 | -3 3 | for index, letter in enumerate(letters): -4 4 | print(letters[index]) # PLR1736 -5 |- blah = letters[index] # PLR1736 - 5 |+ blah = letter # PLR1736 -6 6 | letters[index] = letters[index] # PLR1736 (on the right hand) -7 7 | letters[index] = "d" # Ok +4 4 | def fix_these(): +5 5 | for index, letter in enumerate(letters): +6 6 | print(letters[index]) # PLR1736 +7 |- blah = letters[index] # PLR1736 + 7 |+ blah = letter # PLR1736 +8 8 | letters[index] = letters[index] # PLR1736 (on the right hand) +9 9 | +10 10 | -unnecessary_list_index_lookup.py:6:22: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:8:26: PLR1736 [*] Unnecessary list index lookup | -4 | print(letters[index]) # PLR1736 -5 | blah = letters[index] # PLR1736 -6 | letters[index] = letters[index] # PLR1736 (on the right hand) - | ^^^^^^^^^^^^^^ PLR1736 -7 | letters[index] = "d" # Ok +6 | print(letters[index]) # PLR1736 +7 | blah = letters[index] # PLR1736 +8 | letters[index] = letters[index] # PLR1736 (on the right hand) + | ^^^^^^^^^^^^^^ PLR1736 | = help: Remove unnecessary list index lookup ℹ Fix -3 3 | for index, letter in enumerate(letters): -4 4 | print(letters[index]) # PLR1736 -5 5 | blah = letters[index] # PLR1736 -6 |- letters[index] = letters[index] # PLR1736 (on the right hand) - 6 |+ letters[index] = letter # PLR1736 (on the right hand) -7 7 | letters[index] = "d" # Ok +5 5 | for index, letter in enumerate(letters): +6 6 | print(letters[index]) # PLR1736 +7 7 | blah = letters[index] # PLR1736 +8 |- letters[index] = letters[index] # PLR1736 (on the right hand) + 8 |+ letters[index] = letter # PLR1736 (on the right hand) +9 9 | +10 10 | +11 11 | def dont_fix_these(): From 87ac9402523b0b715977d8fa9f37289d4256f025 Mon Sep 17 00:00:00 2001 From: Steve C Date: Wed, 18 Oct 2023 02:13:45 -0400 Subject: [PATCH 04/19] add the other assignments --- .../pylint/unnecessary_list_index_lookup.py | 4 + .../rules/unnecessary_list_index_lookup.rs | 8 ++ ...1736_unnecessary_list_index_lookup.py.snap | 82 ++++++++++++++----- 3 files changed, 74 insertions(+), 20 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index 7227014c6aec8..215e835cddd5b 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -5,6 +5,8 @@ def fix_these(): for index, letter in enumerate(letters): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 + letters[index]: str = letters[index] # PLR1736 (on the right hand) + letters[index] += letters[index] # PLR1736 (on the right hand) letters[index] = letters[index] # PLR1736 (on the right hand) @@ -17,5 +19,7 @@ def value_intentionally_unused(): for index, _ in enumerate(letters): print(letters[index]) # Ok blah = letters[index] # Ok + letters[index]: str = letters[index] # Ok + letters[index] += letters[index] # Ok letters[index] = letters[index] # Ok letters[index] = "d" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 360c6e3189ffc..87b2709d0e45e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -87,6 +87,14 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { Stmt::Assign(ast::StmtAssign { value, .. }) => { self.visit_expr(value); } + Stmt::AnnAssign(ast::StmtAnnAssign { value, .. }) => { + if let Some(value) = value { + self.visit_expr(value); + } + } + Stmt::AugAssign(ast::StmtAugAssign { value, .. }) => { + self.visit_expr(value); + } _ => visitor::walk_stmt(self, stmt), } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index 4c877419ec22c..b65f4bb825e0c 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -8,7 +8,7 @@ unnecessary_list_index_lookup.py:6:15: PLR1736 [*] Unnecessary list index lookup 6 | print(letters[index]) # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 7 | blah = letters[index] # PLR1736 -8 | letters[index] = letters[index] # PLR1736 (on the right hand) +8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup @@ -19,8 +19,8 @@ unnecessary_list_index_lookup.py:6:15: PLR1736 [*] Unnecessary list index lookup 6 |- print(letters[index]) # PLR1736 6 |+ print(letter) # PLR1736 7 7 | blah = letters[index] # PLR1736 -8 8 | letters[index] = letters[index] # PLR1736 (on the right hand) -9 9 | +8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) unnecessary_list_index_lookup.py:7:16: PLR1736 [*] Unnecessary list index lookup | @@ -28,7 +28,8 @@ unnecessary_list_index_lookup.py:7:16: PLR1736 [*] Unnecessary list index lookup 6 | print(letters[index]) # PLR1736 7 | blah = letters[index] # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -8 | letters[index] = letters[index] # PLR1736 (on the right hand) +8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +9 | letters[index] += letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup @@ -38,27 +39,68 @@ unnecessary_list_index_lookup.py:7:16: PLR1736 [*] Unnecessary list index lookup 6 6 | print(letters[index]) # PLR1736 7 |- blah = letters[index] # PLR1736 7 |+ blah = letter # PLR1736 -8 8 | letters[index] = letters[index] # PLR1736 (on the right hand) -9 9 | -10 10 | +8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) +10 10 | letters[index] = letters[index] # PLR1736 (on the right hand) -unnecessary_list_index_lookup.py:8:26: PLR1736 [*] Unnecessary list index lookup - | -6 | print(letters[index]) # PLR1736 -7 | blah = letters[index] # PLR1736 -8 | letters[index] = letters[index] # PLR1736 (on the right hand) - | ^^^^^^^^^^^^^^ PLR1736 - | - = help: Remove unnecessary list index lookup +unnecessary_list_index_lookup.py:8:31: PLR1736 [*] Unnecessary list index lookup + | + 6 | print(letters[index]) # PLR1736 + 7 | blah = letters[index] # PLR1736 + 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) + | ^^^^^^^^^^^^^^ PLR1736 + 9 | letters[index] += letters[index] # PLR1736 (on the right hand) +10 | letters[index] = letters[index] # PLR1736 (on the right hand) + | + = help: Remove unnecessary list index lookup ℹ Fix 5 5 | for index, letter in enumerate(letters): 6 6 | print(letters[index]) # PLR1736 7 7 | blah = letters[index] # PLR1736 -8 |- letters[index] = letters[index] # PLR1736 (on the right hand) - 8 |+ letters[index] = letter # PLR1736 (on the right hand) -9 9 | -10 10 | -11 11 | def dont_fix_these(): +8 |- letters[index]: str = letters[index] # PLR1736 (on the right hand) + 8 |+ letters[index]: str = letter # PLR1736 (on the right hand) +9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) +10 10 | letters[index] = letters[index] # PLR1736 (on the right hand) +11 11 | + +unnecessary_list_index_lookup.py:9:27: PLR1736 [*] Unnecessary list index lookup + | + 7 | blah = letters[index] # PLR1736 + 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) + 9 | letters[index] += letters[index] # PLR1736 (on the right hand) + | ^^^^^^^^^^^^^^ PLR1736 +10 | letters[index] = letters[index] # PLR1736 (on the right hand) + | + = help: Remove unnecessary list index lookup + +ℹ Fix +6 6 | print(letters[index]) # PLR1736 +7 7 | blah = letters[index] # PLR1736 +8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +9 |- letters[index] += letters[index] # PLR1736 (on the right hand) + 9 |+ letters[index] += letter # PLR1736 (on the right hand) +10 10 | letters[index] = letters[index] # PLR1736 (on the right hand) +11 11 | +12 12 | + +unnecessary_list_index_lookup.py:10:26: PLR1736 [*] Unnecessary list index lookup + | + 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) + 9 | letters[index] += letters[index] # PLR1736 (on the right hand) +10 | letters[index] = letters[index] # PLR1736 (on the right hand) + | ^^^^^^^^^^^^^^ PLR1736 + | + = help: Remove unnecessary list index lookup + +ℹ Fix +7 7 | blah = letters[index] # PLR1736 +8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) +10 |- letters[index] = letters[index] # PLR1736 (on the right hand) + 10 |+ letters[index] = letter # PLR1736 (on the right hand) +11 11 | +12 12 | +13 13 | def dont_fix_these(): From 26e9968c867bc8b28187c2906a620301b38bffe3 Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 19 Oct 2023 21:01:39 -0400 Subject: [PATCH 05/19] add generator/compehrension expressions --- .../pylint/unnecessary_list_index_lookup.py | 8 + .../src/checkers/ast/analyze/expression.rs | 12 ++ .../rules/unnecessary_list_index_lookup.rs | 110 +++++++--- ...1736_unnecessary_list_index_lookup.py.snap | 198 ++++++++++++------ 4 files changed, 232 insertions(+), 96 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index 215e835cddd5b..b7522f4098910 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -2,6 +2,10 @@ def fix_these(): + [letters[index] for index, letter in enumerate(letters)] # PLR1736 + {letters[index] for index, letter in enumerate(letters)} # PLR1736 + {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 + for index, letter in enumerate(letters): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 @@ -16,6 +20,10 @@ def dont_fix_these(): def value_intentionally_unused(): + [letters[index] for index, _ in enumerate(letters)] # PLR1736 + {letters[index] for index, _ in enumerate(letters)} # PLR1736 + {index: letters[index] for index, _ in enumerate(letters)} # PLR1736 + for index, _ in enumerate(letters): print(letters[index]) # Ok blah = letters[index] # Ok diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index f418fbf1212d5..b1418ee2f0981 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1317,6 +1317,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { range: _, }, ) => { + if checker.enabled(Rule::UnnecessaryListIndexLookup) { + pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::UnnecessaryComprehension) { flake8_comprehensions::rules::unnecessary_list_set_comprehension( checker, expr, elt, generators, @@ -1341,6 +1344,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { range: _, }, ) => { + if checker.enabled(Rule::UnnecessaryListIndexLookup) { + pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::UnnecessaryComprehension) { flake8_comprehensions::rules::unnecessary_list_set_comprehension( checker, expr, elt, generators, @@ -1364,6 +1370,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { generators, range: _, }) => { + if checker.enabled(Rule::UnnecessaryListIndexLookup) { + pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::UnnecessaryComprehension) { flake8_comprehensions::rules::unnecessary_dict_comprehension( checker, expr, key, value, generators, @@ -1388,6 +1397,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { range: _, }, ) => { + if checker.enabled(Rule::UnnecessaryListIndexLookup) { + pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::FunctionUsesLoopVariable) { flake8_bugbear::rules::function_uses_loop_variable(checker, &Node::Expr(expr)); } diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 87b2709d0e45e..ea88da604cff5 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -102,63 +102,119 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { /// PLR1736 pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &StmtFor) { + let Some((sequence, index_name, value_name)) = + enumerate_items(checker, &stmt_for.iter, &stmt_for.target) + else { + return; + }; + + let mut visitor = SubscriptVisitor::new(&sequence, &index_name); + + visitor.visit_body(&stmt_for.body); + visitor.visit_body(&stmt_for.orelse); + + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); + + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); + } +} + +/// PLR1736 +pub(crate) fn unnecessary_list_index_lookup_comprehension(checker: &mut Checker, expr: &Expr) { + match expr { + Expr::GeneratorExp(ast::ExprGeneratorExp { + elt, generators, .. + }) + | Expr::DictComp(ast::ExprDictComp { + value: elt, + generators, + .. + }) + | Expr::SetComp(ast::ExprSetComp { + elt, generators, .. + }) + | Expr::ListComp(ast::ExprListComp { + elt, generators, .. + }) => { + for comp in generators { + if let Some((sequence, index_name, value_name)) = + enumerate_items(checker, &comp.iter, &comp.target) + { + let mut visitor = SubscriptVisitor::new(&sequence, &index_name); + + visitor.visit_expr(elt.as_ref()); + + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); + + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); + } + } + } + } + _ => (), + } +} + +fn enumerate_items( + checker: &mut Checker, + call_expr: &Expr, + tuple_expr: &Expr, +) -> Option<(String, String, String)> { let Expr::Call(ast::ExprCall { func, arguments, .. - }) = stmt_for.iter.as_ref() + }) = call_expr else { - return; + return None; }; // Check that the function is the `enumerate` builtin. let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { - return; + return None; }; if id != "enumerate" { - return; + return None; }; if !checker.semantic().is_builtin("enumerate") { - return; + return None; }; - let Expr::Tuple(ast::ExprTuple { elts, .. }) = stmt_for.target.as_ref() else { - return; + let Expr::Tuple(ast::ExprTuple { elts, .. }) = tuple_expr else { + return None; }; let [index, value] = elts.as_slice() else { - return; + return None; }; // Grab the variable names let Expr::Name(ast::ExprName { id: index_name, .. }) = index else { - return; + return None; }; let Expr::Name(ast::ExprName { id: value_name, .. }) = value else { - return; + return None; }; // If either of the variable names are intentionally ignored by naming them `_`, then don't emit if index_name == "_" || value_name == "_" { - return; + return None; } // Get the first argument of the enumerate call let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { - return; + return None; }; - let mut visitor = SubscriptVisitor::new(sequence, index_name); - - visitor.visit_body(&stmt_for.body); - visitor.visit_body(&stmt_for.orelse); - - for range in visitor.diagnostic_ranges { - let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); - - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - value_name.clone(), - range, - ))); - - checker.diagnostics.push(diagnostic); - } + Some((sequence.clone(), index_name.clone(), value_name.clone())) } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index b65f4bb825e0c..82d7f875bc172 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -1,106 +1,166 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -unnecessary_list_index_lookup.py:6:15: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary list index lookup | 4 | def fix_these(): -5 | for index, letter in enumerate(letters): -6 | print(letters[index]) # PLR1736 - | ^^^^^^^^^^^^^^ PLR1736 -7 | blah = letters[index] # PLR1736 -8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 | = help: Remove unnecessary list index lookup ℹ Fix +2 2 | 3 3 | 4 4 | def fix_these(): -5 5 | for index, letter in enumerate(letters): -6 |- print(letters[index]) # PLR1736 - 6 |+ print(letter) # PLR1736 -7 7 | blah = letters[index] # PLR1736 -8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) - -unnecessary_list_index_lookup.py:7:16: PLR1736 [*] Unnecessary list index lookup +5 |- [letters[index] for index, letter in enumerate(letters)] # PLR1736 + 5 |+ [letter for index, letter in enumerate(letters)] # PLR1736 +6 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +8 8 | + +unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary list index lookup | -5 | for index, letter in enumerate(letters): -6 | print(letters[index]) # PLR1736 -7 | blah = letters[index] # PLR1736 - | ^^^^^^^^^^^^^^ PLR1736 -8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -9 | letters[index] += letters[index] # PLR1736 (on the right hand) +4 | def fix_these(): +5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 | = help: Remove unnecessary list index lookup ℹ Fix +3 3 | 4 4 | def fix_these(): -5 5 | for index, letter in enumerate(letters): -6 6 | print(letters[index]) # PLR1736 -7 |- blah = letters[index] # PLR1736 - 7 |+ blah = letter # PLR1736 -8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) -10 10 | letters[index] = letters[index] # PLR1736 (on the right hand) - -unnecessary_list_index_lookup.py:8:31: PLR1736 [*] Unnecessary list index lookup +5 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +6 |- {letters[index] for index, letter in enumerate(letters)} # PLR1736 + 6 |+ {letter for index, letter in enumerate(letters)} # PLR1736 +7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +8 8 | +9 9 | for index, letter in enumerate(letters): + +unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary list index lookup + | +5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +8 | +9 | for index, letter in enumerate(letters): + | + = help: Remove unnecessary list index lookup + +ℹ Fix +4 4 | def fix_these(): +5 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +6 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +7 |- {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 + 7 |+ {letter: letter for index, letter in enumerate(letters)} # PLR1736 +8 8 | +9 9 | for index, letter in enumerate(letters): +10 10 | print(letters[index]) # PLR1736 + +unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index lookup + | + 9 | for index, letter in enumerate(letters): +10 | print(letters[index]) # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +11 | blah = letters[index] # PLR1736 +12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) | - 6 | print(letters[index]) # PLR1736 - 7 | blah = letters[index] # PLR1736 - 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) + = help: Remove unnecessary list index lookup + +ℹ Fix +7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +8 8 | +9 9 | for index, letter in enumerate(letters): +10 |- print(letters[index]) # PLR1736 + 10 |+ print(letter) # PLR1736 +11 11 | blah = letters[index] # PLR1736 +12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) + +unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index lookup + | + 9 | for index, letter in enumerate(letters): +10 | print(letters[index]) # PLR1736 +11 | blah = letters[index] # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 | letters[index] += letters[index] # PLR1736 (on the right hand) + | + = help: Remove unnecessary list index lookup + +ℹ Fix +8 8 | +9 9 | for index, letter in enumerate(letters): +10 10 | print(letters[index]) # PLR1736 +11 |- blah = letters[index] # PLR1736 + 11 |+ blah = letter # PLR1736 +12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) +14 14 | letters[index] = letters[index] # PLR1736 (on the right hand) + +unnecessary_list_index_lookup.py:12:31: PLR1736 [*] Unnecessary list index lookup + | +10 | print(letters[index]) # PLR1736 +11 | blah = letters[index] # PLR1736 +12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) | ^^^^^^^^^^^^^^ PLR1736 - 9 | letters[index] += letters[index] # PLR1736 (on the right hand) -10 | letters[index] = letters[index] # PLR1736 (on the right hand) +13 | letters[index] += letters[index] # PLR1736 (on the right hand) +14 | letters[index] = letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup ℹ Fix -5 5 | for index, letter in enumerate(letters): -6 6 | print(letters[index]) # PLR1736 -7 7 | blah = letters[index] # PLR1736 -8 |- letters[index]: str = letters[index] # PLR1736 (on the right hand) - 8 |+ letters[index]: str = letter # PLR1736 (on the right hand) -9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) -10 10 | letters[index] = letters[index] # PLR1736 (on the right hand) -11 11 | - -unnecessary_list_index_lookup.py:9:27: PLR1736 [*] Unnecessary list index lookup +9 9 | for index, letter in enumerate(letters): +10 10 | print(letters[index]) # PLR1736 +11 11 | blah = letters[index] # PLR1736 +12 |- letters[index]: str = letters[index] # PLR1736 (on the right hand) + 12 |+ letters[index]: str = letter # PLR1736 (on the right hand) +13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) +14 14 | letters[index] = letters[index] # PLR1736 (on the right hand) +15 15 | + +unnecessary_list_index_lookup.py:13:27: PLR1736 [*] Unnecessary list index lookup | - 7 | blah = letters[index] # PLR1736 - 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) - 9 | letters[index] += letters[index] # PLR1736 (on the right hand) +11 | blah = letters[index] # PLR1736 +12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 | letters[index] += letters[index] # PLR1736 (on the right hand) | ^^^^^^^^^^^^^^ PLR1736 -10 | letters[index] = letters[index] # PLR1736 (on the right hand) +14 | letters[index] = letters[index] # PLR1736 (on the right hand) | = help: Remove unnecessary list index lookup ℹ Fix -6 6 | print(letters[index]) # PLR1736 -7 7 | blah = letters[index] # PLR1736 -8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -9 |- letters[index] += letters[index] # PLR1736 (on the right hand) - 9 |+ letters[index] += letter # PLR1736 (on the right hand) -10 10 | letters[index] = letters[index] # PLR1736 (on the right hand) -11 11 | -12 12 | - -unnecessary_list_index_lookup.py:10:26: PLR1736 [*] Unnecessary list index lookup +10 10 | print(letters[index]) # PLR1736 +11 11 | blah = letters[index] # PLR1736 +12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 |- letters[index] += letters[index] # PLR1736 (on the right hand) + 13 |+ letters[index] += letter # PLR1736 (on the right hand) +14 14 | letters[index] = letters[index] # PLR1736 (on the right hand) +15 15 | +16 16 | + +unnecessary_list_index_lookup.py:14:26: PLR1736 [*] Unnecessary list index lookup | - 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) - 9 | letters[index] += letters[index] # PLR1736 (on the right hand) -10 | letters[index] = letters[index] # PLR1736 (on the right hand) +12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 | letters[index] += letters[index] # PLR1736 (on the right hand) +14 | letters[index] = letters[index] # PLR1736 (on the right hand) | ^^^^^^^^^^^^^^ PLR1736 | = help: Remove unnecessary list index lookup ℹ Fix -7 7 | blah = letters[index] # PLR1736 -8 8 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -9 9 | letters[index] += letters[index] # PLR1736 (on the right hand) -10 |- letters[index] = letters[index] # PLR1736 (on the right hand) - 10 |+ letters[index] = letter # PLR1736 (on the right hand) -11 11 | -12 12 | -13 13 | def dont_fix_these(): +11 11 | blah = letters[index] # PLR1736 +12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) +14 |- letters[index] = letters[index] # PLR1736 (on the right hand) + 14 |+ letters[index] = letter # PLR1736 (on the right hand) +15 15 | +16 16 | +17 17 | def dont_fix_these(): From 6a041fac0cf22848e6a32fb241e2acaeb8e019cc Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 4 Nov 2023 14:18:15 -0400 Subject: [PATCH 06/19] tweak logic for assignments --- .../pylint/unnecessary_list_index_lookup.py | 9 +-- .../rules/unnecessary_list_index_lookup.rs | 40 ++++++++++- ...1736_unnecessary_list_index_lookup.py.snap | 72 ++++--------------- 3 files changed, 55 insertions(+), 66 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index b7522f4098910..a7ae2ecc749c0 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -9,14 +9,14 @@ def fix_these(): for index, letter in enumerate(letters): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 - letters[index]: str = letters[index] # PLR1736 (on the right hand) - letters[index] += letters[index] # PLR1736 (on the right hand) - letters[index] = letters[index] # PLR1736 (on the right hand) + assert letters[index] == "d" # PLR1736 def dont_fix_these(): + # once there is an assignment to the sequence[index], we stop emitting diagnostics for index, letter in enumerate(letters): letters[index] = "d" # Ok + assert letters[index] == "d" # Ok def value_intentionally_unused(): @@ -27,7 +27,4 @@ def value_intentionally_unused(): for index, _ in enumerate(letters): print(letters[index]) # Ok blah = letters[index] # Ok - letters[index]: str = letters[index] # Ok - letters[index] += letters[index] # Ok - letters[index] = letters[index] # Ok letters[index] = "d" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index ea88da604cff5..6fb27617ff9b4 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -47,6 +47,7 @@ struct SubscriptVisitor<'a> { sequence_name: &'a str, index_name: &'a str, diagnostic_ranges: Vec, + is_subcript_modified: bool, } impl<'a> SubscriptVisitor<'a> { @@ -55,12 +56,35 @@ impl<'a> SubscriptVisitor<'a> { sequence_name, index_name, diagnostic_ranges: Vec::new(), + is_subcript_modified: false, } } } +fn check_target_for_assignment(expr: &Expr, sequence_name: &str, index_name: &str) -> bool { + // if we see the sequence subscript being modified, we'll stop emitting diagnostics + match expr { + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { + if id == sequence_name { + if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { + if id == index_name { + return true; + } + } + } + } + false + } + _ => false, + } +} + impl<'a> Visitor<'_> for SubscriptVisitor<'a> { fn visit_expr(&mut self, expr: &Expr) { + if self.is_subcript_modified { + return; + } match expr { Expr::Subscript(ast::ExprSubscript { value, @@ -83,16 +107,26 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { } fn visit_stmt(&mut self, stmt: &Stmt) { + if self.is_subcript_modified { + return; + } match stmt { - Stmt::Assign(ast::StmtAssign { value, .. }) => { + Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { + self.is_subcript_modified = targets.iter().any(|target| { + check_target_for_assignment(target, self.sequence_name, self.index_name) + }); self.visit_expr(value); } - Stmt::AnnAssign(ast::StmtAnnAssign { value, .. }) => { + Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { if let Some(value) = value { + self.is_subcript_modified = + check_target_for_assignment(target, self.sequence_name, self.index_name); self.visit_expr(value); } } - Stmt::AugAssign(ast::StmtAugAssign { value, .. }) => { + Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { + self.is_subcript_modified = + check_target_for_assignment(target, self.sequence_name, self.index_name); self.visit_expr(value); } _ => visitor::walk_stmt(self, stmt), diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index 82d7f875bc172..a6a6b971cfd10 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -68,7 +68,7 @@ unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index looku 10 | print(letters[index]) # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 11 | blah = letters[index] # PLR1736 -12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) +12 | assert letters[index] == "d" # PLR1736 | = help: Remove unnecessary list index lookup @@ -79,8 +79,8 @@ unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index looku 10 |- print(letters[index]) # PLR1736 10 |+ print(letter) # PLR1736 11 11 | blah = letters[index] # PLR1736 -12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) +12 12 | assert letters[index] == "d" # PLR1736 +13 13 | unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index lookup | @@ -88,8 +88,7 @@ unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index looku 10 | print(letters[index]) # PLR1736 11 | blah = letters[index] # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 | letters[index] += letters[index] # PLR1736 (on the right hand) +12 | assert letters[index] == "d" # PLR1736 | = help: Remove unnecessary list index lookup @@ -99,18 +98,16 @@ unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index looku 10 10 | print(letters[index]) # PLR1736 11 |- blah = letters[index] # PLR1736 11 |+ blah = letter # PLR1736 -12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) -14 14 | letters[index] = letters[index] # PLR1736 (on the right hand) +12 12 | assert letters[index] == "d" # PLR1736 +13 13 | +14 14 | -unnecessary_list_index_lookup.py:12:31: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:12:16: PLR1736 [*] Unnecessary list index lookup | 10 | print(letters[index]) # PLR1736 11 | blah = letters[index] # PLR1736 -12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) - | ^^^^^^^^^^^^^^ PLR1736 -13 | letters[index] += letters[index] # PLR1736 (on the right hand) -14 | letters[index] = letters[index] # PLR1736 (on the right hand) +12 | assert letters[index] == "d" # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 | = help: Remove unnecessary list index lookup @@ -118,49 +115,10 @@ unnecessary_list_index_lookup.py:12:31: PLR1736 [*] Unnecessary list index looku 9 9 | for index, letter in enumerate(letters): 10 10 | print(letters[index]) # PLR1736 11 11 | blah = letters[index] # PLR1736 -12 |- letters[index]: str = letters[index] # PLR1736 (on the right hand) - 12 |+ letters[index]: str = letter # PLR1736 (on the right hand) -13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) -14 14 | letters[index] = letters[index] # PLR1736 (on the right hand) -15 15 | - -unnecessary_list_index_lookup.py:13:27: PLR1736 [*] Unnecessary list index lookup - | -11 | blah = letters[index] # PLR1736 -12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 | letters[index] += letters[index] # PLR1736 (on the right hand) - | ^^^^^^^^^^^^^^ PLR1736 -14 | letters[index] = letters[index] # PLR1736 (on the right hand) - | - = help: Remove unnecessary list index lookup - -ℹ Fix -10 10 | print(letters[index]) # PLR1736 -11 11 | blah = letters[index] # PLR1736 -12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 |- letters[index] += letters[index] # PLR1736 (on the right hand) - 13 |+ letters[index] += letter # PLR1736 (on the right hand) -14 14 | letters[index] = letters[index] # PLR1736 (on the right hand) -15 15 | -16 16 | - -unnecessary_list_index_lookup.py:14:26: PLR1736 [*] Unnecessary list index lookup - | -12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 | letters[index] += letters[index] # PLR1736 (on the right hand) -14 | letters[index] = letters[index] # PLR1736 (on the right hand) - | ^^^^^^^^^^^^^^ PLR1736 - | - = help: Remove unnecessary list index lookup - -ℹ Fix -11 11 | blah = letters[index] # PLR1736 -12 12 | letters[index]: str = letters[index] # PLR1736 (on the right hand) -13 13 | letters[index] += letters[index] # PLR1736 (on the right hand) -14 |- letters[index] = letters[index] # PLR1736 (on the right hand) - 14 |+ letters[index] = letter # PLR1736 (on the right hand) -15 15 | -16 16 | -17 17 | def dont_fix_these(): +12 |- assert letters[index] == "d" # PLR1736 + 12 |+ assert letter == "d" # PLR1736 +13 13 | +14 14 | +15 15 | def dont_fix_these(): From 85f4c44b82e276f8acf40d78107358dd11152df1 Mon Sep 17 00:00:00 2001 From: Steve C Date: Fri, 24 Nov 2023 15:14:10 -0500 Subject: [PATCH 07/19] fix lint code order --- crates/ruff_linter/src/codes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 76f5e78c143cc..1e19f706a889f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -257,8 +257,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary), - (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), + (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison), (Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf), (Pylint, "R6201") => (RuleGroup::Preview, rules::pylint::rules::LiteralMembership), From 801aca57932047c68decf8efad86bac8ba1854fe Mon Sep 17 00:00:00 2001 From: Steve C Date: Fri, 24 Nov 2023 15:28:54 -0500 Subject: [PATCH 08/19] update snapshots --- ...ts__PLR1736_unnecessary_list_index_lookup.py.snap | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index a6a6b971cfd10..6c7f2fd39ca1b 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -11,7 +11,7 @@ unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary list index lookup | = help: Remove unnecessary list index lookup -ℹ Fix +ℹ Safe fix 2 2 | 3 3 | 4 4 | def fix_these(): @@ -31,7 +31,7 @@ unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary list index lookup | = help: Remove unnecessary list index lookup -ℹ Fix +ℹ Safe fix 3 3 | 4 4 | def fix_these(): 5 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 @@ -52,7 +52,7 @@ unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary list index lookup | = help: Remove unnecessary list index lookup -ℹ Fix +ℹ Safe fix 4 4 | def fix_these(): 5 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 6 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 @@ -72,7 +72,7 @@ unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index looku | = help: Remove unnecessary list index lookup -ℹ Fix +ℹ Safe fix 7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 8 8 | 9 9 | for index, letter in enumerate(letters): @@ -92,7 +92,7 @@ unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index looku | = help: Remove unnecessary list index lookup -ℹ Fix +ℹ Safe fix 8 8 | 9 9 | for index, letter in enumerate(letters): 10 10 | print(letters[index]) # PLR1736 @@ -111,7 +111,7 @@ unnecessary_list_index_lookup.py:12:16: PLR1736 [*] Unnecessary list index looku | = help: Remove unnecessary list index lookup -ℹ Fix +ℹ Safe fix 9 9 | for index, letter in enumerate(letters): 10 10 | print(letters[index]) # PLR1736 11 11 | blah = letters[index] # PLR1736 From 1aa00249852499711c1de88c60fe9cf8f870a53f Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 27 Nov 2023 23:41:43 -0500 Subject: [PATCH 09/19] Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs Co-authored-by: Zanie Blue --- .../src/rules/pylint/rules/unnecessary_list_index_lookup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 6fb27617ff9b4..762784e064633 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -35,7 +35,7 @@ pub struct UnnecessaryListIndexLookup; impl AlwaysFixableViolation for UnnecessaryListIndexLookup { #[derive_message_formats] fn message(&self) -> String { - format!("Unnecessary list index lookup") + format!("Unnecessary lookup of list item by index") } fn fix_title(&self) -> String { From 0dc64ebaa29acd73f6f8447ff83f3015be3d9040 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 27 Nov 2023 23:41:50 -0500 Subject: [PATCH 10/19] Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs Co-authored-by: Zanie Blue --- .../src/rules/pylint/rules/unnecessary_list_index_lookup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 762784e064633..6d8e9e08c6373 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -12,7 +12,7 @@ use crate::checkers::ast::Checker; /// Checks for uses of enumeration and accessing the value by index lookup. /// /// ## Why is this bad? -/// The value is already accessible by the 2nd variable from the enumeration. +/// It is more succinct to use the variable for the value at the current index which is already in scope from the iterator. /// /// ## Example /// ```python From 6f7f90d047356ceb426e8ff8772ddf269381b46d Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 27 Nov 2023 23:41:56 -0500 Subject: [PATCH 11/19] Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs Co-authored-by: Zanie Blue --- .../src/rules/pylint/rules/unnecessary_list_index_lookup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 6d8e9e08c6373..9bee1ffaab9c5 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -39,7 +39,7 @@ impl AlwaysFixableViolation for UnnecessaryListIndexLookup { } fn fix_title(&self) -> String { - format!("Remove unnecessary list index lookup") + format!("Use existing item variable instead") } } From 33bdba85cc18aca266f5fdc64fe32f5139195788 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 27 Nov 2023 23:42:04 -0500 Subject: [PATCH 12/19] Update crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs Co-authored-by: Zanie Blue --- .../src/rules/pylint/rules/unnecessary_list_index_lookup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 9bee1ffaab9c5..d8d463402ba2c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -9,7 +9,7 @@ use ruff_text_size::TextRange; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for uses of enumeration and accessing the value by index lookup. +/// Checks for access of a list item at the current index when using enumeration. /// /// ## Why is this bad? /// It is more succinct to use the variable for the value at the current index which is already in scope from the iterator. From 502f54395f0fcdd811a5bb655b3cca47f7f576cc Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 27 Nov 2023 23:56:19 -0500 Subject: [PATCH 13/19] Update ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap --- ...1736_unnecessary_list_index_lookup.py.snap | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index 6c7f2fd39ca1b..772c4113b3afd 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary lookup of list item by index | 4 | def fix_these(): 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 @@ -9,7 +9,7 @@ unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary list index lookup 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 | - = help: Remove unnecessary list index lookup + = help: Use existing item variable instead ℹ Safe fix 2 2 | @@ -21,7 +21,7 @@ unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary list index lookup 7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 8 8 | -unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary lookup of list item by index | 4 | def fix_these(): 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 @@ -29,7 +29,7 @@ unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary list index lookup | ^^^^^^^^^^^^^^ PLR1736 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 | - = help: Remove unnecessary list index lookup + = help: Use existing item variable instead ℹ Safe fix 3 3 | @@ -41,7 +41,7 @@ unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary list index lookup 8 8 | 9 9 | for index, letter in enumerate(letters): -unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary lookup of list item by index | 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 @@ -50,7 +50,7 @@ unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary list index lookup 8 | 9 | for index, letter in enumerate(letters): | - = help: Remove unnecessary list index lookup + = help: Use existing item variable instead ℹ Safe fix 4 4 | def fix_these(): @@ -62,7 +62,7 @@ unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary list index lookup 9 9 | for index, letter in enumerate(letters): 10 10 | print(letters[index]) # PLR1736 -unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary lookup of list item by index | 9 | for index, letter in enumerate(letters): 10 | print(letters[index]) # PLR1736 @@ -70,7 +70,7 @@ unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index looku 11 | blah = letters[index] # PLR1736 12 | assert letters[index] == "d" # PLR1736 | - = help: Remove unnecessary list index lookup + = help: Use existing item variable instead ℹ Safe fix 7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 @@ -82,7 +82,7 @@ unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary list index looku 12 12 | assert letters[index] == "d" # PLR1736 13 13 | -unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary lookup of list item by index | 9 | for index, letter in enumerate(letters): 10 | print(letters[index]) # PLR1736 @@ -90,7 +90,7 @@ unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index looku | ^^^^^^^^^^^^^^ PLR1736 12 | assert letters[index] == "d" # PLR1736 | - = help: Remove unnecessary list index lookup + = help: Use existing item variable instead ℹ Safe fix 8 8 | @@ -102,14 +102,14 @@ unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary list index looku 13 13 | 14 14 | -unnecessary_list_index_lookup.py:12:16: PLR1736 [*] Unnecessary list index lookup +unnecessary_list_index_lookup.py:12:16: PLR1736 [*] Unnecessary lookup of list item by index | 10 | print(letters[index]) # PLR1736 11 | blah = letters[index] # PLR1736 12 | assert letters[index] == "d" # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 | - = help: Remove unnecessary list index lookup + = help: Use existing item variable instead ℹ Safe fix 9 9 | for index, letter in enumerate(letters): From d0e2f19b84980c25d77c91f7c59bfae141d2b2eb Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 00:08:40 -0500 Subject: [PATCH 14/19] tweak modification detection --- .../rules/unnecessary_list_index_lookup.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index d8d463402ba2c..e585706d9b22b 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -12,7 +12,7 @@ use crate::checkers::ast::Checker; /// Checks for access of a list item at the current index when using enumeration. /// /// ## Why is this bad? -/// It is more succinct to use the variable for the value at the current index which is already in scope from the iterator. +/// It is more succinct to use the variable for the value at the current index which is already in scope from the iterator. /// /// ## Example /// ```python @@ -47,7 +47,7 @@ struct SubscriptVisitor<'a> { sequence_name: &'a str, index_name: &'a str, diagnostic_ranges: Vec, - is_subcript_modified: bool, + modified: bool, } impl<'a> SubscriptVisitor<'a> { @@ -56,14 +56,15 @@ impl<'a> SubscriptVisitor<'a> { sequence_name, index_name, diagnostic_ranges: Vec::new(), - is_subcript_modified: false, + modified: false, } } } fn check_target_for_assignment(expr: &Expr, sequence_name: &str, index_name: &str) -> bool { - // if we see the sequence subscript being modified, we'll stop emitting diagnostics + // if we see the sequence, a subscript, or the index being modified, we'll stop emitting diagnostics match expr { + Expr::Name(ast::ExprName { id, .. }) => id == sequence_name || id == index_name, Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { if id == sequence_name { @@ -82,7 +83,7 @@ fn check_target_for_assignment(expr: &Expr, sequence_name: &str, index_name: &st impl<'a> Visitor<'_> for SubscriptVisitor<'a> { fn visit_expr(&mut self, expr: &Expr) { - if self.is_subcript_modified { + if self.modified { return; } match expr { @@ -107,25 +108,25 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { } fn visit_stmt(&mut self, stmt: &Stmt) { - if self.is_subcript_modified { + if self.modified { return; } match stmt { Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { - self.is_subcript_modified = targets.iter().any(|target| { + self.modified = targets.iter().any(|target| { check_target_for_assignment(target, self.sequence_name, self.index_name) }); self.visit_expr(value); } Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { if let Some(value) = value { - self.is_subcript_modified = + self.modified = check_target_for_assignment(target, self.sequence_name, self.index_name); self.visit_expr(value); } } Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { - self.is_subcript_modified = + self.modified = check_target_for_assignment(target, self.sequence_name, self.index_name); self.visit_expr(value); } From fc43d1b6ea3de26398afe0c7f4a105d9b8230de0 Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 00:10:23 -0500 Subject: [PATCH 15/19] update fixture --- .../pylint/unnecessary_list_index_lookup.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index a7ae2ecc749c0..2e15d9bdb88f0 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -16,7 +16,18 @@ def dont_fix_these(): # once there is an assignment to the sequence[index], we stop emitting diagnostics for index, letter in enumerate(letters): letters[index] = "d" # Ok - assert letters[index] == "d" # Ok + letters[index] += "e" # Ok + assert letters[index] == "de" # Ok + + # once there is an assignment to the index, we stop emitting diagnostics + for index, letter in enumerate(letters): + index += 1 # Ok + print(letters[index]) # Ok + + # once there is an assignment to the sequence, we stop emitting diagnostics + for index, letter in enumerate(letters): + letters = ["d", "e", "f"] # Ok + print(letters[index]) # Ok def value_intentionally_unused(): From bcae782ad53269678f834d9747fe6bb70371d9d8 Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 00:41:22 -0500 Subject: [PATCH 16/19] use `checker.semantic().resolve_call_path` --- .../pylint/unnecessary_list_index_lookup.py | 7 + .../rules/unnecessary_list_index_lookup.rs | 14 +- ...1736_unnecessary_list_index_lookup.py.snap | 235 +++++++++++------- 3 files changed, 162 insertions(+), 94 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index 2e15d9bdb88f0..3e00c7fd86180 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -1,3 +1,5 @@ +import builtins + letters = ["a", "b", "c"] @@ -10,6 +12,11 @@ def fix_these(): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 assert letters[index] == "d" # PLR1736 + + for index, letter in builtins.enumerate(letters): + print(letters[index]) # PLR1736 + blah = letters[index] # PLR1736 + assert letters[index] == "d" # PLR1736 def dont_fix_these(): diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index e585706d9b22b..dbc626f5f1ec4 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -215,16 +215,16 @@ fn enumerate_items( }; // Check that the function is the `enumerate` builtin. - let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { - return None; - }; - if id != "enumerate" { - return None; - }; - if !checker.semantic().is_builtin("enumerate") { + let Some(call_path) = checker.semantic().resolve_call_path(func.as_ref()) else { return None; }; + match call_path.as_slice() { + ["", "enumerate"] => (), + ["builtins", "enumerate"] => (), + _ => return None, + } + let Expr::Tuple(ast::ExprTuple { elts, .. }) = tuple_expr else { return None; }; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap index 772c4113b3afd..aab105471a4c7 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -1,124 +1,185 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -unnecessary_list_index_lookup.py:5:6: PLR1736 [*] Unnecessary lookup of list item by index +unnecessary_list_index_lookup.py:7:6: PLR1736 [*] Unnecessary lookup of list item by index | -4 | def fix_these(): -5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +6 | def fix_these(): +7 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 -7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +8 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +9 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 | = help: Use existing item variable instead ℹ Safe fix -2 2 | -3 3 | -4 4 | def fix_these(): -5 |- [letters[index] for index, letter in enumerate(letters)] # PLR1736 - 5 |+ [letter for index, letter in enumerate(letters)] # PLR1736 -6 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 -7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 -8 8 | - -unnecessary_list_index_lookup.py:6:6: PLR1736 [*] Unnecessary lookup of list item by index +4 4 | +5 5 | +6 6 | def fix_these(): +7 |- [letters[index] for index, letter in enumerate(letters)] # PLR1736 + 7 |+ [letter for index, letter in enumerate(letters)] # PLR1736 +8 8 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +9 9 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +10 10 | + +unnecessary_list_index_lookup.py:8:6: PLR1736 [*] Unnecessary lookup of list item by index | -4 | def fix_these(): -5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 -6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +6 | def fix_these(): +7 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +8 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +9 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 | = help: Use existing item variable instead ℹ Safe fix -3 3 | -4 4 | def fix_these(): -5 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 -6 |- {letters[index] for index, letter in enumerate(letters)} # PLR1736 - 6 |+ {letter for index, letter in enumerate(letters)} # PLR1736 -7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 -8 8 | -9 9 | for index, letter in enumerate(letters): - -unnecessary_list_index_lookup.py:7:14: PLR1736 [*] Unnecessary lookup of list item by index - | -5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 -6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 -7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 - | ^^^^^^^^^^^^^^ PLR1736 -8 | -9 | for index, letter in enumerate(letters): - | - = help: Use existing item variable instead +5 5 | +6 6 | def fix_these(): +7 7 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +8 |- {letters[index] for index, letter in enumerate(letters)} # PLR1736 + 8 |+ {letter for index, letter in enumerate(letters)} # PLR1736 +9 9 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +10 10 | +11 11 | for index, letter in enumerate(letters): + +unnecessary_list_index_lookup.py:9:14: PLR1736 [*] Unnecessary lookup of list item by index + | + 7 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 + 8 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 + 9 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +10 | +11 | for index, letter in enumerate(letters): + | + = help: Use existing item variable instead + +ℹ Safe fix +6 6 | def fix_these(): +7 7 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 +8 8 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 +9 |- {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 + 9 |+ {letter: letter for index, letter in enumerate(letters)} # PLR1736 +10 10 | +11 11 | for index, letter in enumerate(letters): +12 12 | print(letters[index]) # PLR1736 + +unnecessary_list_index_lookup.py:12:15: PLR1736 [*] Unnecessary lookup of list item by index + | +11 | for index, letter in enumerate(letters): +12 | print(letters[index]) # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +13 | blah = letters[index] # PLR1736 +14 | assert letters[index] == "d" # PLR1736 + | + = help: Use existing item variable instead + +ℹ Safe fix +9 9 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 +10 10 | +11 11 | for index, letter in enumerate(letters): +12 |- print(letters[index]) # PLR1736 + 12 |+ print(letter) # PLR1736 +13 13 | blah = letters[index] # PLR1736 +14 14 | assert letters[index] == "d" # PLR1736 +15 15 | + +unnecessary_list_index_lookup.py:13:16: PLR1736 [*] Unnecessary lookup of list item by index + | +11 | for index, letter in enumerate(letters): +12 | print(letters[index]) # PLR1736 +13 | blah = letters[index] # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +14 | assert letters[index] == "d" # PLR1736 + | + = help: Use existing item variable instead + +ℹ Safe fix +10 10 | +11 11 | for index, letter in enumerate(letters): +12 12 | print(letters[index]) # PLR1736 +13 |- blah = letters[index] # PLR1736 + 13 |+ blah = letter # PLR1736 +14 14 | assert letters[index] == "d" # PLR1736 +15 15 | +16 16 | for index, letter in builtins.enumerate(letters): + +unnecessary_list_index_lookup.py:14:16: PLR1736 [*] Unnecessary lookup of list item by index + | +12 | print(letters[index]) # PLR1736 +13 | blah = letters[index] # PLR1736 +14 | assert letters[index] == "d" # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +15 | +16 | for index, letter in builtins.enumerate(letters): + | + = help: Use existing item variable instead ℹ Safe fix -4 4 | def fix_these(): -5 5 | [letters[index] for index, letter in enumerate(letters)] # PLR1736 -6 6 | {letters[index] for index, letter in enumerate(letters)} # PLR1736 -7 |- {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 - 7 |+ {letter: letter for index, letter in enumerate(letters)} # PLR1736 -8 8 | -9 9 | for index, letter in enumerate(letters): -10 10 | print(letters[index]) # PLR1736 - -unnecessary_list_index_lookup.py:10:15: PLR1736 [*] Unnecessary lookup of list item by index +11 11 | for index, letter in enumerate(letters): +12 12 | print(letters[index]) # PLR1736 +13 13 | blah = letters[index] # PLR1736 +14 |- assert letters[index] == "d" # PLR1736 + 14 |+ assert letter == "d" # PLR1736 +15 15 | +16 16 | for index, letter in builtins.enumerate(letters): +17 17 | print(letters[index]) # PLR1736 + +unnecessary_list_index_lookup.py:17:15: PLR1736 [*] Unnecessary lookup of list item by index | - 9 | for index, letter in enumerate(letters): -10 | print(letters[index]) # PLR1736 +16 | for index, letter in builtins.enumerate(letters): +17 | print(letters[index]) # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -11 | blah = letters[index] # PLR1736 -12 | assert letters[index] == "d" # PLR1736 +18 | blah = letters[index] # PLR1736 +19 | assert letters[index] == "d" # PLR1736 | = help: Use existing item variable instead ℹ Safe fix -7 7 | {letter: letters[index] for index, letter in enumerate(letters)} # PLR1736 -8 8 | -9 9 | for index, letter in enumerate(letters): -10 |- print(letters[index]) # PLR1736 - 10 |+ print(letter) # PLR1736 -11 11 | blah = letters[index] # PLR1736 -12 12 | assert letters[index] == "d" # PLR1736 -13 13 | - -unnecessary_list_index_lookup.py:11:16: PLR1736 [*] Unnecessary lookup of list item by index +14 14 | assert letters[index] == "d" # PLR1736 +15 15 | +16 16 | for index, letter in builtins.enumerate(letters): +17 |- print(letters[index]) # PLR1736 + 17 |+ print(letter) # PLR1736 +18 18 | blah = letters[index] # PLR1736 +19 19 | assert letters[index] == "d" # PLR1736 +20 20 | + +unnecessary_list_index_lookup.py:18:16: PLR1736 [*] Unnecessary lookup of list item by index | - 9 | for index, letter in enumerate(letters): -10 | print(letters[index]) # PLR1736 -11 | blah = letters[index] # PLR1736 +16 | for index, letter in builtins.enumerate(letters): +17 | print(letters[index]) # PLR1736 +18 | blah = letters[index] # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -12 | assert letters[index] == "d" # PLR1736 +19 | assert letters[index] == "d" # PLR1736 | = help: Use existing item variable instead ℹ Safe fix -8 8 | -9 9 | for index, letter in enumerate(letters): -10 10 | print(letters[index]) # PLR1736 -11 |- blah = letters[index] # PLR1736 - 11 |+ blah = letter # PLR1736 -12 12 | assert letters[index] == "d" # PLR1736 -13 13 | -14 14 | - -unnecessary_list_index_lookup.py:12:16: PLR1736 [*] Unnecessary lookup of list item by index +15 15 | +16 16 | for index, letter in builtins.enumerate(letters): +17 17 | print(letters[index]) # PLR1736 +18 |- blah = letters[index] # PLR1736 + 18 |+ blah = letter # PLR1736 +19 19 | assert letters[index] == "d" # PLR1736 +20 20 | +21 21 | + +unnecessary_list_index_lookup.py:19:16: PLR1736 [*] Unnecessary lookup of list item by index | -10 | print(letters[index]) # PLR1736 -11 | blah = letters[index] # PLR1736 -12 | assert letters[index] == "d" # PLR1736 +17 | print(letters[index]) # PLR1736 +18 | blah = letters[index] # PLR1736 +19 | assert letters[index] == "d" # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 | = help: Use existing item variable instead ℹ Safe fix -9 9 | for index, letter in enumerate(letters): -10 10 | print(letters[index]) # PLR1736 -11 11 | blah = letters[index] # PLR1736 -12 |- assert letters[index] == "d" # PLR1736 - 12 |+ assert letter == "d" # PLR1736 -13 13 | -14 14 | -15 15 | def dont_fix_these(): +16 16 | for index, letter in builtins.enumerate(letters): +17 17 | print(letters[index]) # PLR1736 +18 18 | blah = letters[index] # PLR1736 +19 |- assert letters[index] == "d" # PLR1736 + 19 |+ assert letter == "d" # PLR1736 +20 20 | +21 21 | +22 22 | def dont_fix_these(): From 267a10b1fdecc7a0e1f6ecb91dc710d0735f2260 Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 02:17:17 -0500 Subject: [PATCH 17/19] add case for deletions --- .../pylint/unnecessary_list_index_lookup.py | 11 ++++++++++ .../rules/unnecessary_list_index_lookup.rs | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index 3e00c7fd86180..522be8931134b 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -36,6 +36,17 @@ def dont_fix_these(): letters = ["d", "e", "f"] # Ok print(letters[index]) # Ok + # once there is an deletion from or of the sequence or index, we stop emitting diagnostics + for index, letter in enumerate(letters): + del letters[index] # Ok + print(letters[index]) # Ok + for index, letter in enumerate(letters): + del letters # Ok + print(letters[index]) # Ok + for index, letter in enumerate(letters): + del index # Ok + print(letters[index]) # Ok + def value_intentionally_unused(): [letters[index] for index, _ in enumerate(letters)] # PLR1736 diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index dbc626f5f1ec4..e0e2096401130 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -130,6 +130,26 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { check_target_for_assignment(target, self.sequence_name, self.index_name); self.visit_expr(value); } + Stmt::Delete(ast::StmtDelete { targets, .. }) => { + self.modified = targets.iter().any(|target| match target { + Expr::Name(ast::ExprName { id, .. }) => { + id == self.sequence_name || id == self.index_name + } + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { + if id == self.sequence_name { + if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { + if id == self.index_name { + return true; + } + } + } + } + false + } + _ => false, + }); + } _ => visitor::walk_stmt(self, stmt), } } From a6502b48ea7cbb5ebd3500f3d048ce062e14db28 Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 02:47:35 -0500 Subject: [PATCH 18/19] fix comments --- .../test/fixtures/pylint/unnecessary_list_index_lookup.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py index 522be8931134b..182e63cb7b913 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -49,9 +49,9 @@ def dont_fix_these(): def value_intentionally_unused(): - [letters[index] for index, _ in enumerate(letters)] # PLR1736 - {letters[index] for index, _ in enumerate(letters)} # PLR1736 - {index: letters[index] for index, _ in enumerate(letters)} # PLR1736 + [letters[index] for index, _ in enumerate(letters)] # Ok + {letters[index] for index, _ in enumerate(letters)} # Ok + {index: letters[index] for index, _ in enumerate(letters)} # Ok for index, _ in enumerate(letters): print(letters[index]) # Ok From 59fec72500cd12db0cbb567f21fd254622ee9459 Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 30 Nov 2023 18:29:13 -0500 Subject: [PATCH 19/19] suggested tweaks --- .../rules/unnecessary_list_index_lookup.rs | 81 ++++++++++--------- 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index e0e2096401130..a668812ff0fd1 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -66,13 +66,15 @@ fn check_target_for_assignment(expr: &Expr, sequence_name: &str, index_name: &st match expr { Expr::Name(ast::ExprName { id, .. }) => id == sequence_name || id == index_name, Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { - if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { - if id == sequence_name { - if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { - if id == index_name { - return true; - } - } + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return false; + }; + if id == sequence_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return false; + }; + if id == index_name { + return true; } } false @@ -93,13 +95,15 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { range, .. }) => { - if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { - if id == self.sequence_name { - if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { - if id == self.index_name { - self.diagnostic_ranges.push(*range); - } - } + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return; + }; + if id == self.sequence_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return; + }; + if id == self.index_name { + self.diagnostic_ranges.push(*range); } } } @@ -136,13 +140,15 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { id == self.sequence_name || id == self.index_name } Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { - if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { - if id == self.sequence_name { - if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { - if id == self.index_name { - return true; - } - } + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return false; + }; + if id == self.sequence_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return false; + }; + if id == self.index_name { + return true; } } false @@ -198,23 +204,25 @@ pub(crate) fn unnecessary_list_index_lookup_comprehension(checker: &mut Checker, elt, generators, .. }) => { for comp in generators { - if let Some((sequence, index_name, value_name)) = + let Some((sequence, index_name, value_name)) = enumerate_items(checker, &comp.iter, &comp.target) - { - let mut visitor = SubscriptVisitor::new(&sequence, &index_name); + else { + return; + }; - visitor.visit_expr(elt.as_ref()); + let mut visitor = SubscriptVisitor::new(&sequence, &index_name); - for range in visitor.diagnostic_ranges { - let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); + visitor.visit_expr(elt.as_ref()); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - value_name.clone(), - range, - ))); + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); - checker.diagnostics.push(diagnostic); - } + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); } } } @@ -227,12 +235,9 @@ fn enumerate_items( call_expr: &Expr, tuple_expr: &Expr, ) -> Option<(String, String, String)> { - let Expr::Call(ast::ExprCall { + let ast::ExprCall { func, arguments, .. - }) = call_expr - else { - return None; - }; + } = call_expr.as_call_expr()?; // Check that the function is the `enumerate` builtin. let Some(call_path) = checker.semantic().resolve_call_path(func.as_ref()) else {