From 98d91ac46262aef92acb2efd38deb24ebfdb8d00 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 16 Oct 2023 22:13:06 -0400 Subject: [PATCH 01/12] add R1733 and autofix --- .../pylint/unnecessary_dict_index_lookup.py | 20 +++ .../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_dict_index_lookup.rs | 156 ++++++++++++++++++ ...1733_unnecessary_dict_index_lookup.py.snap | 64 +++++++ ruff.schema.json | 1 + 8 files changed, 251 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py new file mode 100644 index 0000000000000..9bf289969d01d --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py @@ -0,0 +1,20 @@ +FRUITS = {"apple": 1, "orange": 10, "berry": 22} + +def fix_these(): + for fruit_name, fruit_count in FRUITS.items(): + print(FRUITS[fruit_name]) # PLR1733 + blah = FRUITS[fruit_name] # PLR1733 + FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + + +def dont_fix_these(): + for fruit_name, fruit_count in FRUITS.items(): + FRUITS[fruit_name] = 0 # Ok + + +def value_intentionally_unused(): + for fruit_name, _ in FRUITS.items(): + print(FRUITS[fruit_name]) # Ok + blah = FRUITS[fruit_name] # Ok + FRUITS[fruit_name] = FRUITS[fruit_name] # Ok + FRUITS[fruit_name] = "d" # Ok diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 2208db8779b7d..d4e6162221938 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1280,6 +1280,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListIndexLookup) { pylint::rules::unnecessary_list_index_lookup(checker, for_stmt); } + if checker.enabled(Rule::UnnecessaryDictIndexLookup) { + pylint::rules::unnecessary_dict_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 8ee01e94a1f39..3ea74781f7fb5 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -259,6 +259,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, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index f8417646232ea..ba4536eaf9f49 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -160,6 +160,10 @@ mod tests { )] #[test_case(Rule::NoClassmethodDecorator, Path::new("no_method_decorator.py"))] #[test_case(Rule::NoStaticmethodDecorator, Path::new("no_method_decorator.py"))] + #[test_case( + Rule::UnnecessaryDictIndexLookup, + Path::new("unnecessary_dict_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 6497f9dcb7e27..02ae6796a5dfb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -62,6 +62,7 @@ pub(crate) use type_bivariance::*; pub(crate) use type_name_incorrect_variance::*; pub(crate) use type_param_name_mismatch::*; pub(crate) use unexpected_special_method_signature::*; +pub(crate) use unnecessary_dict_index_lookup::*; pub(crate) use unnecessary_direct_lambda_call::*; pub(crate) use unnecessary_lambda::*; pub(crate) use unnecessary_list_index_lookup::*; @@ -137,6 +138,7 @@ mod type_bivariance; mod type_name_incorrect_variance; mod type_param_name_mismatch; mod unexpected_special_method_signature; +mod unnecessary_dict_index_lookup; mod unnecessary_direct_lambda_call; mod unnecessary_lambda; mod unnecessary_list_index_lookup; diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs new file mode 100644 index 0000000000000..c3fb073899ae9 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -0,0 +1,156 @@ +use ast::{Arguments, Stmt}; +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 `dict.items()` and accessing the value by index lookup. +/// +/// ## Why is this bad? +/// The value is already accessible by the 2nd variable from `dict.items()`. +/// +/// ## Example +/// ```python +/// FRUITS = {"apple": 1, "orange": 10, "berry": 22} +/// +/// for fruit_name, fruit_count in FRUITS.items(): +/// print(FRUITS[fruit_name]) +/// ``` +/// +/// Use instead: +/// ```python +/// FRUITS = {"apple": 1, "orange": 10, "berry": 22} +/// +/// for fruit_name, fruit_count in FRUITS.items(): +/// print(fruit_count) +/// ``` +#[violation] +pub struct UnnecessaryDictIndexLookup; + +impl AlwaysFixableViolation for UnnecessaryDictIndexLookup { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unnecessary dict index lookup") + } + + fn fix_title(&self) -> String { + format!("Remove unnecessary dict index lookup") + } +} + +struct SubscriptVisitor<'a> { + dict_name: &'a str, + index_name: &'a str, + diagnostic_ranges: Vec, +} + +impl<'a> SubscriptVisitor<'a> { + fn new(dict_name: &'a str, index_name: &'a str) -> Self { + Self { + dict_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.dict_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), + } + } + + fn visit_stmt(&mut self, stmt: &Stmt) { + match stmt { + Stmt::Assign(ast::StmtAssign { value, .. }) => { + self.visit_expr(value); + } + _ => visitor::walk_stmt(self, stmt), + } + } +} + +/// PLR1733 +pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &StmtFor) { + let Expr::Call(ast::ExprCall { + func, + arguments: Arguments { args, .. }, + .. + }) = stmt_for.iter.as_ref() + else { + return; + }; + if !args.is_empty() { + return; + } + let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { + return; + }; + if attr != "items" { + return; + } + + let Expr::Name(ast::ExprName { id: dict_name, .. }) = value.as_ref() else { + 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; + }; + + // If either of the variable names are intentionally ignored by naming them `_`, then don't emit + if index_name == "_" || value_name == "_" { + return; + } + + let mut visitor = SubscriptVisitor::new(dict_name, 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(UnnecessaryDictIndexLookup, 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__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap new file mode 100644 index 0000000000000..7c7517a4e806f --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -0,0 +1,64 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +unnecessary_dict_index_lookup.py:5:15: PLR1733 [*] Unnecessary dict index lookup + | +3 | def fix_these(): +4 | for fruit_name, fruit_count in FRUITS.items(): +5 | print(FRUITS[fruit_name]) # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +6 | blah = FRUITS[fruit_name] # PLR1733 +7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +2 2 | +3 3 | def fix_these(): +4 4 | for fruit_name, fruit_count in FRUITS.items(): +5 |- print(FRUITS[fruit_name]) # PLR1733 + 5 |+ print(fruit_count) # PLR1733 +6 6 | blah = FRUITS[fruit_name] # PLR1733 +7 7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 8 | + +unnecessary_dict_index_lookup.py:6:16: PLR1733 [*] Unnecessary dict index lookup + | +4 | for fruit_name, fruit_count in FRUITS.items(): +5 | print(FRUITS[fruit_name]) # PLR1733 +6 | blah = FRUITS[fruit_name] # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +3 3 | def fix_these(): +4 4 | for fruit_name, fruit_count in FRUITS.items(): +5 5 | print(FRUITS[fruit_name]) # PLR1733 +6 |- blah = FRUITS[fruit_name] # PLR1733 + 6 |+ blah = fruit_count # PLR1733 +7 7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 8 | +9 9 | + +unnecessary_dict_index_lookup.py:7:30: PLR1733 [*] Unnecessary dict index lookup + | +5 | print(FRUITS[fruit_name]) # PLR1733 +6 | blah = FRUITS[fruit_name] # PLR1733 +7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +4 4 | for fruit_name, fruit_count in FRUITS.items(): +5 5 | print(FRUITS[fruit_name]) # PLR1733 +6 6 | blah = FRUITS[fruit_name] # PLR1733 +7 |- FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + 7 |+ FRUITS[fruit_name] = fruit_count # PLR1733 (on the right hand) +8 8 | +9 9 | +10 10 | def dont_fix_these(): + + diff --git a/ruff.schema.json b/ruff.schema.json index 4f39eaeb420e0..b0584cad84735 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3129,6 +3129,7 @@ "PLR172", "PLR1722", "PLR173", + "PLR1733", "PLR1736", "PLR2", "PLR20", From 7f0213c71ec994eceb403ca69551a36b9f2af289 Mon Sep 17 00:00:00 2001 From: Steve C Date: Wed, 18 Oct 2023 02:10:16 -0400 Subject: [PATCH 02/12] add the other assignments --- .../pylint/unnecessary_dict_index_lookup.py | 4 ++ .../rules/unnecessary_dict_index_lookup.rs | 8 +++ ...1733_unnecessary_dict_index_lookup.py.snap | 72 +++++++++++++++---- 3 files changed, 69 insertions(+), 15 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py index 9bf289969d01d..f6b88ba7b2d71 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py @@ -4,7 +4,9 @@ def fix_these(): for fruit_name, fruit_count in FRUITS.items(): print(FRUITS[fruit_name]) # PLR1733 blah = FRUITS[fruit_name] # PLR1733 + FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) def dont_fix_these(): @@ -16,5 +18,7 @@ def value_intentionally_unused(): for fruit_name, _ in FRUITS.items(): print(FRUITS[fruit_name]) # Ok blah = FRUITS[fruit_name] # Ok + FRUITS[fruit_name]: int = FRUITS[fruit_name] # Ok FRUITS[fruit_name] = FRUITS[fruit_name] # Ok + FRUITS[fruit_name] += FRUITS[fruit_name] # Ok FRUITS[fruit_name] = "d" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index c3fb073899ae9..ba52284004763 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -88,6 +88,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__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap index 7c7517a4e806f..495e141097566 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -8,7 +8,7 @@ unnecessary_dict_index_lookup.py:5:15: PLR1733 [*] Unnecessary dict index lookup 5 | print(FRUITS[fruit_name]) # PLR1733 | ^^^^^^^^^^^^^^^^^^ PLR1733 6 | blah = FRUITS[fruit_name] # PLR1733 -7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) | = help: Remove unnecessary dict index lookup @@ -19,8 +19,8 @@ unnecessary_dict_index_lookup.py:5:15: PLR1733 [*] Unnecessary dict index lookup 5 |- print(FRUITS[fruit_name]) # PLR1733 5 |+ print(fruit_count) # PLR1733 6 6 | blah = FRUITS[fruit_name] # PLR1733 -7 7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 8 | +7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) unnecessary_dict_index_lookup.py:6:16: PLR1733 [*] Unnecessary dict index lookup | @@ -28,7 +28,8 @@ unnecessary_dict_index_lookup.py:6:16: PLR1733 [*] Unnecessary dict index lookup 5 | print(FRUITS[fruit_name]) # PLR1733 6 | blah = FRUITS[fruit_name] # PLR1733 | ^^^^^^^^^^^^^^^^^^ PLR1733 -7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) | = help: Remove unnecessary dict index lookup @@ -38,16 +39,18 @@ unnecessary_dict_index_lookup.py:6:16: PLR1733 [*] Unnecessary dict index lookup 5 5 | print(FRUITS[fruit_name]) # PLR1733 6 |- blah = FRUITS[fruit_name] # PLR1733 6 |+ blah = fruit_count # PLR1733 -7 7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 8 | -9 9 | +7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +9 9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) -unnecessary_dict_index_lookup.py:7:30: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:7:35: PLR1733 [*] Unnecessary dict index lookup | 5 | print(FRUITS[fruit_name]) # PLR1733 6 | blah = FRUITS[fruit_name] # PLR1733 -7 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 +7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 +8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) | = help: Remove unnecessary dict index lookup @@ -55,10 +58,49 @@ unnecessary_dict_index_lookup.py:7:30: PLR1733 [*] Unnecessary dict index lookup 4 4 | for fruit_name, fruit_count in FRUITS.items(): 5 5 | print(FRUITS[fruit_name]) # PLR1733 6 6 | blah = FRUITS[fruit_name] # PLR1733 -7 |- FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - 7 |+ FRUITS[fruit_name] = fruit_count # PLR1733 (on the right hand) -8 8 | -9 9 | -10 10 | def dont_fix_these(): +7 |- FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) + 7 |+ FRUITS[fruit_name]: int = fruit_count # PLR1733 (on the right hand) +8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +9 9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +10 10 | + +unnecessary_dict_index_lookup.py:8:30: PLR1733 [*] Unnecessary dict index lookup + | +6 | blah = FRUITS[fruit_name] # PLR1733 +7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 +9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +5 5 | print(FRUITS[fruit_name]) # PLR1733 +6 6 | blah = FRUITS[fruit_name] # PLR1733 +7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 |- FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + 8 |+ FRUITS[fruit_name] = fruit_count # PLR1733 (on the right hand) +9 9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +10 10 | +11 11 | + +unnecessary_dict_index_lookup.py:9:31: PLR1733 [*] Unnecessary dict index lookup + | +7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +6 6 | blah = FRUITS[fruit_name] # PLR1733 +7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +9 |- FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + 9 |+ FRUITS[fruit_name] += fruit_count # PLR1733 (on the right hand) +10 10 | +11 11 | +12 12 | def dont_fix_these(): From 0307325600c867d9c9bfacd36f5e881328f0b78a Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 19 Oct 2023 20:33:58 -0400 Subject: [PATCH 03/12] add generator/compehrension expressions --- .../pylint/unnecessary_dict_index_lookup.py | 8 + .../src/checkers/ast/analyze/expression.rs | 12 + .../rules/unnecessary_dict_index_lookup.rs | 110 ++++++--- ...1733_unnecessary_dict_index_lookup.py.snap | 216 +++++++++++------- 4 files changed, 241 insertions(+), 105 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py index f6b88ba7b2d71..0558cfc54ef12 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py @@ -1,6 +1,10 @@ FRUITS = {"apple": 1, "orange": 10, "berry": 22} def fix_these(): + [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 + {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 + {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 + for fruit_name, fruit_count in FRUITS.items(): print(FRUITS[fruit_name]) # PLR1733 blah = FRUITS[fruit_name] # PLR1733 @@ -15,6 +19,10 @@ def dont_fix_these(): def value_intentionally_unused(): + [FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()] # PLR1733 + {FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()} # PLR1733 + {fruit_name: FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()} # PLR1733 + for fruit_name, _ in FRUITS.items(): print(FRUITS[fruit_name]) # Ok blah = FRUITS[fruit_name] # Ok diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 0f5a7c3b60643..87a354b9641f3 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1333,6 +1333,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListIndexLookup) { pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); } + if checker.enabled(Rule::UnnecessaryDictIndexLookup) { + pylint::rules::unnecessary_dict_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::UnnecessaryComprehension) { flake8_comprehensions::rules::unnecessary_list_set_comprehension( checker, expr, elt, generators, @@ -1360,6 +1363,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListIndexLookup) { pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); } + if checker.enabled(Rule::UnnecessaryDictIndexLookup) { + pylint::rules::unnecessary_dict_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::UnnecessaryComprehension) { flake8_comprehensions::rules::unnecessary_list_set_comprehension( checker, expr, elt, generators, @@ -1386,6 +1392,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListIndexLookup) { pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); } + if checker.enabled(Rule::UnnecessaryDictIndexLookup) { + pylint::rules::unnecessary_dict_index_lookup_comprehension(checker, expr); + } if checker.enabled(Rule::UnnecessaryComprehension) { flake8_comprehensions::rules::unnecessary_dict_comprehension( checker, expr, key, value, generators, @@ -1413,6 +1422,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListIndexLookup) { pylint::rules::unnecessary_list_index_lookup_comprehension(checker, expr); } + if checker.enabled(Rule::UnnecessaryDictIndexLookup) { + pylint::rules::unnecessary_dict_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_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index ba52284004763..161e45b90caf1 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -103,62 +103,118 @@ impl<'a> Visitor<'_> for SubscriptVisitor<'a> { /// PLR1733 pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &StmtFor) { + let Some((dict_name, index_name, value_name)) = dict_items(&stmt_for.iter, &stmt_for.target) + else { + return; + }; + + let mut visitor = SubscriptVisitor::new(&dict_name, &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(UnnecessaryDictIndexLookup, range); + + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); + } +} + +/// PLR1733 +pub(crate) fn unnecessary_dict_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 { + let Some((dict_name, index_name, value_name)) = + dict_items(&comp.iter, &comp.target) + else { + continue; + }; + + let mut visitor = SubscriptVisitor::new(&dict_name, &index_name); + + visitor.visit_expr(elt.as_ref()); + for expr in &comp.ifs { + visitor.visit_expr(expr); + } + + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range); + + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); + } + } + } + _ => (), + } +} + +fn dict_items(call_expr: &Expr, tuple_expr: &Expr) -> Option<(String, String, String)> { let Expr::Call(ast::ExprCall { func, arguments: Arguments { args, .. }, .. - }) = stmt_for.iter.as_ref() + }) = call_expr else { - return; + return None; }; if !args.is_empty() { - return; + return None; } let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { - return; + return None; }; if attr != "items" { - return; + return None; } let Expr::Name(ast::ExprName { id: dict_name, .. }) = value.as_ref() else { - 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; } - let mut visitor = SubscriptVisitor::new(dict_name, 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(UnnecessaryDictIndexLookup, range); - - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - value_name.clone(), - range, - ))); - - checker.diagnostics.push(diagnostic); - } + Some((dict_name.clone(), index_name.clone(), value_name.clone())) } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap index 495e141097566..60eb5107e5b58 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -1,106 +1,166 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -unnecessary_dict_index_lookup.py:5:15: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary dict index lookup | 3 | def fix_these(): -4 | for fruit_name, fruit_count in FRUITS.items(): -5 | print(FRUITS[fruit_name]) # PLR1733 - | ^^^^^^^^^^^^^^^^^^ PLR1733 -6 | blah = FRUITS[fruit_name] # PLR1733 -7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 | = help: Remove unnecessary dict index lookup ℹ Fix +1 1 | FRUITS = {"apple": 1, "orange": 10, "berry": 22} 2 2 | 3 3 | def fix_these(): -4 4 | for fruit_name, fruit_count in FRUITS.items(): -5 |- print(FRUITS[fruit_name]) # PLR1733 - 5 |+ print(fruit_count) # PLR1733 -6 6 | blah = FRUITS[fruit_name] # PLR1733 -7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - -unnecessary_dict_index_lookup.py:6:16: PLR1733 [*] Unnecessary dict index lookup +4 |- [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 + 4 |+ [fruit_count for fruit_name, fruit_count in FRUITS.items()] # PLR1733 +5 5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +7 7 | + +unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary dict index lookup | -4 | for fruit_name, fruit_count in FRUITS.items(): -5 | print(FRUITS[fruit_name]) # PLR1733 -6 | blah = FRUITS[fruit_name] # PLR1733 - | ^^^^^^^^^^^^^^^^^^ PLR1733 -7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +3 | def fix_these(): +4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 +5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 | = help: Remove unnecessary dict index lookup ℹ Fix +2 2 | 3 3 | def fix_these(): -4 4 | for fruit_name, fruit_count in FRUITS.items(): -5 5 | print(FRUITS[fruit_name]) # PLR1733 -6 |- blah = FRUITS[fruit_name] # PLR1733 - 6 |+ blah = fruit_count # PLR1733 -7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -9 9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - -unnecessary_dict_index_lookup.py:7:35: PLR1733 [*] Unnecessary dict index lookup +4 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 +5 |- {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 + 5 |+ {fruit_count for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +7 7 | +8 8 | for fruit_name, fruit_count in FRUITS.items(): + +unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary dict index lookup | -5 | print(FRUITS[fruit_name]) # PLR1733 -6 | blah = FRUITS[fruit_name] # PLR1733 -7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 -8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 +5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +7 | +8 | for fruit_name, fruit_count in FRUITS.items(): | = help: Remove unnecessary dict index lookup ℹ Fix -4 4 | for fruit_name, fruit_count in FRUITS.items(): -5 5 | print(FRUITS[fruit_name]) # PLR1733 -6 6 | blah = FRUITS[fruit_name] # PLR1733 -7 |- FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) - 7 |+ FRUITS[fruit_name]: int = fruit_count # PLR1733 (on the right hand) -8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -9 9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) -10 10 | - -unnecessary_dict_index_lookup.py:8:30: PLR1733 [*] Unnecessary dict index lookup - | -6 | blah = FRUITS[fruit_name] # PLR1733 -7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 -9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - | - = help: Remove unnecessary dict index lookup +3 3 | def fix_these(): +4 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 +5 5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +6 |- {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 + 6 |+ {fruit_name: fruit_count for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +7 7 | +8 8 | for fruit_name, fruit_count in FRUITS.items(): +9 9 | print(FRUITS[fruit_name]) # PLR1733 + +unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup + | + 8 | for fruit_name, fruit_count in FRUITS.items(): + 9 | print(FRUITS[fruit_name]) # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +10 | blah = FRUITS[fruit_name] # PLR1733 +11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup ℹ Fix -5 5 | print(FRUITS[fruit_name]) # PLR1733 -6 6 | blah = FRUITS[fruit_name] # PLR1733 -7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 |- FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - 8 |+ FRUITS[fruit_name] = fruit_count # PLR1733 (on the right hand) -9 9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) -10 10 | -11 11 | - -unnecessary_dict_index_lookup.py:9:31: PLR1733 [*] Unnecessary dict index lookup - | -7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -9 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 - | - = help: Remove unnecessary dict index lookup +6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 +7 7 | +8 8 | for fruit_name, fruit_count in FRUITS.items(): +9 |- print(FRUITS[fruit_name]) # PLR1733 + 9 |+ print(fruit_count) # PLR1733 +10 10 | blah = FRUITS[fruit_name] # PLR1733 +11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + +unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index lookup + | + 8 | for fruit_name, fruit_count in FRUITS.items(): + 9 | print(FRUITS[fruit_name]) # PLR1733 +10 | blah = FRUITS[fruit_name] # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 +11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +7 7 | +8 8 | for fruit_name, fruit_count in FRUITS.items(): +9 9 | print(FRUITS[fruit_name]) # PLR1733 +10 |- blah = FRUITS[fruit_name] # PLR1733 + 10 |+ blah = fruit_count # PLR1733 +11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +13 13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + +unnecessary_dict_index_lookup.py:11:35: PLR1733 [*] Unnecessary dict index lookup + | + 9 | print(FRUITS[fruit_name]) # PLR1733 +10 | blah = FRUITS[fruit_name] # PLR1733 +11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 +12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +8 8 | for fruit_name, fruit_count in FRUITS.items(): +9 9 | print(FRUITS[fruit_name]) # PLR1733 +10 10 | blah = FRUITS[fruit_name] # PLR1733 +11 |- FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) + 11 |+ FRUITS[fruit_name]: int = fruit_count # PLR1733 (on the right hand) +12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +13 13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +14 14 | + +unnecessary_dict_index_lookup.py:12:30: PLR1733 [*] Unnecessary dict index lookup + | +10 | blah = FRUITS[fruit_name] # PLR1733 +11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 +13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + | + = help: Remove unnecessary dict index lookup + +ℹ Fix +9 9 | print(FRUITS[fruit_name]) # PLR1733 +10 10 | blah = FRUITS[fruit_name] # PLR1733 +11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 |- FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) + 12 |+ FRUITS[fruit_name] = fruit_count # PLR1733 (on the right hand) +13 13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +14 14 | +15 15 | + +unnecessary_dict_index_lookup.py:13:31: PLR1733 [*] Unnecessary dict index lookup + | +11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + | ^^^^^^^^^^^^^^^^^^ PLR1733 + | + = help: Remove unnecessary dict index lookup ℹ Fix -6 6 | blah = FRUITS[fruit_name] # PLR1733 -7 7 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -8 8 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -9 |- FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - 9 |+ FRUITS[fruit_name] += fruit_count # PLR1733 (on the right hand) -10 10 | -11 11 | -12 12 | def dont_fix_these(): +10 10 | blah = FRUITS[fruit_name] # PLR1733 +11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +13 |- FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + 13 |+ FRUITS[fruit_name] += fruit_count # PLR1733 (on the right hand) +14 14 | +15 15 | +16 16 | def dont_fix_these(): From ae0806e9f12af08dbd9499d99682045e3b96981e Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 4 Nov 2023 14:26:13 -0400 Subject: [PATCH 04/12] tweak logic for assignments --- .../pylint/unnecessary_dict_index_lookup.py | 11 ++- .../rules/unnecessary_dict_index_lookup.rs | 40 ++++++++++- ...1733_unnecessary_dict_index_lookup.py.snap | 72 ++++--------------- 3 files changed, 56 insertions(+), 67 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py index 0558cfc54ef12..ebb8e8a57e30a 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py @@ -8,14 +8,14 @@ def fix_these(): for fruit_name, fruit_count in FRUITS.items(): print(FRUITS[fruit_name]) # PLR1733 blah = FRUITS[fruit_name] # PLR1733 - FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) - FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) + assert FRUITS[fruit_name] == "pear" # PLR1733 def dont_fix_these(): + # once there is an assignment to the dict[index], we stop emitting diagnostics for fruit_name, fruit_count in FRUITS.items(): FRUITS[fruit_name] = 0 # Ok + assert FRUITS[fruit_name] == 0 # Ok def value_intentionally_unused(): @@ -26,7 +26,4 @@ def value_intentionally_unused(): for fruit_name, _ in FRUITS.items(): print(FRUITS[fruit_name]) # Ok blah = FRUITS[fruit_name] # Ok - FRUITS[fruit_name]: int = FRUITS[fruit_name] # Ok - FRUITS[fruit_name] = FRUITS[fruit_name] # Ok - FRUITS[fruit_name] += FRUITS[fruit_name] # Ok - FRUITS[fruit_name] = "d" # Ok + assert FRUITS[fruit_name] == "pear" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index 161e45b90caf1..c973c6a3a9a53 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -48,6 +48,7 @@ struct SubscriptVisitor<'a> { dict_name: &'a str, index_name: &'a str, diagnostic_ranges: Vec, + is_subcript_modified: bool, } impl<'a> SubscriptVisitor<'a> { @@ -56,12 +57,35 @@ impl<'a> SubscriptVisitor<'a> { dict_name, index_name, diagnostic_ranges: Vec::new(), + is_subcript_modified: false, } } } +fn check_target_for_assignment(expr: &Expr, dict_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 == dict_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, @@ -84,16 +108,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.dict_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.dict_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.dict_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__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap index 60eb5107e5b58..80e0e2c59bf97 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -68,7 +68,7 @@ unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup 9 | print(FRUITS[fruit_name]) # PLR1733 | ^^^^^^^^^^^^^^^^^^ PLR1733 10 | blah = FRUITS[fruit_name] # PLR1733 -11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) +11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | = help: Remove unnecessary dict index lookup @@ -79,8 +79,8 @@ unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup 9 |- print(FRUITS[fruit_name]) # PLR1733 9 |+ print(fruit_count) # PLR1733 10 10 | blah = FRUITS[fruit_name] # PLR1733 -11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +11 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 +12 12 | unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index lookup | @@ -88,8 +88,7 @@ unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index looku 9 | print(FRUITS[fruit_name]) # PLR1733 10 | blah = FRUITS[fruit_name] # PLR1733 | ^^^^^^^^^^^^^^^^^^ PLR1733 -11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) +11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | = help: Remove unnecessary dict index lookup @@ -99,18 +98,16 @@ unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index looku 9 9 | print(FRUITS[fruit_name]) # PLR1733 10 |- blah = FRUITS[fruit_name] # PLR1733 10 |+ blah = fruit_count # PLR1733 -11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -13 13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +11 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 +12 12 | +13 13 | -unnecessary_dict_index_lookup.py:11:35: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:11:16: PLR1733 [*] Unnecessary dict index lookup | 9 | print(FRUITS[fruit_name]) # PLR1733 10 | blah = FRUITS[fruit_name] # PLR1733 -11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 -12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) +11 | assert FRUITS[fruit_name] == "pear" # PLR1733 + | ^^^^^^^^^^^^^^^^^^ PLR1733 | = help: Remove unnecessary dict index lookup @@ -118,49 +115,10 @@ unnecessary_dict_index_lookup.py:11:35: PLR1733 [*] Unnecessary dict index looku 8 8 | for fruit_name, fruit_count in FRUITS.items(): 9 9 | print(FRUITS[fruit_name]) # PLR1733 10 10 | blah = FRUITS[fruit_name] # PLR1733 -11 |- FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) - 11 |+ FRUITS[fruit_name]: int = fruit_count # PLR1733 (on the right hand) -12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -13 13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) -14 14 | - -unnecessary_dict_index_lookup.py:12:30: PLR1733 [*] Unnecessary dict index lookup - | -10 | blah = FRUITS[fruit_name] # PLR1733 -11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 -13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - | - = help: Remove unnecessary dict index lookup - -ℹ Fix -9 9 | print(FRUITS[fruit_name]) # PLR1733 -10 10 | blah = FRUITS[fruit_name] # PLR1733 -11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 |- FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) - 12 |+ FRUITS[fruit_name] = fruit_count # PLR1733 (on the right hand) -13 13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) -14 14 | -15 15 | - -unnecessary_dict_index_lookup.py:13:31: PLR1733 [*] Unnecessary dict index lookup - | -11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -13 | FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - | ^^^^^^^^^^^^^^^^^^ PLR1733 - | - = help: Remove unnecessary dict index lookup - -ℹ Fix -10 10 | blah = FRUITS[fruit_name] # PLR1733 -11 11 | FRUITS[fruit_name]: int = FRUITS[fruit_name] # PLR1733 (on the right hand) -12 12 | FRUITS[fruit_name] = FRUITS[fruit_name] # PLR1733 (on the right hand) -13 |- FRUITS[fruit_name] += FRUITS[fruit_name] # PLR1733 (on the right hand) - 13 |+ FRUITS[fruit_name] += fruit_count # PLR1733 (on the right hand) -14 14 | -15 15 | -16 16 | def dont_fix_these(): +11 |- assert FRUITS[fruit_name] == "pear" # PLR1733 + 11 |+ assert fruit_count == "pear" # PLR1733 +12 12 | +13 13 | +14 14 | def dont_fix_these(): From d5c72f0b03b205ab896504263c13277c74b67d43 Mon Sep 17 00:00:00 2001 From: Steve C Date: Fri, 24 Nov 2023 15:11:25 -0500 Subject: [PATCH 05/12] 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 3ea74781f7fb5..cb1f907b658e0 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -259,8 +259,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, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), + (Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup), (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison), (Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf), From 877382c5954567597f692adcf16dc0860db9f1dc Mon Sep 17 00:00:00 2001 From: Steve C Date: Fri, 24 Nov 2023 15:33:03 -0500 Subject: [PATCH 06/12] update snapshots --- ...ts__PLR1733_unnecessary_dict_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__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap index 80e0e2c59bf97..45efa392dac50 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -11,7 +11,7 @@ unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary dict index lookup | = help: Remove unnecessary dict index lookup -ℹ Fix +ℹ Safe fix 1 1 | FRUITS = {"apple": 1, "orange": 10, "berry": 22} 2 2 | 3 3 | def fix_these(): @@ -31,7 +31,7 @@ unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary dict index lookup | = help: Remove unnecessary dict index lookup -ℹ Fix +ℹ Safe fix 2 2 | 3 3 | def fix_these(): 4 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 @@ -52,7 +52,7 @@ unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary dict index lookup | = help: Remove unnecessary dict index lookup -ℹ Fix +ℹ Safe fix 3 3 | def fix_these(): 4 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 5 5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 @@ -72,7 +72,7 @@ unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup | = help: Remove unnecessary dict index lookup -ℹ Fix +ℹ Safe fix 6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 7 7 | 8 8 | for fruit_name, fruit_count in FRUITS.items(): @@ -92,7 +92,7 @@ unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index looku | = help: Remove unnecessary dict index lookup -ℹ Fix +ℹ Safe fix 7 7 | 8 8 | for fruit_name, fruit_count in FRUITS.items(): 9 9 | print(FRUITS[fruit_name]) # PLR1733 @@ -111,7 +111,7 @@ unnecessary_dict_index_lookup.py:11:16: PLR1733 [*] Unnecessary dict index looku | = help: Remove unnecessary dict index lookup -ℹ Fix +ℹ Safe fix 8 8 | for fruit_name, fruit_count in FRUITS.items(): 9 9 | print(FRUITS[fruit_name]) # PLR1733 10 10 | blah = FRUITS[fruit_name] # PLR1733 From 4811f7237f1347c96462467e6e0be86c3fa8953a Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 00:16:17 -0500 Subject: [PATCH 07/12] tweak wordings --- .../rules/pylint/rules/unnecessary_dict_index_lookup.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index c973c6a3a9a53..fdc690c8e9808 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -10,10 +10,10 @@ use ruff_text_size::TextRange; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for uses of `dict.items()` and accessing the value by index lookup. +/// Checks for access of a dict value at the current index when iterating through the dict items. /// /// ## Why is this bad? -/// The value is already accessible by the 2nd variable from `dict.items()`. +/// 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 @@ -36,11 +36,11 @@ pub struct UnnecessaryDictIndexLookup; impl AlwaysFixableViolation for UnnecessaryDictIndexLookup { #[derive_message_formats] fn message(&self) -> String { - format!("Unnecessary dict index lookup") + format!("Unnecessary lookup of dict item by index") } fn fix_title(&self) -> String { - format!("Remove unnecessary dict index lookup") + format!("Use existing item variable instead") } } From 9ae5ed2cd716bc43120e0f17c00c161558eb7cdc Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 02:49:34 -0500 Subject: [PATCH 08/12] fix comments --- .../test/fixtures/pylint/unnecessary_dict_index_lookup.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py index ebb8e8a57e30a..cfdd9fc42ae04 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py @@ -19,9 +19,9 @@ def dont_fix_these(): def value_intentionally_unused(): - [FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()] # PLR1733 - {FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()} # PLR1733 - {fruit_name: FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()} # PLR1733 + [FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()] # Ok + {FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()} # Ok + {fruit_name: FRUITS[fruit_name] for fruit_name, _ in FRUITS.items()} # Ok for fruit_name, _ in FRUITS.items(): print(FRUITS[fruit_name]) # Ok From b4b47fbb85e294a3f8c18ba26e4415b40080fc6e Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 28 Nov 2023 02:49:47 -0500 Subject: [PATCH 09/12] fix snapshot --- ...1733_unnecessary_dict_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__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap index 45efa392dac50..aeaad23437783 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary lookup of dict item by index | 3 | def fix_these(): 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 @@ -9,7 +9,7 @@ unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary dict index lookup 5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 | - = help: Remove unnecessary dict index lookup + = help: Use existing item variable instead ℹ Safe fix 1 1 | FRUITS = {"apple": 1, "orange": 10, "berry": 22} @@ -21,7 +21,7 @@ unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary dict index lookup 6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 7 7 | -unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary lookup of dict item by index | 3 | def fix_these(): 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 @@ -29,7 +29,7 @@ unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary dict index lookup | ^^^^^^^^^^^^^^^^^^ PLR1733 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 | - = help: Remove unnecessary dict index lookup + = help: Use existing item variable instead ℹ Safe fix 2 2 | @@ -41,7 +41,7 @@ unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary dict index lookup 7 7 | 8 8 | for fruit_name, fruit_count in FRUITS.items(): -unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary lookup of dict item by index | 4 | [FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()] # PLR1733 5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 @@ -50,7 +50,7 @@ unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary dict index lookup 7 | 8 | for fruit_name, fruit_count in FRUITS.items(): | - = help: Remove unnecessary dict index lookup + = help: Use existing item variable instead ℹ Safe fix 3 3 | def fix_these(): @@ -62,7 +62,7 @@ unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary dict index lookup 8 8 | for fruit_name, fruit_count in FRUITS.items(): 9 9 | print(FRUITS[fruit_name]) # PLR1733 -unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary lookup of dict item by index | 8 | for fruit_name, fruit_count in FRUITS.items(): 9 | print(FRUITS[fruit_name]) # PLR1733 @@ -70,7 +70,7 @@ unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup 10 | blah = FRUITS[fruit_name] # PLR1733 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | - = help: Remove unnecessary dict index lookup + = help: Use existing item variable instead ℹ Safe fix 6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 @@ -82,7 +82,7 @@ unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary dict index lookup 11 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 12 12 | -unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary lookup of dict item by index | 8 | for fruit_name, fruit_count in FRUITS.items(): 9 | print(FRUITS[fruit_name]) # PLR1733 @@ -90,7 +90,7 @@ unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index looku | ^^^^^^^^^^^^^^^^^^ PLR1733 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | - = help: Remove unnecessary dict index lookup + = help: Use existing item variable instead ℹ Safe fix 7 7 | @@ -102,14 +102,14 @@ unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary dict index looku 12 12 | 13 13 | -unnecessary_dict_index_lookup.py:11:16: PLR1733 [*] Unnecessary dict index lookup +unnecessary_dict_index_lookup.py:11:16: PLR1733 [*] Unnecessary lookup of dict item by index | 9 | print(FRUITS[fruit_name]) # PLR1733 10 | blah = FRUITS[fruit_name] # PLR1733 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | ^^^^^^^^^^^^^^^^^^ PLR1733 | - = help: Remove unnecessary dict index lookup + = help: Use existing item variable instead ℹ Safe fix 8 8 | for fruit_name, fruit_count in FRUITS.items(): From a6a286e4c285e69ce430ad17ce2aaf88aa82bfb4 Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 30 Nov 2023 18:32:44 -0500 Subject: [PATCH 10/12] tweaks --- .../rules/unnecessary_dict_index_lookup.rs | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index fdc690c8e9808..ff49560bd303c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -1,4 +1,4 @@ -use ast::{Arguments, Stmt}; +use ast::Stmt; use ruff_python_ast::{self as ast, Expr, StmtFor}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; @@ -66,13 +66,15 @@ fn check_target_for_assignment(expr: &Expr, dict_name: &str, index_name: &str) - // 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 == dict_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 == dict_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.dict_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.dict_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return; + }; + if id == self.index_name { + self.diagnostic_ranges.push(*range); } } } @@ -207,15 +211,11 @@ pub(crate) fn unnecessary_dict_index_lookup_comprehension(checker: &mut Checker, } fn dict_items(call_expr: &Expr, tuple_expr: &Expr) -> Option<(String, String, String)> { - let Expr::Call(ast::ExprCall { - func, - arguments: Arguments { args, .. }, - .. - }) = call_expr - else { - return None; - }; - if !args.is_empty() { + let ast::ExprCall { + func, arguments, .. + } = call_expr.as_call_expr()?; + + if !arguments.is_empty() { return None; } let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { From b32cc3979454e86a175d079ca5cf16e4b90c661e Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 30 Nov 2023 22:51:47 -0500 Subject: [PATCH 11/12] Make suggested changes --- .../rules/unnecessary_dict_index_lookup.rs | 277 +++++++++--------- ...1733_unnecessary_dict_index_lookup.py.snap | 12 +- 2 files changed, 145 insertions(+), 144 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index ff49560bd303c..e623f6797ed6d 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -10,10 +10,12 @@ use ruff_text_size::TextRange; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for access of a dict value at the current index when iterating through the dict items. +/// Checks for index-based dict accesses during iterations through +/// the `dict.items()`. /// /// ## 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 @@ -40,102 +42,7 @@ impl AlwaysFixableViolation for UnnecessaryDictIndexLookup { } fn fix_title(&self) -> String { - format!("Use existing item variable instead") - } -} - -struct SubscriptVisitor<'a> { - dict_name: &'a str, - index_name: &'a str, - diagnostic_ranges: Vec, - is_subcript_modified: bool, -} - -impl<'a> SubscriptVisitor<'a> { - fn new(dict_name: &'a str, index_name: &'a str) -> Self { - Self { - dict_name, - index_name, - diagnostic_ranges: Vec::new(), - is_subcript_modified: false, - } - } -} - -fn check_target_for_assignment(expr: &Expr, dict_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, .. }) => { - let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { - return false; - }; - if id == dict_name { - let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { - return false; - }; - 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, - slice, - range, - .. - }) => { - let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { - return; - }; - if id == self.dict_name { - let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { - return; - }; - if id == self.index_name { - self.diagnostic_ranges.push(*range); - } - } - } - _ => visitor::walk_expr(self, expr), - } - } - - fn visit_stmt(&mut self, stmt: &Stmt) { - if self.is_subcript_modified { - return; - } - match stmt { - Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { - self.is_subcript_modified = targets.iter().any(|target| { - check_target_for_assignment(target, self.dict_name, self.index_name) - }); - self.visit_expr(value); - } - Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { - if let Some(value) = value { - self.is_subcript_modified = - check_target_for_assignment(target, self.dict_name, self.index_name); - self.visit_expr(value); - } - } - Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { - self.is_subcript_modified = - check_target_for_assignment(target, self.dict_name, self.index_name); - self.visit_expr(value); - } - _ => visitor::walk_stmt(self, stmt), - } + format!("Use existing variable") } } @@ -146,7 +53,7 @@ pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &St return; }; - let mut visitor = SubscriptVisitor::new(&dict_name, &index_name); + let mut visitor = SubscriptVisitor::new(dict_name, index_name); visitor.visit_body(&stmt_for.body); visitor.visit_body(&stmt_for.orelse); @@ -155,7 +62,7 @@ pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &St let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range); diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - value_name.clone(), + value_name.to_string(), range, ))); @@ -165,52 +72,53 @@ pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &St /// PLR1733 pub(crate) fn unnecessary_dict_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 { - let Some((dict_name, index_name, value_name)) = - dict_items(&comp.iter, &comp.target) - else { - continue; - }; + let (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, .. + })) = expr + else { + return; + }; - let mut visitor = SubscriptVisitor::new(&dict_name, &index_name); + for comp in generators { + let Some((dict_name, index_name, value_name)) = dict_items(&comp.iter, &comp.target) else { + continue; + }; - visitor.visit_expr(elt.as_ref()); - for expr in &comp.ifs { - visitor.visit_expr(expr); - } + let mut visitor = SubscriptVisitor::new(dict_name, index_name); - for range in visitor.diagnostic_ranges { - let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range); + visitor.visit_expr(elt.as_ref()); + for expr in &comp.ifs { + visitor.visit_expr(expr); + } - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - value_name.clone(), - range, - ))); + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range); - checker.diagnostics.push(diagnostic); - } - } + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.to_string(), + range, + ))); + + checker.diagnostics.push(diagnostic); } - _ => (), } } -fn dict_items(call_expr: &Expr, tuple_expr: &Expr) -> Option<(String, String, String)> { +fn dict_items<'a>( + call_expr: &'a Expr, + tuple_expr: &'a Expr, +) -> Option<(&'a str, &'a str, &'a str)> { let ast::ExprCall { func, arguments, .. } = call_expr.as_call_expr()?; @@ -250,5 +158,98 @@ fn dict_items(call_expr: &Expr, tuple_expr: &Expr) -> Option<(String, String, St return None; } - Some((dict_name.clone(), index_name.clone(), value_name.clone())) + Some((dict_name, index_name, value_name)) +} + +#[derive(Debug)] +struct SubscriptVisitor<'a> { + dict_name: &'a str, + index_name: &'a str, + diagnostic_ranges: Vec, + modified: bool, +} + +impl<'a> SubscriptVisitor<'a> { + fn new(dict_name: &'a str, index_name: &'a str) -> Self { + Self { + dict_name, + index_name, + diagnostic_ranges: Vec::new(), + modified: false, + } + } +} + +impl SubscriptVisitor<'_> { + fn check_target(&self, expr: &Expr) -> bool { + match expr { + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return false; + }; + if id == self.dict_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return false; + }; + if id == self.index_name { + return true; + } + } + false + } + _ => false, + } + } +} + +impl<'a> Visitor<'_> for SubscriptVisitor<'a> { + fn visit_stmt(&mut self, stmt: &Stmt) { + if self.modified { + return; + } + match stmt { + Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { + self.modified = targets.iter().any(|target| self.check_target(target)); + self.visit_expr(value); + } + Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { + if let Some(value) = value { + self.modified = self.check_target(target); + self.visit_expr(value); + } + } + Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { + self.modified = self.check_target(target); + self.visit_expr(value); + } + _ => visitor::walk_stmt(self, stmt), + } + } + + fn visit_expr(&mut self, expr: &Expr) { + if self.modified { + return; + } + match expr { + Expr::Subscript(ast::ExprSubscript { + value, + slice, + range, + .. + }) => { + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return; + }; + if id == self.dict_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return; + }; + if id == self.index_name { + self.diagnostic_ranges.push(*range); + } + } + } + _ => visitor::walk_expr(self, expr), + } + } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap index aeaad23437783..9a98058dc6a7a 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1733_unnecessary_dict_index_lookup.py.snap @@ -9,7 +9,7 @@ unnecessary_dict_index_lookup.py:4:6: PLR1733 [*] Unnecessary lookup of dict ite 5 | {FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 | - = help: Use existing item variable instead + = help: Use existing variable ℹ Safe fix 1 1 | FRUITS = {"apple": 1, "orange": 10, "berry": 22} @@ -29,7 +29,7 @@ unnecessary_dict_index_lookup.py:5:6: PLR1733 [*] Unnecessary lookup of dict ite | ^^^^^^^^^^^^^^^^^^ PLR1733 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 | - = help: Use existing item variable instead + = help: Use existing variable ℹ Safe fix 2 2 | @@ -50,7 +50,7 @@ unnecessary_dict_index_lookup.py:6:18: PLR1733 [*] Unnecessary lookup of dict it 7 | 8 | for fruit_name, fruit_count in FRUITS.items(): | - = help: Use existing item variable instead + = help: Use existing variable ℹ Safe fix 3 3 | def fix_these(): @@ -70,7 +70,7 @@ unnecessary_dict_index_lookup.py:9:15: PLR1733 [*] Unnecessary lookup of dict it 10 | blah = FRUITS[fruit_name] # PLR1733 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | - = help: Use existing item variable instead + = help: Use existing variable ℹ Safe fix 6 6 | {fruit_name: FRUITS[fruit_name] for fruit_name, fruit_count in FRUITS.items()} # PLR1733 @@ -90,7 +90,7 @@ unnecessary_dict_index_lookup.py:10:16: PLR1733 [*] Unnecessary lookup of dict i | ^^^^^^^^^^^^^^^^^^ PLR1733 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | - = help: Use existing item variable instead + = help: Use existing variable ℹ Safe fix 7 7 | @@ -109,7 +109,7 @@ unnecessary_dict_index_lookup.py:11:16: PLR1733 [*] Unnecessary lookup of dict i 11 | assert FRUITS[fruit_name] == "pear" # PLR1733 | ^^^^^^^^^^^^^^^^^^ PLR1733 | - = help: Use existing item variable instead + = help: Use existing variable ℹ Safe fix 8 8 | for fruit_name, fruit_count in FRUITS.items(): From dac65ec0209deff0415249cd32cf534985f8886d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 1 Dec 2023 00:02:15 -0500 Subject: [PATCH 12/12] Tweaks --- .../rules/unnecessary_dict_index_lookup.rs | 36 +++++++++---------- .../rules/unnecessary_list_index_lookup.rs | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs index e623f6797ed6d..a77843f052fa3 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs @@ -53,19 +53,19 @@ pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &St return; }; - let mut visitor = SubscriptVisitor::new(dict_name, index_name); - - visitor.visit_body(&stmt_for.body); - visitor.visit_body(&stmt_for.orelse); + let ranges = { + let mut visitor = SubscriptVisitor::new(dict_name, index_name); + visitor.visit_body(&stmt_for.body); + visitor.visit_body(&stmt_for.orelse); + visitor.diagnostic_ranges + }; - for range in visitor.diagnostic_ranges { + for range in ranges { let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( value_name.to_string(), range, ))); - checker.diagnostics.push(diagnostic); } } @@ -95,21 +95,21 @@ pub(crate) fn unnecessary_dict_index_lookup_comprehension(checker: &mut Checker, continue; }; - let mut visitor = SubscriptVisitor::new(dict_name, index_name); - - visitor.visit_expr(elt.as_ref()); - for expr in &comp.ifs { - visitor.visit_expr(expr); - } + let ranges = { + let mut visitor = SubscriptVisitor::new(dict_name, index_name); + visitor.visit_expr(elt.as_ref()); + for expr in &comp.ifs { + visitor.visit_expr(expr); + } + visitor.diagnostic_ranges + }; - for range in visitor.diagnostic_ranges { + for range in ranges { let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( value_name.to_string(), range, ))); - checker.diagnostics.push(diagnostic); } } @@ -144,7 +144,7 @@ fn dict_items<'a>( return None; }; - // Grab the variable names + // Grab the variable names. let Expr::Name(ast::ExprName { id: index_name, .. }) = index else { return None; }; @@ -153,7 +153,7 @@ fn dict_items<'a>( return None; }; - // If either of the variable names are intentionally ignored by naming them `_`, then don't emit + // If either of the variable names are intentionally ignored by naming them `_`, then don't emit. if index_name == "_" || value_name == "_" { return None; } 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 a668812ff0fd1..9a52352ce0ab4 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 @@ -266,7 +266,7 @@ fn enumerate_items( return None; }; - // If either of the variable names are intentionally ignored by naming them `_`, then don't emit + // If either of the variable names are intentionally ignored by naming them `_`, then don't emit. if index_name == "_" || value_name == "_" { return None; }