Skip to content

Commit

Permalink
Allow async with in redefined-loop-name (#5125)
Browse files Browse the repository at this point in the history 
## Summary

Closes #5124.
  • Loading branch information
@charliermarsh
charliermarsh committed Jun 15, 2023
1 parent c811213 commit 107a295
Show file tree
Hide file tree
Showing 4 changed files with 244 additions and 216 deletions.
15 changes: 15 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/redefined_loop_name.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@
with None as j: # ok
pass

# Async with -> with, variable reused
async with None as i:
with None as i: # error
pass

# Async with -> with, different variable
async with None as i:
with None as j: # ok
pass

# Async for -> for, variable reused
async for i in []:
for i in []: # error
pass

# For -> for -> for, doubly nested variable reuse
for i in []:
for j in []:
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/src/checkers/ast/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1483,7 +1483,7 @@ where
);
}
if self.enabled(Rule::RedefinedLoopName) {
pylint::rules::redefined_loop_name(self, &Node::Stmt(stmt));
pylint::rules::redefined_loop_name(self, stmt);
}
}
Stmt::While(ast::StmtWhile { body, orelse, .. }) => {
Expand Down Expand Up @@ -1524,7 +1524,7 @@ where
pylint::rules::useless_else_on_loop(self, stmt, body, orelse);
}
if self.enabled(Rule::RedefinedLoopName) {
pylint::rules::redefined_loop_name(self, &Node::Stmt(stmt));
pylint::rules::redefined_loop_name(self, stmt);
}
if self.enabled(Rule::IterationOverSet) {
pylint::rules::iteration_over_set(self, iter);
Expand Down
191 changes: 93 additions & 98 deletions crates/ruff/src/rules/pylint/rules/redefined_loop_name.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,53 +7,10 @@ use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor};
use ruff_python_ast::types::Node;
use ruff_python_semantic::SemanticModel;

use crate::checkers::ast::Checker;

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub(crate) enum OuterBindingKind {
For,
With,
}

impl fmt::Display for OuterBindingKind {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self {
OuterBindingKind::For => fmt.write_str("`for` loop"),
OuterBindingKind::With => fmt.write_str("`with` statement"),
}
}
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub(crate) enum InnerBindingKind {
For,
With,
Assignment,
}

impl fmt::Display for InnerBindingKind {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self {
InnerBindingKind::For => fmt.write_str("`for` loop"),
InnerBindingKind::With => fmt.write_str("`with` statement"),
InnerBindingKind::Assignment => fmt.write_str("assignment"),
}
}
}

impl PartialEq<InnerBindingKind> for OuterBindingKind {
fn eq(&self, other: &InnerBindingKind) -> bool {
matches!(
(self, other),
(OuterBindingKind::For, InnerBindingKind::For)
| (OuterBindingKind::With, InnerBindingKind::With)
)
}
}

/// ## What it does
/// Checks for variables defined in `for` loops and `with` statements that
/// get overwritten within the body, for example by another `for` loop or
Expand Down Expand Up @@ -128,6 +85,48 @@ impl Violation for RedefinedLoopName {
}
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
enum OuterBindingKind {
For,
With,
}

impl fmt::Display for OuterBindingKind {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self {
OuterBindingKind::For => fmt.write_str("`for` loop"),
OuterBindingKind::With => fmt.write_str("`with` statement"),
}
}
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
enum InnerBindingKind {
For,
With,
Assignment,
}

impl fmt::Display for InnerBindingKind {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self {
InnerBindingKind::For => fmt.write_str("`for` loop"),
InnerBindingKind::With => fmt.write_str("`with` statement"),
InnerBindingKind::Assignment => fmt.write_str("assignment"),
}
}
}

impl PartialEq<InnerBindingKind> for OuterBindingKind {
fn eq(&self, other: &InnerBindingKind) -> bool {
matches!(
(self, other),
(OuterBindingKind::For, InnerBindingKind::For)
| (OuterBindingKind::With, InnerBindingKind::With)
)
}
}

struct ExprWithOuterBindingKind<'a> {
expr: &'a Expr,
binding_kind: OuterBindingKind,
Expand Down Expand Up @@ -255,10 +254,10 @@ fn assignment_is_cast_expr(value: &Expr, target: &Expr, semantic: &SemanticModel
semantic.match_typing_expr(func, "cast")
}

fn assignment_targets_from_expr<'a, U>(
expr: &'a Expr<U>,
fn assignment_targets_from_expr<'a>(
expr: &'a Expr,
dummy_variable_rgx: &'a Regex,
) -> Box<dyn Iterator<Item = &'a Expr<U>> + 'a> {
) -> Box<dyn Iterator<Item = &'a Expr> + 'a> {
// The Box is necessary to ensure the match arms have the same return type - we can't use
// a cast to "impl Iterator", since at the time of writing that is only allowed for
// return types and argument types.
Expand Down Expand Up @@ -308,77 +307,73 @@ fn assignment_targets_from_expr<'a, U>(
}
}

fn assignment_targets_from_with_items<'a, U>(
items: &'a [Withitem<U>],
fn assignment_targets_from_with_items<'a>(
items: &'a [Withitem],
dummy_variable_rgx: &'a Regex,
) -> impl Iterator<Item = &'a Expr<U>> + 'a {
) -> impl Iterator<Item = &'a Expr> + 'a {
items
.iter()
.filter_map(|item| {
item.optional_vars
.as_ref()
.map(|expr| assignment_targets_from_expr(&**expr, dummy_variable_rgx))
.map(|expr| assignment_targets_from_expr(expr, dummy_variable_rgx))
})
.flatten()
}

fn assignment_targets_from_assign_targets<'a, U>(
targets: &'a [Expr<U>],
fn assignment_targets_from_assign_targets<'a>(
targets: &'a [Expr],
dummy_variable_rgx: &'a Regex,
) -> impl Iterator<Item = &'a Expr<U>> + 'a {
) -> impl Iterator<Item = &'a Expr> + 'a {
targets
.iter()
.flat_map(|target| assignment_targets_from_expr(target, dummy_variable_rgx))
}

/// PLW2901
pub(crate) fn redefined_loop_name<'a, 'b>(checker: &'a mut Checker<'b>, node: &Node<'b>) {
let (outer_assignment_targets, inner_assignment_targets) = match node {
Node::Stmt(stmt) => match stmt {
// With.
Stmt::With(ast::StmtWith { items, body, .. }) => {
let outer_assignment_targets: Vec<ExprWithOuterBindingKind<'a>> =
assignment_targets_from_with_items(items, &checker.settings.dummy_variable_rgx)
.map(|expr| ExprWithOuterBindingKind {
expr,
binding_kind: OuterBindingKind::With,
})
.collect();
let mut visitor = InnerForWithAssignTargetsVisitor {
context: checker.semantic(),
dummy_variable_rgx: &checker.settings.dummy_variable_rgx,
assignment_targets: vec![],
};
for stmt in body {
visitor.visit_stmt(stmt);
}
(outer_assignment_targets, visitor.assignment_targets)
pub(crate) fn redefined_loop_name(checker: &mut Checker, stmt: &Stmt) {
let (outer_assignment_targets, inner_assignment_targets) = match stmt {
Stmt::With(ast::StmtWith { items, body, .. })
| Stmt::AsyncWith(ast::StmtAsyncWith { items, body, .. }) => {
let outer_assignment_targets: Vec<ExprWithOuterBindingKind> =
assignment_targets_from_with_items(items, &checker.settings.dummy_variable_rgx)
.map(|expr| ExprWithOuterBindingKind {
expr,
binding_kind: OuterBindingKind::With,
})
.collect();
let mut visitor = InnerForWithAssignTargetsVisitor {
context: checker.semantic(),
dummy_variable_rgx: &checker.settings.dummy_variable_rgx,
assignment_targets: vec![],
};
for stmt in body {
visitor.visit_stmt(stmt);
}
// For and async for.
Stmt::For(ast::StmtFor { target, body, .. })
| Stmt::AsyncFor(ast::StmtAsyncFor { target, body, .. }) => {
let outer_assignment_targets: Vec<ExprWithOuterBindingKind<'a>> =
assignment_targets_from_expr(target, &checker.settings.dummy_variable_rgx)
.map(|expr| ExprWithOuterBindingKind {
expr,
binding_kind: OuterBindingKind::For,
})
.collect();
let mut visitor = InnerForWithAssignTargetsVisitor {
context: checker.semantic(),
dummy_variable_rgx: &checker.settings.dummy_variable_rgx,
assignment_targets: vec![],
};
for stmt in body {
visitor.visit_stmt(stmt);
}
(outer_assignment_targets, visitor.assignment_targets)
(outer_assignment_targets, visitor.assignment_targets)
}
Stmt::For(ast::StmtFor { target, body, .. })
| Stmt::AsyncFor(ast::StmtAsyncFor { target, body, .. }) => {
let outer_assignment_targets: Vec<ExprWithOuterBindingKind> =
assignment_targets_from_expr(target, &checker.settings.dummy_variable_rgx)
.map(|expr| ExprWithOuterBindingKind {
expr,
binding_kind: OuterBindingKind::For,
})
.collect();
let mut visitor = InnerForWithAssignTargetsVisitor {
context: checker.semantic(),
dummy_variable_rgx: &checker.settings.dummy_variable_rgx,
assignment_targets: vec![],
};
for stmt in body {
visitor.visit_stmt(stmt);
}
_ => panic!(
"redefined_loop_name called on Statement that is not a With, For, or AsyncFor"
),
},
Node::Expr(_) => panic!("redefined_loop_name called on Node that is not a Statement"),
(outer_assignment_targets, visitor.assignment_targets)
}
_ => panic!(
"redefined_loop_name called on Statement that is not a With, For, AsyncWith, or AsyncFor"
)
};

let mut diagnostics = Vec::new();
Expand Down
Loading

0 comments on commit 107a295

Please sign in to comment.