Skip to content

Commit

Permalink
Apply unnecessary index rule prior to enumerate rewrite (#9012)
Browse files Browse the repository at this point in the history 
This PR adds synthetic edits to `PLR1736` to avoid removing the
referenced value as part of `FURB148`.

Closes #9010.
  • Loading branch information
@charliermarsh
charliermarsh committed Dec 5, 2023
1 parent 3def18f commit 268d95e
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 30 deletions.
37 changes: 22 additions & 15 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
Expand Up @@ -2,6 +2,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, Expr, StmtFor};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::rules::pylint::helpers::SequenceIndexVisitor;
Expand Down Expand Up @@ -51,18 +52,18 @@ pub(crate) fn unnecessary_dict_index_lookup(checker: &mut Checker, stmt_for: &St
};

let ranges = {
let mut visitor = SequenceIndexVisitor::new(dict_name, index_name, value_name);
let mut visitor = SequenceIndexVisitor::new(&dict_name.id, &index_name.id, &value_name.id);
visitor.visit_body(&stmt_for.body);
visitor.visit_body(&stmt_for.orelse);
visitor.into_accesses()
};

for range in ranges {
let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
value_name.to_string(),
range,
)));
diagnostic.set_fix(Fix::safe_edits(
Edit::range_replacement(value_name.id.to_string(), range),
[noop(index_name), noop(value_name)],
));
checker.diagnostics.push(diagnostic);
}
}
Expand Down Expand Up @@ -93,7 +94,8 @@ pub(crate) fn unnecessary_dict_index_lookup_comprehension(checker: &mut Checker,
};

let ranges = {
let mut visitor = SequenceIndexVisitor::new(dict_name, index_name, value_name);
let mut visitor =
SequenceIndexVisitor::new(&dict_name.id, &index_name.id, &value_name.id);
visitor.visit_expr(elt.as_ref());
for expr in &comp.ifs {
visitor.visit_expr(expr);
Expand All @@ -103,10 +105,10 @@ pub(crate) fn unnecessary_dict_index_lookup_comprehension(checker: &mut Checker,

for range in ranges {
let mut diagnostic = Diagnostic::new(UnnecessaryDictIndexLookup, range);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
value_name.to_string(),
range,
)));
diagnostic.set_fix(Fix::safe_edits(
Edit::range_replacement(value_name.id.to_string(), range),
[noop(index_name), noop(value_name)],
));
checker.diagnostics.push(diagnostic);
}
}
Expand All @@ -115,7 +117,7 @@ pub(crate) fn unnecessary_dict_index_lookup_comprehension(checker: &mut Checker,
fn dict_items<'a>(
call_expr: &'a Expr,
tuple_expr: &'a Expr,
) -> Option<(&'a str, &'a str, &'a str)> {
) -> Option<(&'a ast::ExprName, &'a ast::ExprName, &'a ast::ExprName)> {
let ast::ExprCall {
func, arguments, ..
} = call_expr.as_call_expr()?;
Expand All @@ -130,7 +132,7 @@ fn dict_items<'a>(
return None;
}

let Expr::Name(ast::ExprName { id: dict_name, .. }) = value.as_ref() else {
let Expr::Name(dict_name) = value.as_ref() else {
return None;
};

Expand All @@ -142,19 +144,24 @@ fn dict_items<'a>(
};

// Grab the variable names.
let Expr::Name(ast::ExprName { id: index_name, .. }) = index else {
let Expr::Name(index_name) = index else {
return None;
};

let Expr::Name(ast::ExprName { id: value_name, .. }) = value else {
let Expr::Name(value_name) = value else {
return None;
};

// If either of the variable names are intentionally ignored by naming them `_`, then don't
// emit.
if index_name == "_" || value_name == "_" {
if index_name.id == "_" || value_name.id == "_" {
return None;
}

Some((dict_name, index_name, value_name))
}

/// Return a no-op edit for the given name.
fn noop(name: &ast::ExprName) -> Edit {
Edit::range_replacement(name.id.to_string(), name.range())
}
37 changes: 22 additions & 15 deletions crates/ruff_linter/src/rules/pylint/rules/unnecessary_list_index_lookup.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, Expr, StmtFor};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::rules::pylint::helpers::SequenceIndexVisitor;
Expand Down Expand Up @@ -53,18 +54,18 @@ pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &St
};

let ranges = {
let mut visitor = SequenceIndexVisitor::new(sequence, index_name, value_name);
let mut visitor = SequenceIndexVisitor::new(&sequence.id, &index_name.id, &value_name.id);
visitor.visit_body(&stmt_for.body);
visitor.visit_body(&stmt_for.orelse);
visitor.into_accesses()
};

for range in ranges {
let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
value_name.to_string(),
range,
)));
diagnostic.set_fix(Fix::safe_edits(
Edit::range_replacement(value_name.id.to_string(), range),
[noop(index_name), noop(value_name)],
));
checker.diagnostics.push(diagnostic);
}
}
Expand Down Expand Up @@ -97,17 +98,18 @@ pub(crate) fn unnecessary_list_index_lookup_comprehension(checker: &mut Checker,
};

let ranges = {
let mut visitor = SequenceIndexVisitor::new(sequence, index_name, value_name);
let mut visitor =
SequenceIndexVisitor::new(&sequence.id, &index_name.id, &value_name.id);
visitor.visit_expr(elt.as_ref());
visitor.into_accesses()
};

for range in ranges {
let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
value_name.to_string(),
range,
)));
diagnostic.set_fix(Fix::safe_edits(
Edit::range_replacement(value_name.id.to_string(), range),
[noop(index_name), noop(value_name)],
));
checker.diagnostics.push(diagnostic);
}
}
Expand All @@ -117,7 +119,7 @@ fn enumerate_items<'a>(
call_expr: &'a Expr,
tuple_expr: &'a Expr,
semantic: &SemanticModel,
) -> Option<(&'a str, &'a str, &'a str)> {
) -> Option<(&'a ast::ExprName, &'a ast::ExprName, &'a ast::ExprName)> {
let ast::ExprCall {
func, arguments, ..
} = call_expr.as_call_expr()?;
Expand All @@ -138,24 +140,29 @@ fn enumerate_items<'a>(
};

// Grab the variable names.
let Expr::Name(ast::ExprName { id: index_name, .. }) = index else {
let Expr::Name(index_name) = index else {
return None;
};

let Expr::Name(ast::ExprName { id: value_name, .. }) = value else {
let Expr::Name(value_name) = value else {
return None;
};

// If either of the variable names are intentionally ignored by naming them `_`, then don't
// emit.
if index_name == "_" || value_name == "_" {
if index_name.id == "_" || value_name.id == "_" {
return None;
}

// Get the first argument of the enumerate call.
let Some(Expr::Name(ast::ExprName { id: sequence, .. })) = arguments.args.first() else {
let Some(Expr::Name(sequence)) = arguments.args.first() else {
return None;
};

Some((sequence, index_name, value_name))
}

/// Return a no-op edit for the given name.
fn noop(name: &ast::ExprName) -> Edit {
Edit::range_replacement(name.id.to_string(), name.range())
}

0 comments on commit 268d95e

Please sign in to comment.