From 70fb21868d317eee2cb0cf8b6cc7032f36d369b5 Mon Sep 17 00:00:00 2001 From: Martin Imre Date: Fri, 19 Jan 2024 14:52:26 +0100 Subject: [PATCH] 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. --- .../test/fixtures/flake8_bugbear/B038.py | 75 +++++++ .../ast/analyze/deferred_for_loops.rs | 3 + .../src/checkers/ast/analyze/statement.rs | 1 + crates/ruff_linter/src/codes.rs | 1 + .../src/rules/flake8_bugbear/mod.rs | 1 + .../rules/loop_iterator_mutated.rs | 185 ++++++++++++++++++ .../src/rules/flake8_bugbear/rules/mod.rs | 2 + ...__flake8_bugbear__tests__B038_B038.py.snap | 137 +++++++++++++ ruff.schema.json | 1 + 9 files changed, 406 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B038.py create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/rules/loop_iterator_mutated.rs create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B038_B038.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B038.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B038.py new file mode 100644 index 0000000000000..6e116034fe801 --- /dev/null +++ b/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 + diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs index ee2378dd8bd0e..90acb6d6a300a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/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); + } } } } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index c31f147997663..9e21d527bbfdd 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/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()); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d073f0223f089..7df72c78633d1 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index 8cde263044c04..29aa39b46acf5 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/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( diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/loop_iterator_mutated.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/loop_iterator_mutated.rs new file mode 100644 index 0000000000000..fa9637562b478 --- /dev/null +++ b/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>, +} + +/// `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); + } +} diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs index 3f8a1fdaf646f..4ddfce6d95cb4 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B038_B038.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B038_B038.py.snap new file mode 100644 index 0000000000000..043c533b3a6ea --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B038_B038.py.snap @@ -0,0 +1,137 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +--- +B038.py:11:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | + 9 | print(elem) +10 | if elem % 2 == 0: +11 | some_list.remove(elem) # should error + | ^^^^^^^^^^^^^^^^^^^^^^ B038 +12 | +13 | some_list = [1, 2, 3] + | + +B038.py:26:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +24 | print(elem) +25 | if elem % 2 == 0: +26 | del some_list[2] # should error + | ^^^^^^^^^^^^^^^^ B038 +27 | del some_list + | + +B038.py:27:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +25 | if elem % 2 == 0: +26 | del some_list[2] # should error +27 | del some_list + | ^^^^^^^^^^^^^ B038 + | + +B038.py:41:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +39 | print(elem) +40 | if elem % 2 == 0: +41 | a.some_list.remove(elem) # should error + | ^^^^^^^^^^^^^^^^^^^^^^^^ B038 +42 | +43 | a = A((1, 2, 3)) + | + +B038.py:47:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +45 | print(elem) +46 | if elem % 2 == 0: +47 | del a.some_list[2] # should error + | ^^^^^^^^^^^^^^^^^^ B038 + | + +B038.py:56:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +54 | if elem == 2: +55 | found_idx = some_list.index(elem) # should not error +56 | some_list.append(elem) # should error + | ^^^^^^^^^^^^^^^^^^^^^^ B038 +57 | some_list.sort() # should error +58 | some_list.reverse() # should error + | + +B038.py:57:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +55 | found_idx = some_list.index(elem) # should not error +56 | some_list.append(elem) # should error +57 | some_list.sort() # should error + | ^^^^^^^^^^^^^^^^ B038 +58 | some_list.reverse() # should error +59 | some_list.clear() # should error + | + +B038.py:58:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +56 | some_list.append(elem) # should error +57 | some_list.sort() # should error +58 | some_list.reverse() # should error + | ^^^^^^^^^^^^^^^^^^^ B038 +59 | some_list.clear() # should error +60 | some_list.extend([1,2]) # should error + | + +B038.py:59:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +57 | some_list.sort() # should error +58 | some_list.reverse() # should error +59 | some_list.clear() # should error + | ^^^^^^^^^^^^^^^^^ B038 +60 | some_list.extend([1,2]) # should error +61 | some_list.insert(1, 1) # should error + | + +B038.py:60:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +58 | some_list.reverse() # should error +59 | some_list.clear() # should error +60 | some_list.extend([1,2]) # should error + | ^^^^^^^^^^^^^^^^^^^^^^^ B038 +61 | some_list.insert(1, 1) # should error +62 | some_list.pop(1) # should error + | + +B038.py:61:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +59 | some_list.clear() # should error +60 | some_list.extend([1,2]) # should error +61 | some_list.insert(1, 1) # should error + | ^^^^^^^^^^^^^^^^^^^^^^ B038 +62 | some_list.pop(1) # should error +63 | some_list.pop() # should error + | + +B038.py:62:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +60 | some_list.extend([1,2]) # should error +61 | some_list.insert(1, 1) # should error +62 | some_list.pop(1) # should error + | ^^^^^^^^^^^^^^^^ B038 +63 | some_list.pop() # should error +64 | some_list = 3 # should error + | + +B038.py:63:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +61 | some_list.insert(1, 1) # should error +62 | some_list.pop(1) # should error +63 | some_list.pop() # should error + | ^^^^^^^^^^^^^^^ B038 +64 | some_list = 3 # should error +65 | break + | + +B038.py:74:9: B038 editing a loop's mutable iterable often leads to unexpected results/bugs + | +72 | if mydicts.get('a', ''): +73 | print(mydict['foo']) # should not error +74 | mydicts.popitem() # should error + | ^^^^^^^^^^^^^^^^^ B038 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index ef61102357472..c74ff4d4835c4 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2600,6 +2600,7 @@ "B032", "B033", "B034", + "B038", "B9", "B90", "B904",