Improve the message for PLW2901: use "outer" and "inner" judiciously (#3263)

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"),
+            InnerBindingKind::For => fmt.write_str("`for` loop"),
-            BindingKind::With => fmt.write_str("with statement"),
+            InnerBindingKind::With => fmt.write_str("`with` statement"),
-            BindingKind::Assignment => fmt.write_str("assignment"),
+            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)
+        )
+    }
+}
+
 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 outer_kind: OuterBindingKind,
-        pub inner_kind: BindingKind,
+        pub inner_kind: InnerBindingKind,
     }
 );
 impl Violation for RedefinedLoopName {
@@ -80,18 +108,42 @@ impl Violation for RedefinedLoopName {
             outer_kind,
             inner_kind,
         } = self;
+        // 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<ExprWithBindingKind<'a>>,
+    assignment_targets: Vec<ExprWithInnerBindingKind<'a>>,
 }
 
 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<ExprWithBindingKind<'a>> =
+                let outer_assignment_targets: Vec<ExprWithOuterBindingKind<'a>> =
                     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<ExprWithBindingKind<'a>> =
+                let outer_assignment_targets: Vec<ExprWithOuterBindingKind<'a>> =
                     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 {