From f2d1f1eceb25f146ae07b0045de2533ef7702446 Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 19 Oct 2023 21:01:39 -0400 Subject: [PATCH] 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 215e835cddd5bf..b7522f4098910e 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 13ad26bed79338..c8e80be6677051 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1295,6 +1295,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, @@ -1319,6 +1322,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, @@ -1342,6 +1348,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, @@ -1366,6 +1375,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 87b2709d0e45e0..ea88da604cff50 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 b65f4bb825e0cc..82d7f875bc172b 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():