Skip to content

Commit

Permalink
Fix false positives
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jan 14, 2024
1 parent ef4dae4 commit 5c25d68
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,22 @@ def func():
if idx > 10:
idx *= 2
h(x)


def func():
# OK (index used after loop)
idx = 0
for x in range(5):
g(x, idx)
idx += 1
h(x)
print(idx)


def func():
# OK (index within nested loop)
idx = 0
for x in range(5):
for y in range(5):
g(x, idx)
idx += 1
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, refurb};
use crate::rules::{flake8_bugbear, flake8_simplify, 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 @@ -27,6 +27,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryEnumerate) {
refurb::rules::unnecessary_enumerate(checker, stmt_for);
}
if checker.enabled(Rule::EnumerateForLoop) {
flake8_simplify::rules::enumerate_for_loop(checker, stmt_for);
}
}
}
}
6 changes: 2 additions & 4 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1258,9 +1258,10 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
},
) => {
if checker.any_enabled(&[
Rule::UnusedLoopControlVariable,
Rule::EnumerateForLoop,
Rule::IncorrectDictIterator,
Rule::UnnecessaryEnumerate,
Rule::UnusedLoopControlVariable,
Rule::YieldInForLoop,
]) {
checker.deferred.for_loops.push(checker.semantic.snapshot());
Expand Down Expand Up @@ -1311,9 +1312,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::TryExceptInLoop) {
perflint::rules::try_except_in_loop(checker, body);
}
if checker.enabled(Rule::EnumerateForLoop) {
flake8_simplify::rules::enumerate_for_loop(checker, for_stmt);
}
}
}
Stmt::Try(ast::StmtTry {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor)
return;
}

// If the loop contains an increment statement (e.g., `i += 1`)...
for stmt in &for_stmt.body {
// Find the augmented assignment expression (e.g., `i += 1`).
if let Some(index) = match_index_increment(stmt) {
// Find the binding corresponding to the augmented assignment (e.g., `i += 1`).
// Find the binding corresponding to the initialization (e.g., `i = 1`).
let Some(id) = checker.semantic().resolve_name(index) else {
continue;
};
Expand All @@ -70,6 +70,11 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor)
continue;
}

// If the variable is global or nonlocal, ignore it.
if binding.is_global() || binding.is_nonlocal() {
continue;
}

// Ensure that the index variable was initialized to 0.
let Some(value) = typing::find_binding_value(&index.id, binding, checker.semantic())
else {
Expand All @@ -93,20 +98,41 @@ pub(crate) fn enumerate_for_loop(checker: &mut Checker, for_stmt: &ast::StmtFor)
let Some(assignment_id) = binding.source else {
continue;
};
if !checker.semantic().same_branch(for_loop_id, assignment_id) {
if checker.semantic().parent_statement_id(for_loop_id)
!= checker.semantic().parent_statement_id(assignment_id)
{
continue;
}

// If there are multiple assignments to this variable _within_ the loop, ignore it.
if checker
.semantic()
.current_scope()
.get_all(&index.id)
.map(|id| checker.semantic().binding(id))
.filter(|binding| for_stmt.range().contains_range(binding.range()))
.count()
> 1
{
// Identify the binding created by the augmented assignment.
// TODO(charlie): There should be a way to go from `ExprName` to `BindingId` (like
// `resolve_name`, but for bindings rather than references).
let binding = {
let mut bindings = checker
.semantic()
.current_scope()
.get_all(&index.id)
.map(|id| checker.semantic().binding(id))
.filter(|binding| for_stmt.range().contains_range(binding.range()));

let Some(binding) = bindings.next() else {
continue;
};

// If there are multiple assignments to this variable _within_ the loop, ignore it.
if bindings.next().is_some() {
continue;
}

binding
};

// If the variable is used _after_ the loop, ignore it.
// Find the binding for the augmented assignment.
if binding.references.iter().any(|id| {
let reference = checker.semantic().reference(*id);
reference.start() > for_stmt.end()
}) {
continue;
}

Expand Down

0 comments on commit 5c25d68

Please sign in to comment.