Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow async with in redefined-loop-name #5125

Merged
merged 1 commit into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/redefined_loop_name.py
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
Original file line number Diff line number Diff line change
Expand Up @@ -1482,7 +1482,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 @@ -1523,7 +1523,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));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this while I was here.

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
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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made these private, moved them below the violation (so the violation appears first in the module).

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>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need these generics, and we can remove some of the lifetimes too (unrelated to the bug fix).

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