Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid unnecessary index diagnostics when value is modified #8970

Merged
merged 1 commit into from
Dec 2, 2023
Merged

Avoid unnecessary index diagnostics when value is modified #8970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading