From 592f4e126bc63146b346a8bae37fd8d67611d499 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 16 Oct 2023 21:49:11 -0400 Subject: [PATCH] add PLR1736 and autofix --- .../pylint/unnecessary_list_index_lookup.py | 5 + .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 4 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../rules/unnecessary_list_index_lookup.rs | 121 ++++++++++++++++++ ...1736_unnecessary_list_index_lookup.py.snap | 37 ++++++ ruff.schema.json | 2 + 8 files changed, 175 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py new file mode 100644 index 00000000000000..4ee655227c08fe --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py @@ -0,0 +1,5 @@ +letters = ["a", "b", "c"] + +for index, letter in enumerate(letters): + print(letters[index]) # PLR1736 + blah = letters[index] # PLR1736 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index a39fd0734355c8..97db6ab56327ba 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1226,6 +1226,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryListCast) { perflint::rules::unnecessary_list_cast(checker, iter); } + if checker.enabled(Rule::UnnecessaryListIndexLookup) { + pylint::rules::unnecessary_list_index_lookup(checker, for_stmt); + } if !is_async { if checker.enabled(Rule::ReimplementedBuiltin) { flake8_simplify::rules::convert_for_loop_to_any_all(checker, stmt); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index bd03e13b5a8a3c..19c0352eab7c98 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -250,6 +250,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary), + (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison), (Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 21b0c0250f911f..cf2f2ba4ddc50c 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -135,6 +135,10 @@ mod tests { )] #[test_case(Rule::BadDunderMethodName, Path::new("bad_dunder_method_name.py"))] #[test_case(Rule::NoSelfUse, Path::new("no_self_use.py"))] + #[test_case( + Rule::UnnecessaryListIndexLookup, + Path::new("unnecessary_list_index_lookup.py") + )] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 317657266794bb..286a217779799e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -52,6 +52,7 @@ 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_direct_lambda_call::*; +pub(crate) use unnecessary_list_index_lookup::*; pub(crate) use useless_else_on_loop::*; pub(crate) use useless_import_alias::*; pub(crate) use useless_return::*; @@ -112,6 +113,7 @@ mod type_name_incorrect_variance; mod type_param_name_mismatch; mod unexpected_special_method_signature; mod unnecessary_direct_lambda_call; +mod unnecessary_list_index_lookup; mod useless_else_on_loop; mod useless_import_alias; mod useless_return; diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs new file mode 100644 index 00000000000000..adf7f6535dcaaa --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -0,0 +1,121 @@ +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; + +#[violation] +pub struct UnnecessaryListIndexLookup; + +impl AlwaysFixableViolation for UnnecessaryListIndexLookup { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unnecessary list index lookup") + } + + fn fix_title(&self) -> String { + format!("Remove unnecessary list index lookup") + } +} + +struct SubscriptVisitor<'a> { + sequence_name: &'a str, + index_name: &'a str, + diagnostic_ranges: Vec, +} + +impl<'a> SubscriptVisitor<'a> { + fn new(sequence_name: &'a str, index_name: &'a str) -> Self { + Self { + sequence_name, + index_name, + diagnostic_ranges: Vec::new(), + } + } +} + +impl<'a> Visitor<'_> for SubscriptVisitor<'a> { + fn visit_expr(&mut self, expr: &Expr) { + match expr { + Expr::Subscript(ast::ExprSubscript { + value, + slice, + range, + .. + }) => { + if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() { + if id == self.sequence_name { + if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() { + if id == self.index_name { + self.diagnostic_ranges.push(*range); + } + } + } + } + } + _ => visitor::walk_expr(self, expr), + } + } +} + +/// PLR1736 +pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &StmtFor) { + let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = stmt_for.iter.as_ref() + else { + return; + }; + + // Check that the function is the `enumerate` builtin. + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return; + }; + if id != "enumerate" { + return; + }; + if !checker.semantic().is_builtin("enumerate") { + return; + }; + + let Expr::Tuple(ast::ExprTuple { elts, .. }) = stmt_for.target.as_ref() else { + return; + }; + let [index, value] = elts.as_slice() else { + return; + }; + + // Grab the variable names + let Expr::Name(ast::ExprName { id: index_name, .. }) = index else { + return; + }; + + let Expr::Name(ast::ExprName { id: value_name, .. }) = value else { + return; + }; + + // Get the first argument of the enumerate call + let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else { + return; + }; + + let mut visitor = SubscriptVisitor::new(sequence, index_name); + + visitor.visit_body(&stmt_for.body); + visitor.visit_body(&stmt_for.orelse); + + for range in visitor.diagnostic_ranges { + let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range); + + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + value_name.clone(), + range, + ))); + + checker.diagnostics.push(diagnostic); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap new file mode 100644 index 00000000000000..c898cf48628e24 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap @@ -0,0 +1,37 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +unnecessary_list_index_lookup.py:4:11: PLR1736 [*] Unnecessary list index lookup + | +3 | for index, letter in enumerate(letters): +4 | print(letters[index]) # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 +5 | blah = letters[index] # PLR1736 + | + = help: Remove unnecessary list index lookup + +ℹ Fix +1 1 | letters = ["a", "b", "c"] +2 2 | +3 3 | for index, letter in enumerate(letters): +4 |- print(letters[index]) # PLR1736 + 4 |+ print(letter) # PLR1736 +5 5 | blah = letters[index] # PLR1736 + +unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup + | +3 | for index, letter in enumerate(letters): +4 | print(letters[index]) # PLR1736 +5 | blah = letters[index] # PLR1736 + | ^^^^^^^^^^^^^^ PLR1736 + | + = help: Remove unnecessary list index lookup + +ℹ Fix +2 2 | +3 3 | for index, letter in enumerate(letters): +4 4 | print(letters[index]) # PLR1736 +5 |- blah = letters[index] # PLR1736 + 5 |+ blah = letter # PLR1736 + + diff --git a/ruff.schema.json b/ruff.schema.json index e6f9230a6cc1a4..1c4eaf1ccbc398 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2956,6 +2956,8 @@ "PLR1714", "PLR172", "PLR1722", + "PLR173", + "PLR1736", "PLR2", "PLR20", "PLR200",