Skip to content

Commit

Permalink
Avoid unnecessary index diagnostics when value is modified (#8970)
Browse files Browse the repository at this point in the history 
Closes #8969.
  • Loading branch information
@charliermarsh
charliermarsh authored Dec 2, 2023
1 parent 22d8a98 commit 20ab14e
Show file tree
Hide file tree
Showing 6 changed files with 180 additions and 246 deletions.
27 changes: 19 additions & 8 deletions crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 changes: 27 additions & 22 deletions crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_list_index_lookup.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 changes: 116 additions & 1 deletion crates/ruff_linter/src/rules/pylint/helpers.rs
View file
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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 changes: 6 additions & 100 deletions crates/ruff_linter/src/rules/pylint/rules/unnecessary_dict_index_lookup.rs
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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),
}
}
}
Loading

0 comments on commit 20ab14e

Please sign in to comment.