feat(rules): Implement flake8-bugbear B038 rule

The B038 rule checks for mutation of loop iterators in the body
of a for loop and alerts when found.
Editing the loop iterator can lead to undesired behavior and is probably
a bug in most cases.

crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B038.py

@@ -0,0 +1,75 @@
+"""
+Should emit:
+B999 - on lines 11, 25, 26, 40, 46
+"""
+
+
+some_list = [1, 2, 3]
+for elem in some_list:
+    print(elem)
+    if elem % 2 == 0:
+        some_list.remove(elem)  # should error
+
+some_list = [1, 2, 3]
+some_other_list = [1, 2, 3]
+for elem in some_list:
+    print(elem)
+    if elem % 2 == 0:
+        some_other_list.remove(elem)  # should not error
+        del some_other_list
+
+
+some_list = [1, 2, 3]
+for elem in some_list:
+    print(elem)
+    if elem % 2 == 0:
+        del some_list[2]  # should error
+        del some_list
+
+
+class A:
+    some_list: list
+
+    def __init__(self, ls):
+        self.some_list = list(ls)
+
+
+a = A((1, 2, 3))
+for elem in a.some_list:
+    print(elem)
+    if elem % 2 == 0:
+        a.some_list.remove(elem)  # should error
+
+a = A((1, 2, 3))
+for elem in a.some_list:
+    print(elem)
+    if elem % 2 == 0:
+        del a.some_list[2]  # should error
+
+
+
+some_list = [1, 2, 3]
+for elem in some_list:
+    print(elem)
+    if elem == 2:
+        found_idx = some_list.index(elem)  # should not error
+        some_list.append(elem)  # should error
+        some_list.sort()  # should error
+        some_list.reverse()  # should error
+        some_list.clear()  # should error
+        some_list.extend([1,2])  # should error
+        some_list.insert(1, 1)  # should error
+        some_list.pop(1) # should error
+        some_list.pop() # should error
+        some_list = 3 # should error
+        break
+
+
+
+mydicts = {'a': {'foo': 1, 'bar': 2}}
+
+for mydict in mydicts:
+    if mydicts.get('a', ''):
+        print(mydict['foo'])  # should not error
+        mydicts.popitem() # should error
+

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

@@ -30,6 +30,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) {
             if checker.enabled(Rule::EnumerateForLoop) {
                 flake8_simplify::rules::enumerate_for_loop(checker, stmt_for);
             }
+            if checker.enabled(Rule::LoopIteratorMutated) {
+                flake8_bugbear::rules::loop_iterator_mutated(checker, stmt_for);
+            }
         }
     }
 }

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

@@ -1266,6 +1266,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
                 Rule::UnnecessaryEnumerate,
                 Rule::UnusedLoopControlVariable,
                 Rule::YieldInForLoop,
+                Rule::LoopIteratorMutated,
             ]) {
                 checker.analyze.for_loops.push(checker.semantic.snapshot());
             }

crates/ruff_linter/src/codes.rs

@@ -348,6 +348,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Flake8Bugbear, "032") => (RuleGroup::Stable, rules::flake8_bugbear::rules::UnintentionalTypeAnnotation),
         (Flake8Bugbear, "033") => (RuleGroup::Stable, rules::flake8_bugbear::rules::DuplicateValue),
         (Flake8Bugbear, "034") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ReSubPositionalArgs),
+        (Flake8Bugbear, "038") => (RuleGroup::Preview, rules::flake8_bugbear::rules::LoopIteratorMutated),
         (Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept),
         (Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict),
 

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

@@ -60,6 +60,7 @@ mod tests {
     #[test_case(Rule::UselessContextlibSuppress, Path::new("B022.py"))]
     #[test_case(Rule::UselessExpression, Path::new("B018.py"))]
     #[test_case(Rule::UselessExpression, Path::new("B018.ipynb"))]
+    #[test_case(Rule::LoopIteratorMutated, Path::new("B038.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
         let diagnostics = test_path(

crates/ruff_linter/src/rules/flake8_bugbear/rules/loop_iterator_mutated.rs

@@ -0,0 +1,185 @@
+use ruff_diagnostics::Violation;
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{
+    visitor::{self, Visitor},
+    Expr, ExprAttribute, ExprCall, ExprContext, ExprName, ExprSubscript, Stmt, StmtDelete, StmtFor,
+};
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+use ruff_diagnostics::Diagnostic;
+
+static MUTATING_FUNCTIONS: &'static [&'static str] = &[
+    "append", "sort", "reverse", "remove", "clear", "extend", "insert", "pop", "popitem",
+];
+
+/// ## What it does
+/// Checks for mutation of the iterator of a loop in the loop's body
+///
+/// ## Why is this bad?
+/// Changing the structure that is being iterated over will usually lead to
+/// unintended behavior as not all elements will be addressed.
+///
+/// ## Example
+/// ```python
+/// some_list = [1,2,3]
+/// for i in some_list:
+///   some_list.remove(i) # this will lead to not all elements being printed
+///   print(i)
+/// ```
+///
+///
+/// ## References
+/// - [Python documentation: Mutable Sequence Types](https://docs.python.org/3/library/stdtypes.html#typesseq-mutable)
+#[violation]
+pub struct LoopIteratorMutated;
+
+impl Violation for LoopIteratorMutated {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("editing a loop's mutable iterable often leads to unexpected results/bugs")
+    }
+}
+
+fn _to_name_str(node: &Expr) -> String {
+    match node {
+        Expr::Name(ExprName { id, .. }) => {
+            return id.to_string();
+        }
+        Expr::Attribute(ExprAttribute {
+            range: _,
+            value,
+            attr,
+            ..
+        }) => {
+            let mut inner = _to_name_str(value);
+            match inner.as_str() {
+                "" => {
+                    return "".into();
+                }
+                _ => {
+                    inner.push_str(".");
+                    inner.push_str(attr);
+                    return inner;
+                }
+            }
+        }
+        Expr::Call(ExprCall { range: _, func, .. }) => {
+            return _to_name_str(func);
+        }
+        _ => {
+            return "".into();
+        }
+    }
+}
+// B038
+pub(crate) fn loop_iterator_mutated(checker: &mut Checker, stmt_for: &StmtFor) {
+    let StmtFor {
+        target: _,
+        iter,
+        body,
+        orelse: _,
+        is_async: _,
+        range: _,
+    } = stmt_for;
+    let name;
+
+    match iter.as_ref() {
+        Expr::Name(ExprName { .. }) => {
+            name = _to_name_str(iter.as_ref());
+        }
+        Expr::Attribute(ExprAttribute { .. }) => {
+            name = _to_name_str(iter.as_ref());
+        }
+        _ => {
+            println!("Shouldn't happen");
+            return;
+        }
+    }
+    let mut visitor = LoopMutationsVisitor {
+        name: &name,
+        mutations: Vec::new(),
+    };
+    visitor.visit_body(body);
+    for mutation in visitor.mutations {
+        checker
+            .diagnostics
+            .push(Diagnostic::new(LoopIteratorMutated {}, mutation.range()));
+    }
+}
+struct LoopMutationsVisitor<'a> {
+    name: &'a String,
+    mutations: Vec<Box<dyn Ranged + 'a>>,
+}
+
+/// `Visitor` to collect all used identifiers in a statement.
+impl<'a> Visitor<'a> for LoopMutationsVisitor<'a> {
+    fn visit_stmt(&mut self, stmt: &'a Stmt) {
+        match stmt {
+            Stmt::Delete(StmtDelete { range, targets }) => {
+                for target in targets {
+                    let name;
+                    match target {
+                        Expr::Subscript(ExprSubscript {
+                            range: _,
+                            value,
+                            slice: _,
+                            ctx: _,
+                        }) => {
+                            name = _to_name_str(value);
+                        }
+
+                        Expr::Attribute(_) | Expr::Name(_) => {
+                            name = _to_name_str(target);
+                        }
+                        _ => {
+                            name = String::new();
+                            visitor::walk_expr(self, target);
+                        }
+                    }
+                    if self.name.eq(&name) {
+                        self.mutations.push(Box::new(range));
+                    }
+                }
+            }
+            _ => {
+                visitor::walk_stmt(self, stmt);
+            }
+        }
+    }
+
+    fn visit_expr(&mut self, expr: &'a Expr) {
+        match expr {
+            Expr::Name(ExprName { range: _, id, ctx }) => {
+                if self.name.eq(id) {
+                    match ctx {
+                        ExprContext::Del => {
+                            self.mutations.push(Box::new(expr));
+                        }
+                        _ => {}
+                    }
+                }
+            }
+            Expr::Call(ExprCall {
+                range: _,
+                func,
+                arguments: _,
+            }) => match func.as_ref() {
+                Expr::Attribute(ExprAttribute {
+                    range: _,
+                    value,
+                    attr,
+                    ctx: _,
+                }) => {
+                    let name = _to_name_str(value);
+                    if self.name.eq(&name) && MUTATING_FUNCTIONS.contains(&attr.as_str()) {
+                        self.mutations.push(Box::new(expr));
+                    }
+                }
+                _ => {}
+            },
+            _ => {}
+        }
+        visitor::walk_expr(self, expr);
+    }
+}

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

@@ -12,6 +12,7 @@ pub(crate) use function_call_in_argument_default::*;
 pub(crate) use function_uses_loop_variable::*;
 pub(crate) use getattr_with_constant::*;
 pub(crate) use jump_statement_in_finally::*;
+pub(crate) use loop_iterator_mutated::*;
 pub(crate) use loop_variable_overrides_iterator::*;
 pub(crate) use mutable_argument_default::*;
 pub(crate) use no_explicit_stacklevel::*;
@@ -46,6 +47,7 @@ mod function_call_in_argument_default;
 mod function_uses_loop_variable;
 mod getattr_with_constant;
 mod jump_statement_in_finally;
+mod loop_iterator_mutated;
 mod loop_variable_overrides_iterator;
 mod mutable_argument_default;
 mod no_explicit_stacklevel;