Cyclic0007/ruff
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Avoid unnecessary index diagnostics when value is modified (#8970)

Browse Source 
Closes https://github.com/astral-sh/ruff/issues/8969.
This commit is contained in:
Charlie Marsh
2023-12-02 13:17:17 -05:00
committed by GitHub
parent 22d8a989d4
commit 20ab14e354
6 changed files with 180 additions and 246 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py vendored
View File

@@ -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

49
crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py vendored
View File

@@ -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

117
crates/ruff_linter/src/rules/pylint/helpers.rs
View File

@@ -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<TextRange>,
/// 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<TextRange> {
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),
}
}
}

106
crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs
View File

@@ -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<TextRange>,
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),
}
}
}

115
crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs
View File

@@ -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<TextRange>,
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),
}
}
}

12
crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1736_unnecessary_list_index_lookup.py.snap
View File

@@ -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