Skip to content

Commit

Permalink
add PLR1736 and autofix
Browse files Browse the repository at this point in the history
  • Loading branch information
diceroll123 committed Oct 17, 2023
1 parent 4113d65 commit 7cff2fb
Show file tree
Hide file tree
Showing 8 changed files with 196 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
letters = ["a", "b", "c"]

for index, letter in enumerate(letters):
print(letters[index]) # PLR1736
blah = letters[index] # PLR1736
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(checker, iter);
}
if checker.enabled(Rule::UnnecessaryListIndexLookup) {
pylint::rules::unnecessary_list_index_lookup(checker, for_stmt);
}
if !is_async {
if checker.enabled(Rule::ReimplementedBuiltin) {
flake8_simplify::rules::convert_for_loop_to_any_all(checker, stmt);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),
(Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
(Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary),
(Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup),
(Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias),
(Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison),
(Pylint, "R5501") => (RuleGroup::Stable, rules::pylint::rules::CollapsibleElseIf),
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ mod tests {
)]
#[test_case(Rule::BadDunderMethodName, Path::new("bad_dunder_method_name.py"))]
#[test_case(Rule::NoSelfUse, Path::new("no_self_use.py"))]
#[test_case(
Rule::UnnecessaryListIndexLookup,
Path::new("unnecessary_list_index_lookup.py")
)]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ pub(crate) use type_name_incorrect_variance::*;
pub(crate) use type_param_name_mismatch::*;
pub(crate) use unexpected_special_method_signature::*;
pub(crate) use unnecessary_direct_lambda_call::*;
pub(crate) use unnecessary_list_index_lookup::*;
pub(crate) use useless_else_on_loop::*;
pub(crate) use useless_import_alias::*;
pub(crate) use useless_return::*;
Expand Down Expand Up @@ -112,6 +113,7 @@ mod type_name_incorrect_variance;
mod type_param_name_mismatch;
mod unexpected_special_method_signature;
mod unnecessary_direct_lambda_call;
mod unnecessary_list_index_lookup;
mod useless_else_on_loop;
mod useless_import_alias;
mod useless_return;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
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 crate::checkers::ast::Checker;

/// ## What it does
/// Checks for uses of enumeration and accessing the value by index lookup.
///
/// ## Why is this bad?
/// The value is already accessible by the 2nd variable from the enumeration.
///
/// ## Example
/// ```python
/// letters = ["a", "b", "c"]
///
/// for index, letter in enumerate(letters):
/// print(letters[index])
/// ```
///
/// Use instead:
/// ```python
/// letters = ["a", "b", "c"]
///
/// for index, letter in enumerate(letters):
/// print(letter)
/// ```
#[violation]
pub struct UnnecessaryListIndexLookup;

impl AlwaysFixableViolation for UnnecessaryListIndexLookup {
#[derive_message_formats]
fn message(&self) -> String {
format!("Unnecessary list index lookup")
}

fn fix_title(&self) -> String {
format!("Remove unnecessary list index lookup")
}
}

struct SubscriptVisitor<'a> {
sequence_name: &'a str,
index_name: &'a str,
diagnostic_ranges: Vec<TextRange>,
}

impl<'a> SubscriptVisitor<'a> {
fn new(sequence_name: &'a str, index_name: &'a str) -> Self {
Self {
sequence_name,
index_name,
diagnostic_ranges: Vec::new(),
}
}
}

impl<'a> Visitor<'_> for SubscriptVisitor<'a> {
fn visit_expr(&mut self, expr: &Expr) {
match expr {
Expr::Subscript(ast::ExprSubscript {
value,
slice,
range,
..
}) => {
if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() {
if id == self.sequence_name {
if let Expr::Name(ast::ExprName { id, .. }) = slice.as_ref() {
if id == self.index_name {
self.diagnostic_ranges.push(*range);
}
}
}
}
}
_ => visitor::walk_expr(self, expr),
}
}
}

/// PLR1736
pub(crate) fn unnecessary_list_index_lookup(checker: &mut Checker, stmt_for: &StmtFor) {
let Expr::Call(ast::ExprCall {
func, arguments, ..
}) = stmt_for.iter.as_ref()
else {
return;
};

// Check that the function is the `enumerate` builtin.
let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else {
return;
};
if id != "enumerate" {
return;
};
if !checker.semantic().is_builtin("enumerate") {
return;
};

let Expr::Tuple(ast::ExprTuple { elts, .. }) = stmt_for.target.as_ref() else {
return;
};
let [index, value] = elts.as_slice() else {
return;
};

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

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

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

let mut visitor = SubscriptVisitor::new(sequence, index_name);

visitor.visit_body(&stmt_for.body);
visitor.visit_body(&stmt_for.orelse);

for range in visitor.diagnostic_ranges {
let mut diagnostic = Diagnostic::new(UnnecessaryListIndexLookup, range);

diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
value_name.clone(),
range,
)));

checker.diagnostics.push(diagnostic);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
unnecessary_list_index_lookup.py:4:11: PLR1736 [*] Unnecessary list index lookup
|
3 | for index, letter in enumerate(letters):
4 | print(letters[index]) # PLR1736
| ^^^^^^^^^^^^^^ PLR1736
5 | blah = letters[index] # PLR1736
|
= help: Remove unnecessary list index lookup

Fix
1 1 | letters = ["a", "b", "c"]
2 2 |
3 3 | for index, letter in enumerate(letters):
4 |- print(letters[index]) # PLR1736
4 |+ print(letter) # PLR1736
5 5 | blah = letters[index] # PLR1736

unnecessary_list_index_lookup.py:5:12: PLR1736 [*] Unnecessary list index lookup
|
3 | for index, letter in enumerate(letters):
4 | print(letters[index]) # PLR1736
5 | blah = letters[index] # PLR1736
| ^^^^^^^^^^^^^^ PLR1736
|
= help: Remove unnecessary list index lookup

Fix
2 2 |
3 3 | for index, letter in enumerate(letters):
4 4 | print(letters[index]) # PLR1736
5 |- blah = letters[index] # PLR1736
5 |+ blah = letter # PLR1736


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 7cff2fb

Please sign in to comment.