Skip to content

Commit

Permalink
Check sequence type before triggering unnecessary-enumerate (`FURB1…
Browse files Browse the repository at this point in the history
…48`) `len` suggestion (#7781)

## Summary

Check that the sequence type is a list, set, dict, or tuple before
recommending replacing the `enumerate(...)` call with `range(len(...))`.
Document behaviour so users are aware of the type inference limitation
leading to false negatives.

Closes #7656.
  • Loading branch information
tjkuson committed Oct 3, 2023
1 parent 69b8136 commit 37d21c0
Show file tree
Hide file tree
Showing 4 changed files with 436 additions and 234 deletions.
34 changes: 34 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB148.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
books = ["Dune", "Foundation", "Neuromancer"]

books_and_authors = {
"Dune": "Frank Herbert",
"Foundation": "Isaac Asimov",
"Neuromancer": "William Gibson",
}

books_set = {"Dune", "Foundation", "Neuromancer"}

books_tuple = ("Dune", "Foundation", "Neuromancer")

# Errors
for index, _ in enumerate(books):
print(index)
Expand Down Expand Up @@ -55,6 +65,24 @@
for(index), _ in enumerate(books):
print(index)

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

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

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

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

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

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

# OK
for index, book in enumerate(books):
print(index, book)
Expand All @@ -64,3 +92,9 @@

for book in books:
print(book)

# Generators don't support the len() function.
# https://github.com/astral-sh/ruff/issues/7656
a = (b for b in range(1, 100))
for i, _ in enumerate(a):
print(i)
32 changes: 32 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/unnecessary_enumerate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::{Arguments, Constant, Expr, Int};
use ruff_python_codegen::Generator;
use ruff_python_semantic::analyze::typing::{is_dict, is_list, is_set, is_tuple};
use ruff_python_semantic::Binding;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
Expand All @@ -23,6 +25,16 @@ use crate::registry::AsRule;
/// `range(len(...))` or the sequence itself, respectively, instead. This is
/// more efficient and communicates the intent of the code more clearly.
///
/// ## Known problems
/// This rule is prone to false negatives due to type inference limitations;
/// namely, it will only suggest a fix using the `len` builtin function if the
/// sequence passed to `enumerate` is an instantiated as a list, set, dict, or
/// tuple literal, or annotated as such with a type annotation.
///
/// The `len` builtin function is not defined for all object types (such as
/// generators), and so refactoring to use `len` over `enumerate` is not always
/// safe.
///
/// ## Example
/// ```python
/// for index, _ in enumerate(sequence):
Expand Down Expand Up @@ -143,6 +155,26 @@ pub(crate) fn unnecessary_enumerate(checker: &mut Checker, stmt_for: &ast::StmtF
checker.diagnostics.push(diagnostic);
}
(false, true) => {
// Ensure the sequence object works with `len`. If it doesn't, the
// fix is unclear.
let scope = checker.semantic().current_scope();
let bindings: Vec<&Binding> = scope
.get_all(sequence)
.map(|binding_id| checker.semantic().binding(binding_id))
.collect();
let [binding] = bindings.as_slice() else {
return;
};
// This will lead to a lot of false negatives, but it is the best
// we can do with the current type inference.
if !is_list(binding, checker.semantic())
&& !is_dict(binding, checker.semantic())
&& !is_set(binding, checker.semantic())
&& !is_tuple(binding, checker.semantic())
{
return;
}

// The value is unused, so replace with `for index in range(len(sequence))`.
let mut diagnostic = Diagnostic::new(
UnnecessaryEnumerate {
Expand Down
Loading

0 comments on commit 37d21c0

Please sign in to comment.