Implement ForVariableUsedAfterBlock (based on wemake_python_styleguide ControlVarUsedAfterBlockViolation WPS441)

crates/ruff_linter/resources/test/fixtures/wemake_python_styleguide/control_var_used_after_block.py

@@ -0,0 +1,90 @@
+for global_var in []:
+    _ = global_var
+    pass
+
+# ❌
+_ = global_var
+
+def foo():
+    # For control var used outside block
+    for event in []:
+        _ = event
+        pass
+
+    # ❌
+    _ = event
+
+    for x in []:
+        _ = x
+        pass
+
+    # Using the same control variable name in a different loop is ok
+    for x in []:
+        _ = x
+        pass
+
+    for y in []:
+        # ❌ x is used outside of the loop it was defined in (meant to use y)
+        if x == 5:
+            pass
+
+    # Assign a variable before the loop
+    room_id = 3
+    _ = room_id
+    # Use the same variable name in a loop
+    for room_id in []:
+        _ = room_id
+        pass
+
+    # ❌ After the loop is not ok because the value is probably not what you expect
+    _ = room_id
+
+    # Tuple destructuring
+    for a, b, c in []:
+        _ = a
+        _ = b
+        _ = c
+        pass
+
+    # ❌
+    _ = a
+    _ = b
+    _ = c
+
+    # Array destructuring
+    for [d, e, f] in []:
+        _ = d
+        _ = e
+        _ = f
+        pass
+
+    # ❌
+    _ = d
+    _ = e
+    _ = f
+
+    # with statement
+    with None as w:
+        _ = w
+        pass
+
+    # ❌
+    _ = w
+
+    # Nested blocks
+    with None as q:
+        _ = q
+
+        for n in []:
+            _ = n
+            _ = q
+            pass
+
+        # ❌
+        _ = n
+
+    # ❌
+    _ = q
+    _ = n
+
+

crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs

@@ -8,7 +8,7 @@ use crate::codes::Rule;
 use crate::fix;
 use crate::rules::{
     flake8_builtins, flake8_pyi, flake8_type_checking, flake8_unused_arguments, pep8_naming,
-    pyflakes, pylint, ruff,
+    pyflakes, pylint, ruff, wemake_python_styleguide,
 };
 
 /// Run lint rules over all deferred scopes in the [`SemanticModel`].
@@ -17,6 +17,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
         Rule::AsyncioDanglingTask,
         Rule::BadStaticmethodArgument,
         Rule::BuiltinAttributeShadowing,
+        Rule::ControlVarUsedAfterBlock,
         Rule::GlobalVariableNotAssigned,
         Rule::ImportPrivateName,
         Rule::ImportShadowedByLoopVar,
@@ -86,6 +87,14 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
     for scope_id in checker.analyze.scopes.iter().rev().copied() {
         let scope = &checker.semantic.scopes[scope_id];
 
+        if checker.enabled(Rule::ControlVarUsedAfterBlock) {
+            wemake_python_styleguide::rules::control_var_used_after_block(
+                checker,
+                scope,
+                &mut diagnostics,
+            );
+        }
+
         if checker.enabled(Rule::UndefinedLocal) {
             pyflakes::rules::undefined_local(checker, scope_id, scope, &mut diagnostics);
         }

crates/ruff_linter/src/codes.rs

@@ -1087,6 +1087,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Flake8Logging, "002") => (RuleGroup::Stable, rules::flake8_logging::rules::InvalidGetLoggerArgument),
         (Flake8Logging, "007") => (RuleGroup::Stable, rules::flake8_logging::rules::ExceptionWithoutExcInfo),
         (Flake8Logging, "009") => (RuleGroup::Stable, rules::flake8_logging::rules::UndocumentedWarn),
+
+        // wemake-python-styleguide
+        (WemakePythonStyleguide, "441") => (RuleGroup::Stable, rules::wemake_python_styleguide::rules::ControlVarUsedAfterBlock),
 
         _ => return None,
     })

crates/ruff_linter/src/registry.rs

@@ -208,6 +208,9 @@ pub enum Linter {
     /// Ruff-specific rules
     #[prefix = "RUF"]
     Ruff,
+    /// [wemake-python-styleguide](https://github.com/wemake-services/wemake-python-styleguide)
+    #[prefix = "WPS"]
+    WemakePythonStyleguide,
 }
 
 pub trait RuleNamespace: Sized {

crates/ruff_linter/src/rules/mod.rs

@@ -57,3 +57,4 @@ pub mod pyupgrade;
 pub mod refurb;
 pub mod ruff;
 pub mod tryceratops;
+pub mod wemake_python_styleguide;

crates/ruff_linter/src/rules/wemake_python_styleguide/mod.rs

@@ -0,0 +1,27 @@
+pub(crate) mod rules;
+
+#[cfg(test)]
+mod tests {
+    use std::path::Path;
+
+    use anyhow::Result;
+    use test_case::test_case;
+
+    use crate::registry::Rule;
+    use crate::test::test_path;
+    use crate::{assert_messages, settings};
+
+    #[test_case(
+        Rule::ControlVarUsedAfterBlock,
+        Path::new("control_var_used_after_block.py")
+    )]
+    fn rules(rule_code: Rule, path: &Path) -> Result<()> {
+        let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
+        let diagnostics = test_path(
+            Path::new("wemake_python_styleguide").join(path).as_path(),
+            &settings::LinterSettings::for_rule(rule_code),
+        )?;
+        assert_messages!(snapshot, diagnostics);
+        Ok(())
+    }
+}

crates/ruff_linter/src/rules/wemake_python_styleguide/rules/control_var_used_after_block.rs

@@ -0,0 +1,151 @@
+use std::fmt;
+
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::Stmt;
+use ruff_python_semantic::{BindingId, NodeId, Scope};
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for variables defined in `for` loops and `with` statements that are used
+/// outside of their respective blocks.
+///
+/// ## Why is this bad?
+/// Usage of of a control variable outside of the block they're defined in will probably
+/// lead to flawed logic in a way that will likely cause bugs. The variable might not
+/// contain what you expect.
+///
+/// In Python, unlike many other languages, `for` loops and `with` statements don't
+/// define their own scopes. Therefore, usage of the control variables outside of the
+/// block will be the the value from the last iteration until re-assigned.
+///
+/// While this mistake is easy to spot in small examples, it can be hidden in larger
+/// blocks of code, where the the loop and downstream usage may not be visible at the
+/// same time.
+///
+/// ## Example
+/// ```python
+/// for x in range(10):
+///     pass
+///
+/// print(x)  # prints 9
+///
+/// with path.open() as f:
+///     pass
+///
+/// print(f.readline())  # prints a line from a file that is already closed (error)
+/// ```
+#[violation]
+pub struct ControlVarUsedAfterBlock {
+    control_var_name: String,
+    block_kind: BlockKind,
+}
+
+impl Violation for ControlVarUsedAfterBlock {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let ControlVarUsedAfterBlock {
+            control_var_name,
+            block_kind,
+        } = self;
+
+        format!("{block_kind} variable {control_var_name} is used outside of block")
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+enum BlockKind {
+    For,
+    With,
+}
+
+impl fmt::Display for BlockKind {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            BlockKind::For => fmt.write_str("`for` loop"),
+            BlockKind::With => fmt.write_str("`with` statement"),
+        }
+    }
+}
+
+/// WPS441: Forbid control variables after the block body.
+pub(crate) fn control_var_used_after_block(
+    checker: &Checker,
+    scope: &Scope,
+    diagnostics: &mut Vec<Diagnostic>,
+) {
+    // Keep track of node_ids we know are used in the same block. This helps us when
+    // people use the same variable name in multiple blocks.
+    let mut known_good_reference_node_ids: Vec<NodeId> = Vec::new();
+
+    let all_bindings: Vec<(&str, BindingId)> = scope.all_bindings().collect();
+    // We want to reverse the bindings so that we iterate in source order and shadowed
+    // bindings come first.
+    let reversed_bindings = all_bindings.iter().rev();
+
+    // Find for-loop and with-statement variable bindings
+    for (&name, binding) in reversed_bindings
+        .map(|(name, binding_id)| (name, checker.semantic().binding(*binding_id)))
+        .filter_map(|(name, binding)| {
+            if binding.kind.is_loop_var() || binding.kind.is_with_item_var() {
+                return Some((name, binding));
+            }
+
+            None
+        })
+    {
+        let binding_statement = binding.statement(checker.semantic()).unwrap();
+        let binding_source_node_id = binding.source.unwrap();
+        // The node_id of the for-loop that contains the binding
+        let binding_statement_id = checker.semantic().statement_id(binding_source_node_id);
+
+        // Loop over the references of those bindings to see if they're in the same block-scope
+        for reference in binding.references() {
+            let reference = checker.semantic().reference(reference);
+            let reference_node_id = reference.expression_id().unwrap();
+
+            // Skip any reference that come before the control var binding in the source
+            // order, skip it because people can assign and use the same variable name
+            // above the block.
+            if reference.range().end() < binding_statement.range().start() {
+                continue;
+            }
+
+            // Traverse the hierarchy and look for a block match
+            let statement_hierarchy: Vec<NodeId> = checker
+                .semantic()
+                .parent_statement_ids(reference_node_id)
+                .collect();
+
+            let mut is_used_in_block = false;
+            for ancestor_node_id in statement_hierarchy {
+                if binding_statement_id == ancestor_node_id {
+                    is_used_in_block = true;
+                    known_good_reference_node_ids.push(reference_node_id);
+                    break;
+                }
+            }
+
+            // If the reference wasn't used in the same block, report a violation/diagnostic
+            if !is_used_in_block && !known_good_reference_node_ids.contains(&reference_node_id) {
+                let block_kind = match binding_statement {
+                    Stmt::For(_) => BlockKind::For,
+                    Stmt::With(_) => BlockKind::With,
+                    _ => {
+                        panic!("Unexpected block item. This is a problem with ruff itself. Fix the `filter_map` above.")
+                    }
+                };
+
+                diagnostics.push(Diagnostic::new(
+                    ControlVarUsedAfterBlock {
+                        control_var_name: name.to_owned(),
+                        block_kind,
+                    },
+                    reference.range(),
+                ));
+            }
+        }
+    }
+}

crates/ruff_linter/src/rules/wemake_python_styleguide/rules/mod.rs

@@ -0,0 +1,3 @@
+pub(crate) use control_var_used_after_block::*;
+
+mod control_var_used_after_block;