Skip to content

Commit

Permalink
[refurb] Implement unnecessary-enumerate (FURB148) (#7454)
Browse files Browse the repository at this point in the history
## Summary

Implement
[`no-ignored-enumerate-items`](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/no_ignored_enumerate.py)
as `unnecessary-enumerate` (`FURB148`).

The auto-fix considers if a `start` argument is passed to the
`enumerate()` function. If only the index is used, then the suggested
fix is to pass the `start` value to the `range()` function. So,

```python
for i, _ in enumerate(xs, 1):
    ...
```

becomes

```python
for i in range(1, len(xs)):
    ...
```

If the index is ignored and only the value is ignored, and if a start
value greater than zero is passed to `enumerate()`, the rule doesn't
produce a suggestion. I couldn't find a unanimously accepted best way to
iterate over a collection whilst skipping the first n elements. The rule
still triggers, however.

Related to #1348.

## Test Plan

`cargo test`

---------

Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
  • Loading branch information
3 people committed Sep 19, 2023
1 parent 4c4ecee commit 511cc25
Show file tree
Hide file tree
Showing 12 changed files with 695 additions and 40 deletions.
66 changes: 66 additions & 0 deletions crates/ruff/resources/test/fixtures/refurb/FURB148.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
books = ["Dune", "Foundation", "Neuromancer"]

# Errors
for index, _ in enumerate(books):
print(index)

for index, _ in enumerate(books, start=0):
print(index)

for index, _ in enumerate(books, 0):
print(index)

for index, _ in enumerate(books, start=1):
print(index)

for index, _ in enumerate(books, 1):
print(index)

for index, _ in enumerate(books, start=x):
print(book)

for index, _ in enumerate(books, x):
print(book)

for _, book in enumerate(books):
print(book)

for _, book in enumerate(books, start=0):
print(book)

for _, book in enumerate(books, 0):
print(book)

for _, book in enumerate(books, start=1):
print(book)

for _, book in enumerate(books, 1):
print(book)

for _, book in enumerate(books, start=x):
print(book)

for _, book in enumerate(books, x):
print(book)

for index, (_, _) in enumerate(books):
print(index)

for (_, _), book in enumerate(books):
print(book)

for(index, _)in enumerate(books):
print(index)

for(index), _ in enumerate(books):
print(index)

# OK
for index, book in enumerate(books):
print(index, book)

for index in range(len(books)):
print(index)

for book in books:
print(book)
5 changes: 4 additions & 1 deletion crates/ruff/src/checkers/ast/analyze/deferred_for_loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use ruff_python_ast::Stmt;

use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::rules::{flake8_bugbear, perflint, pyupgrade};
use crate::rules::{flake8_bugbear, perflint, pyupgrade, refurb};

/// Run lint rules over all deferred for-loops in the [`SemanticModel`].
pub(crate) fn deferred_for_loops(checker: &mut Checker) {
Expand All @@ -24,6 +24,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) {
if checker.enabled(Rule::YieldInForLoop) {
pyupgrade::rules::yield_in_for_loop(checker, stmt_for);
}
if checker.enabled(Rule::UnnecessaryEnumerate) {
refurb::rules::unnecessary_enumerate(checker, stmt_for);
}
}
}
}
1 change: 1 addition & 0 deletions crates/ruff/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1190,6 +1190,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.any_enabled(&[
Rule::UnusedLoopControlVariable,
Rule::IncorrectDictIterator,
Rule::UnnecessaryEnumerate,
Rule::YieldInForLoop,
]) {
checker.deferred.for_loops.push(checker.semantic.snapshot());
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Refurb, "132") => (RuleGroup::Nursery, rules::refurb::rules::CheckAndRemoveFromSet),
(Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap),
(Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
(Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),

// flake8-logging
(Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation),
Expand Down
38 changes: 2 additions & 36 deletions crates/ruff/src/rules/perflint/rules/incorrect_dict_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::{Arguments, Expr};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -83,8 +82,8 @@ pub(crate) fn incorrect_dict_iterator(checker: &mut Checker, stmt_for: &ast::Stm
}

match (
is_unused(key, checker.semantic()),
is_unused(value, checker.semantic()),
checker.semantic().is_unused(key),
checker.semantic().is_unused(value),
) {
(true, true) => {
// Both the key and the value are unused.
Expand Down Expand Up @@ -145,36 +144,3 @@ impl fmt::Display for DictSubset {
}
}
}

/// Returns `true` if the given expression is either an unused value or a tuple of unused values.
fn is_unused(expr: &Expr, semantic: &SemanticModel) -> bool {
match expr {
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
elts.iter().all(|expr| is_unused(expr, semantic))
}
Expr::Name(ast::ExprName { id, .. }) => {
// Treat a variable as used if it has any usages, _or_ it's shadowed by another variable
// with usages.
//
// If we don't respect shadowing, we'll incorrectly flag `bar` as unused in:
// ```python
// from random import random
//
// for bar in range(10):
// if random() > 0.5:
// break
// else:
// bar = 1
//
// print(bar)
// ```
let scope = semantic.current_scope();
scope
.get_all(id)
.map(|binding_id| semantic.binding(binding_id))
.filter(|binding| binding.start() >= expr.start())
.all(|binding| !binding.is_used())
}
_ => false,
}
}
1 change: 1 addition & 0 deletions crates/ruff/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mod tests {
#[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))]
#[test_case(Rule::ReimplementedStarmap, Path::new("FURB140.py"))]
#[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
#[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.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/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ pub(crate) use delete_full_slice::*;
pub(crate) use reimplemented_starmap::*;
pub(crate) use repeated_append::*;
pub(crate) use slice_copy::*;
pub(crate) use unnecessary_enumerate::*;

mod check_and_remove_from_set;
mod delete_full_slice;
mod reimplemented_starmap;
mod repeated_append;
mod slice_copy;
mod unnecessary_enumerate;

0 comments on commit 511cc25

Please sign in to comment.