From 20ab14e3548ffcc2da2dd35562a1365145fb9f87 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 2 Dec 2023 13:17:17 -0500 Subject: [PATCH] Avoid unnecessary index diagnostics when value is modified (#8970) Closes https://github.com/astral-sh/ruff/issues/8969. --- .../pylint/unnecessary_dict_index_lookup.py | 27 ++-- .../pylint/unnecessary_list_index_lookup.py | 49 ++++---- .../ruff_linter/src/rules/pylint/helpers.rs | 117 +++++++++++++++++- .../rules/unnecessary_dict_index_lookup.rs | 106 +--------------- .../rules/unnecessary_list_index_lookup.rs | 115 +---------------- ...1736_unnecessary_list_index_lookup.py.snap | 12 +- 6 files changed, 180 insertions(+), 246 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 cfdd9fc42a..d3daeb83c9 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 @@ -14,16 +14,27 @@ def fix_these(): 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 + FRUITS[fruit_name] = 0 # OK + assert FRUITS[fruit_name] == 0 # OK + + # once there is an assignment to the key, we stop emitting diagnostics + for fruit_name, fruit_count in FRUITS.items(): + fruit_name = 0 # OK + assert FRUITS[fruit_name] == 0 # OK + + # once there is an assignment to the value, we stop emitting diagnostics + for fruit_name, fruit_count in FRUITS.items(): + if fruit_count < 5: + fruit_count = -fruit_count + assert FRUITS[fruit_name] == 0 # OK def value_intentionally_unused(): - [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 + [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 - blah = FRUITS[fruit_name] # Ok - assert FRUITS[fruit_name] == "pear" # Ok + print(FRUITS[fruit_name]) # OK + blah = FRUITS[fruit_name] # OK + assert FRUITS[fruit_name] == "pear" # OK 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 182e63cb7b..8911c8bd26 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 @@ -12,7 +12,7 @@ def fix_these(): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 assert letters[index] == "d" # PLR1736 - + for index, letter in builtins.enumerate(letters): print(letters[index]) # PLR1736 blah = letters[index] # PLR1736 @@ -22,38 +22,43 @@ def fix_these(): def dont_fix_these(): # once there is an assignment to the sequence[index], we stop emitting diagnostics for index, letter in enumerate(letters): - letters[index] = "d" # Ok - letters[index] += "e" # Ok - assert letters[index] == "de" # Ok - + letters[index] = "d" # OK + letters[index] += "e" # OK + assert letters[index] == "de" # OK + # once there is an assignment to the index, we stop emitting diagnostics for index, letter in enumerate(letters): - index += 1 # Ok - print(letters[index]) # Ok - + index += 1 # OK + print(letters[index]) # OK + # once there is an assignment to the sequence, we stop emitting diagnostics for index, letter in enumerate(letters): - letters = ["d", "e", "f"] # Ok - print(letters[index]) # Ok + letters = ["d", "e", "f"] # OK + print(letters[index]) # OK + + # once there is an assignment to the value, we stop emitting diagnostics + for index, letter in enumerate(letters): + letter = "d" + print(letters[index]) # OK # once there is an deletion from or of the sequence or index, we stop emitting diagnostics for index, letter in enumerate(letters): - del letters[index] # Ok - print(letters[index]) # Ok + del letters[index] # OK + print(letters[index]) # OK for index, letter in enumerate(letters): - del letters # Ok - print(letters[index]) # Ok + del letters # OK + print(letters[index]) # OK for index, letter in enumerate(letters): - del index # Ok - print(letters[index]) # Ok + del index # OK + print(letters[index]) # OK def value_intentionally_unused(): - [letters[index] for index, _ in enumerate(letters)] # Ok - {letters[index] for index, _ in enumerate(letters)} # Ok - {index: letters[index] for index, _ in enumerate(letters)} # Ok + [letters[index] for index, _ in enumerate(letters)] # OK + {letters[index] for index, _ in enumerate(letters)} # OK + {index: letters[index] for index, _ in enumerate(letters)} # OK for index, _ in enumerate(letters): - print(letters[index]) # Ok - blah = letters[index] # Ok - letters[index] = "d" # Ok + print(letters[index]) # OK + blah = letters[index] # OK + letters[index] = "d" # OK diff --git a/crates/ruff_linter/src/rules/pylint/helpers.rs b/crates/ruff_linter/src/rules/pylint/helpers.rs index 027272d03c..d68e639b59 100644 --- a/crates/ruff_linter/src/rules/pylint/helpers.rs +++ b/crates/ruff_linter/src/rules/pylint/helpers.rs @@ -1,9 +1,11 @@ use std::fmt; use ruff_python_ast as ast; -use ruff_python_ast::{Arguments, CmpOp, Expr}; +use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{visitor, Arguments, CmpOp, Expr, Stmt}; use ruff_python_semantic::analyze::function_type; use ruff_python_semantic::{ScopeKind, SemanticModel}; +use ruff_text_size::TextRange; use crate::settings::LinterSettings; @@ -82,3 +84,116 @@ impl fmt::Display for CmpOpExt { write!(f, "{representation}") } } + +/// Visitor to track reads from an iterable in a loop. +#[derive(Debug)] +pub(crate) struct SequenceIndexVisitor<'a> { + /// `letters`, given `for index, letter in enumerate(letters)`. + sequence_name: &'a str, + /// `index`, given `for index, letter in enumerate(letters)`. + index_name: &'a str, + /// `letter`, given `for index, letter in enumerate(letters)`. + value_name: &'a str, + /// The ranges of any `letters[index]` accesses. + accesses: Vec, + /// Whether any of the variables have been modified. + modified: bool, +} + +impl<'a> SequenceIndexVisitor<'a> { + pub(crate) fn new(sequence_name: &'a str, index_name: &'a str, value_name: &'a str) -> Self { + Self { + sequence_name, + index_name, + value_name, + accesses: Vec::new(), + modified: false, + } + } + + pub(crate) fn into_accesses(self) -> Vec { + self.accesses + } +} + +impl SequenceIndexVisitor<'_> { + fn is_assignment(&self, expr: &Expr) -> bool { + // If we see the sequence, a subscript, or the index being modified, we'll stop emitting + // diagnostics. + match expr { + Expr::Name(ast::ExprName { id, .. }) => { + id == self.sequence_name || id == self.index_name || id == self.value_name + } + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return false; + }; + if id == self.sequence_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return false; + }; + if id == self.index_name { + return true; + } + } + false + } + _ => false, + } + } +} + +impl<'a> Visitor<'_> for SequenceIndexVisitor<'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.is_assignment(target)); + self.visit_expr(value); + } + Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { + if let Some(value) = value { + self.modified = self.is_assignment(target); + self.visit_expr(value); + } + } + Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { + self.modified = self.is_assignment(target); + self.visit_expr(value); + } + Stmt::Delete(ast::StmtDelete { targets, .. }) => { + self.modified = targets.iter().any(|target| self.is_assignment(target)); + } + _ => 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.sequence_name { + let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { + return; + }; + if id == self.index_name { + self.accesses.push(*range); + } + } + } + _ => visitor::walk_expr(self, 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 16dc733d61..88a842b7d6 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,13 +1,10 @@ -use ast::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 ruff_python_ast::{self as ast, Expr, StmtFor}; use crate::checkers::ast::Checker; +use crate::rules::pylint::helpers::SequenceIndexVisitor; /// ## What it does /// Checks for key-based dict accesses during `.items()` iterations. @@ -54,10 +51,10 @@ pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &St }; let ranges = { - let mut visitor = SubscriptVisitor::new(dict_name, index_name); + let mut visitor = SequenceIndexVisitor::new(dict_name, index_name, value_name); visitor.visit_body(&stmt_for.body); visitor.visit_body(&stmt_for.orelse); - visitor.diagnostic_ranges + visitor.into_accesses() }; for range in ranges { @@ -96,12 +93,12 @@ pub(crate) fn unnecessary_dict_index_lookup_comprehension(checker: &mut Checker, }; let ranges = { - let mut visitor = SubscriptVisitor::new(dict_name, index_name); + let mut visitor = SequenceIndexVisitor::new(dict_name, index_name, value_name); visitor.visit_expr(elt.as_ref()); for expr in &comp.ifs { visitor.visit_expr(expr); } - visitor.diagnostic_ranges + visitor.into_accesses() }; for range in ranges { @@ -161,94 +158,3 @@ fn dict_items<'a>( 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 is_assignment(&self, expr: &Expr) -> bool { - let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr else { - return false; - }; - 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 - } -} - -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.is_assignment(target)); - self.visit_expr(value); - } - Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { - if let Some(value) = value { - self.modified = self.is_assignment(target); - self.visit_expr(value); - } - } - Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { - self.modified = self.is_assignment(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/rules/unnecessary_list_index_lookup.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs index 17a42b29e2..920d2442e9 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs @@ -1,13 +1,11 @@ -use ruff_python_ast::{self as ast, Expr, Stmt, 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_python_ast::{self as ast, Expr, StmtFor}; use ruff_python_semantic::SemanticModel; -use ruff_text_size::TextRange; use crate::checkers::ast::Checker; +use crate::rules::pylint::helpers::SequenceIndexVisitor; /// ## What it does /// Checks for index-based list accesses during `enumerate` iterations. @@ -55,10 +53,10 @@ pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &St }; let ranges = { - let mut visitor = SubscriptVisitor::new(sequence, index_name); + let mut visitor = SequenceIndexVisitor::new(sequence, index_name, value_name); visitor.visit_body(&stmt_for.body); visitor.visit_body(&stmt_for.orelse); - visitor.diagnostic_ranges + visitor.into_accesses() }; for range in ranges { @@ -99,9 +97,9 @@ pub(crate) fn unnecessary_list_index_lookup_comprehension(checker: &mut Checker, }; let ranges = { - let mut visitor = SubscriptVisitor::new(sequence, index_name); + let mut visitor = SequenceIndexVisitor::new(sequence, index_name, value_name); visitor.visit_expr(elt.as_ref()); - visitor.diagnostic_ranges + visitor.into_accesses() }; for range in ranges { @@ -161,104 +159,3 @@ fn enumerate_items<'a>( Some((sequence, index_name, value_name)) } - -#[derive(Debug)] -struct SubscriptVisitor<'a> { - sequence_name: &'a str, - index_name: &'a str, - diagnostic_ranges: Vec, - modified: bool, -} - -impl<'a> SubscriptVisitor<'a> { - fn new(sequence_name: &'a str, index_name: &'a str) -> Self { - Self { - sequence_name, - index_name, - diagnostic_ranges: Vec::new(), - modified: false, - } - } -} - -impl SubscriptVisitor<'_> { - fn is_assignment(&self, expr: &Expr) -> bool { - // If we see the sequence, a subscript, or the index being modified, we'll stop emitting - // diagnostics. - match expr { - Expr::Name(ast::ExprName { id, .. }) => { - id == self.sequence_name || id == self.index_name - } - Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { - let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { - return false; - }; - if id == self.sequence_name { - let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() else { - return false; - }; - if id == self.index_name { - return true; - } - } - false - } - _ => 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.is_assignment(target)); - self.visit_expr(value); - } - Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { - if let Some(value) = value { - self.modified = self.is_assignment(target); - self.visit_expr(value); - } - } - Stmt::AugAssign(ast::StmtAugAssign { target, value, .. }) => { - self.modified = self.is_assignment(target); - self.visit_expr(value); - } - Stmt::Delete(ast::StmtDelete { targets, .. }) => { - self.modified = targets.iter().any(|target| self.is_assignment(target)); - } - _ => 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.sequence_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__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 8e4d22472d..880422eab6 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 @@ -80,7 +80,7 @@ unnecessary_list_index_lookup.py:12:15: PLR1736 [*] Unnecessary lookup of list i 12 |+ print(letter) # PLR1736 13 13 | blah = letters[index] # PLR1736 14 14 | assert letters[index] == "d" # PLR1736 -15 15 | +15 15 | unnecessary_list_index_lookup.py:13:16: PLR1736 [*] Unnecessary lookup of list item by index | @@ -99,7 +99,7 @@ unnecessary_list_index_lookup.py:13:16: PLR1736 [*] Unnecessary lookup of list i 13 |- blah = letters[index] # PLR1736 13 |+ blah = letter # PLR1736 14 14 | assert letters[index] == "d" # PLR1736 -15 15 | +15 15 | 16 16 | for index, letter in builtins.enumerate(letters): unnecessary_list_index_lookup.py:14:16: PLR1736 [*] Unnecessary lookup of list item by index @@ -108,7 +108,7 @@ unnecessary_list_index_lookup.py:14:16: PLR1736 [*] Unnecessary lookup of list i 13 | blah = letters[index] # PLR1736 14 | assert letters[index] == "d" # PLR1736 | ^^^^^^^^^^^^^^ PLR1736 -15 | +15 | 16 | for index, letter in builtins.enumerate(letters): | = help: Use existing variable @@ -119,7 +119,7 @@ unnecessary_list_index_lookup.py:14:16: PLR1736 [*] Unnecessary lookup of list i 13 13 | blah = letters[index] # PLR1736 14 |- assert letters[index] == "d" # PLR1736 14 |+ assert letter == "d" # PLR1736 -15 15 | +15 15 | 16 16 | for index, letter in builtins.enumerate(letters): 17 17 | print(letters[index]) # PLR1736 @@ -135,7 +135,7 @@ unnecessary_list_index_lookup.py:17:15: PLR1736 [*] Unnecessary lookup of list i ℹ Safe fix 14 14 | assert letters[index] == "d" # PLR1736 -15 15 | +15 15 | 16 16 | for index, letter in builtins.enumerate(letters): 17 |- print(letters[index]) # PLR1736 17 |+ print(letter) # PLR1736 @@ -154,7 +154,7 @@ unnecessary_list_index_lookup.py:18:16: PLR1736 [*] Unnecessary lookup of list i = help: Use existing variable ℹ Safe fix -15 15 | +15 15 | 16 16 | for index, letter in builtins.enumerate(letters): 17 17 | print(letters[index]) # PLR1736 18 |- blah = letters[index] # PLR1736