diff --git a/crates/ruff/src/rules/pylint/rules/redefined_loop_name.rs b/crates/ruff/src/rules/pylint/rules/redefined_loop_name.rs index 66d114586e..abf7b5224e 100644 --- a/crates/ruff/src/rules/pylint/rules/redefined_loop_name.rs +++ b/crates/ruff/src/rules/pylint/rules/redefined_loop_name.rs @@ -1,3 +1,10 @@ +use std::{fmt, iter}; + +use rustpython_parser::ast::{Expr, ExprContext, ExprKind, Stmt, StmtKind, Withitem}; +use serde::{Deserialize, Serialize}; + +use ruff_macros::{define_violation, derive_message_formats}; + use crate::ast::comparable::ComparableExpr; use crate::ast::helpers::unparse_expr; use crate::ast::types::{Node, Range}; @@ -7,28 +14,49 @@ use crate::checkers::ast::Checker; use crate::registry::Diagnostic; use crate::settings::hashable::HashableRegex; use crate::violation::Violation; -use ruff_macros::{define_violation, derive_message_formats}; -use rustpython_parser::ast::{Expr, ExprContext, ExprKind, Stmt, StmtKind, Withitem}; -use serde::{Deserialize, Serialize}; -use std::{fmt, iter}; #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone, Copy)] -pub enum BindingKind { +pub 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, Serialize, Deserialize, Clone, Copy)] +pub enum InnerBindingKind { For, With, Assignment, } -impl fmt::Display for BindingKind { +impl fmt::Display for InnerBindingKind { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { match self { - BindingKind::For => fmt.write_str("for loop"), - BindingKind::With => fmt.write_str("with statement"), - BindingKind::Assignment => fmt.write_str("assignment"), + InnerBindingKind::For => fmt.write_str("`for` loop"), + InnerBindingKind::With => fmt.write_str("`with` statement"), + InnerBindingKind::Assignment => fmt.write_str("assignment"), } } } +impl PartialEq for OuterBindingKind { + fn eq(&self, other: &InnerBindingKind) -> bool { + matches!( + (self, other), + (OuterBindingKind::For, InnerBindingKind::For) + | (OuterBindingKind::With, InnerBindingKind::With) + ) + } +} + define_violation!( /// ## What it does /// Checks for variables defined in `for` loops and `with` statements that @@ -68,8 +96,8 @@ define_violation!( /// ``` pub struct RedefinedLoopName { pub name: String, - pub outer_kind: BindingKind, - pub inner_kind: BindingKind, + pub outer_kind: OuterBindingKind, + pub inner_kind: InnerBindingKind, } ); impl Violation for RedefinedLoopName { @@ -80,18 +108,42 @@ impl Violation for RedefinedLoopName { outer_kind, inner_kind, } = self; - format!("Outer {outer_kind} variable `{name}` overwritten by inner {inner_kind} target") + // Prefix the nouns describing the outer and inner kinds with "outer" and "inner" + // to better distinguish them, but to avoid confusion, only do so if the outer and inner + // kinds are equal. For example, instead of: + // + // "Outer `for` loop variable `i` overwritten by inner assignment target." + // + // We have: + // + // "`for` loop variable `i` overwritten by assignment target." + // + // While at the same time, we have: + // + // "Outer `for` loop variable `i` overwritten by inner `for` loop target." + // "Outer `with` statement variable `f` overwritten by inner `with` statement target." + + if outer_kind == inner_kind { + format!("Outer {outer_kind} variable `{name}` overwritten by inner {inner_kind} target") + } else { + format!("{outer_kind} variable `{name}` overwritten by {inner_kind} target") + } } } -struct ExprWithBindingKind<'a> { +struct ExprWithOuterBindingKind<'a> { expr: &'a Expr, - binding_kind: BindingKind, + binding_kind: OuterBindingKind, +} + +struct ExprWithInnerBindingKind<'a> { + expr: &'a Expr, + binding_kind: InnerBindingKind, } struct InnerForWithAssignTargetsVisitor<'a> { dummy_variable_rgx: &'a HashableRegex, - assignment_targets: Vec>, + assignment_targets: Vec>, } impl<'a, 'b> Visitor<'b> for InnerForWithAssignTargetsVisitor<'a> @@ -105,9 +157,9 @@ where StmtKind::For { target, .. } | StmtKind::AsyncFor { target, .. } => { self.assignment_targets.extend( assignment_targets_from_expr(target, self.dummy_variable_rgx).map(|expr| { - ExprWithBindingKind { + ExprWithInnerBindingKind { expr, - binding_kind: BindingKind::For, + binding_kind: InnerBindingKind::For, } }), ); @@ -116,9 +168,9 @@ where StmtKind::With { items, .. } => { self.assignment_targets.extend( assignment_targets_from_with_items(items, self.dummy_variable_rgx).map( - |expr| ExprWithBindingKind { + |expr| ExprWithInnerBindingKind { expr, - binding_kind: BindingKind::With, + binding_kind: InnerBindingKind::With, }, ), ); @@ -127,9 +179,9 @@ where StmtKind::Assign { targets, .. } => { self.assignment_targets.extend( assignment_targets_from_assign_targets(targets, self.dummy_variable_rgx).map( - |expr| ExprWithBindingKind { + |expr| ExprWithInnerBindingKind { expr, - binding_kind: BindingKind::Assignment, + binding_kind: InnerBindingKind::Assignment, }, ), ); @@ -137,9 +189,9 @@ where StmtKind::AugAssign { target, .. } | StmtKind::AnnAssign { target, .. } => { self.assignment_targets.extend( assignment_targets_from_expr(target, self.dummy_variable_rgx).map(|expr| { - ExprWithBindingKind { + ExprWithInnerBindingKind { expr, - binding_kind: BindingKind::Assignment, + binding_kind: InnerBindingKind::Assignment, } }), ); @@ -241,11 +293,11 @@ pub fn redefined_loop_name<'a, 'b>(checker: &'a mut Checker<'b>, node: &Node<'b> Node::Stmt(stmt) => match &stmt.node { // With. StmtKind::With { items, body, .. } => { - let outer_assignment_targets: Vec> = + let outer_assignment_targets: Vec> = assignment_targets_from_with_items(items, &checker.settings.dummy_variable_rgx) - .map(|expr| ExprWithBindingKind { + .map(|expr| ExprWithOuterBindingKind { expr, - binding_kind: BindingKind::With, + binding_kind: OuterBindingKind::With, }) .collect(); let mut visitor = InnerForWithAssignTargetsVisitor { @@ -259,11 +311,11 @@ pub fn redefined_loop_name<'a, 'b>(checker: &'a mut Checker<'b>, node: &Node<'b> } // For and async for. StmtKind::For { target, body, .. } | StmtKind::AsyncFor { target, body, .. } => { - let outer_assignment_targets: Vec> = + let outer_assignment_targets: Vec> = assignment_targets_from_expr(target, &checker.settings.dummy_variable_rgx) - .map(|expr| ExprWithBindingKind { + .map(|expr| ExprWithOuterBindingKind { expr, - binding_kind: BindingKind::For, + binding_kind: OuterBindingKind::For, }) .collect(); let mut visitor = InnerForWithAssignTargetsVisitor {