Skip to content

Commit

Permalink
[flake8-bugbear] Implement loop-iterator-mutation (B909) (astra…
Browse files Browse the repository at this point in the history
…l-sh#9578)

## Summary
This PR adds the implementation for the current
[flake8-bugbear](https://github.com/PyCQA/flake8-bugbear)'s B038 rule.
The B038 rule checks for mutation of loop iterators in the body of a for
loop and alerts when found.

Rational: 
Editing the loop iterator can lead to undesired behavior and is probably
a bug in most cases.

Closes astral-sh#9511.

Note there will be a second iteration of B038 implemented in
`flake8-bugbear` soon, and this PR currently only implements the weakest
form of the rule.
I'd be happy to also implement the further improvements to B038 here in
ruff 🙂
See PyCQA/flake8-bugbear#454 for more
information on the planned improvements.

## Test Plan
Re-using the same test file that I've used for `flake8-bugbear`, which
is included in this PR (look for the `B038.py` file).


Note: this is my first time using `rust` (beside `rustlings`) - I'd be
very happy about thorough feedback on what I could've done better
:slightly_smiling_face: - Bring it on :grinning:
  • Loading branch information
mimre25 authored and Glyphack committed Apr 12, 2024
1 parent 1a0bdf9 commit 0813b9d
Show file tree
Hide file tree
Showing 9 changed files with 805 additions and 0 deletions.
160 changes: 160 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B909.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
"""
Should emit:
B909 - on lines 11, 25, 26, 40, 46
"""

# lists

some_list = [1, 2, 3]
some_other_list = [1, 2, 3]
for elem in some_list:
# errors
some_list.remove(0)
del some_list[2]
some_list.append(elem)
some_list.sort()
some_list.reverse()
some_list.clear()
some_list.extend([1, 2])
some_list.insert(1, 1)
some_list.pop(1)
some_list.pop()

# conditional break should error
if elem == 2:
some_list.remove(0)
if elem == 3:
break

# non-errors
some_other_list.remove(elem)
del some_list
del some_other_list
found_idx = some_list.index(elem)
some_list = 3

# unconditional break should not error
if elem == 2:
some_list.remove(elem)
break


# dicts
mydicts = {"a": {"foo": 1, "bar": 2}}

for elem in mydicts:
# errors
mydicts.popitem()
mydicts.setdefault("foo", 1)
mydicts.update({"foo": "bar"})

# no errors
elem.popitem()
elem.setdefault("foo", 1)
elem.update({"foo": "bar"})

# sets

myset = {1, 2, 3}

for _ in myset:
# errors
myset.update({4, 5})
myset.intersection_update({4, 5})
myset.difference_update({4, 5})
myset.symmetric_difference_update({4, 5})
myset.add(4)
myset.discard(3)

# no errors
del myset


# members
class A:
some_list: list

def __init__(self, ls):
self.some_list = list(ls)


a = A((1, 2, 3))
# ensure member accesses are handled as errors
for elem in a.some_list:
a.some_list.remove(0)
del a.some_list[2]


# Augassign should error

foo = [1, 2, 3]
bar = [4, 5, 6]
for _ in foo:
foo *= 2
foo += bar
foo[1] = 9
foo[1:2] = bar
foo[1:2:3] = bar

foo = {1, 2, 3}
bar = {4, 5, 6}
for _ in foo: # should error
foo |= bar
foo &= bar
foo -= bar
foo ^= bar


# more tests for unconditional breaks - should not error
for _ in foo:
foo.remove(1)
for _ in bar:
bar.remove(1)
break
break

# should not error
for _ in foo:
foo.remove(1)
for _ in bar:
...
break

# should error (?)
for _ in foo:
foo.remove(1)
if bar:
bar.remove(1)
break
break

# should error
for _ in foo:
if bar:
pass
else:
foo.remove(1)

# should error
for elem in some_list:
if some_list.pop() == 2:
pass

# should not error
for elem in some_list:
if some_list.pop() == 2:
break

# should error
for elem in some_list:
if some_list.pop() == 2:
pass
else:
break

# should not error
for elem in some_list:
del some_list[elem]
some_list[elem] = 1
some_list.remove(elem)
some_list.discard(elem)
Original file line number Diff line number Diff line change
Expand Up @@ -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::LoopIteratorMutation) {
flake8_bugbear::rules::loop_iterator_mutation(checker, stmt_for);
}
}
}
}
1 change: 1 addition & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1268,6 +1268,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.any_enabled(&[
Rule::EnumerateForLoop,
Rule::IncorrectDictIterator,
Rule::LoopIteratorMutation,
Rule::UnnecessaryEnumerate,
Rule::UnusedLoopControlVariable,
Rule::YieldInForLoop,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Bugbear, "035") => (RuleGroup::Stable, rules::flake8_bugbear::rules::StaticKeyDictComprehension),
(Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept),
(Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict),
(Flake8Bugbear, "909") => (RuleGroup::Preview, rules::flake8_bugbear::rules::LoopIteratorMutation),

// flake8-blind-except
(Flake8BlindExcept, "001") => (RuleGroup::Stable, rules::flake8_blind_except::rules::BlindExcept),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ mod tests {
#[test_case(Rule::UselessContextlibSuppress, Path::new("B022.py"))]
#[test_case(Rule::UselessExpression, Path::new("B018.ipynb"))]
#[test_case(Rule::UselessExpression, Path::new("B018.py"))]
#[test_case(Rule::LoopIteratorMutation, Path::new("B909.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
Loading

0 comments on commit 0813b9d

Please sign in to comment.