[`flake8-bugbear`] Implement `loop-iterator-mutation` (`B909`) by mimre25 · Pull Request #9578 · astral-sh/ruff

mimre25 · 2024-01-19T15:12:02Z

Summary

This PR adds the implementation for the current 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 #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 🙂 - Bring it on 😀

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.

charliermarsh · 2024-01-19T16:16:22Z

Awesome, thank you! Excited to review, I'll give it a close read per request :)

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

charliermarsh · 2024-01-21T15:25:30Z

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

+            return _to_name_str(func);
+        }
+        _ => {
+            return "".into();


This feels off to me... Can you give me some more context on what this function is intended to do?

I think this is just the base case of the recursion.

Probably gonna remove this function anyway in favor of compose_call_path.

charliermarsh · 2024-01-21T15:25:57Z

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

+            }
+        }
+        Expr::Call(ExprCall { range: _, func, .. }) => {
+            return _to_name_str(func);


Does this mean that foo().bar and foo.bar would be considered identical?

I tried out collect_call_path and compose_call_path as you've suggested below, and both of them return None for a().foo.

Not 100% sure how I should go about this - should this be the case?

Also wondering, would you say the following should be caught?:

for bar in a().foo: del a().foo

I would say that should not be caught, since we can't know that a() returns the same object.

ok, then I think I can use compose_call_path - I was just worrying that it returns None whenever there is a() in the call path.

charliermarsh · 2024-01-21T15:26:37Z

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

+    }
+}
+
+fn _to_name_str(node: &Expr) -> String {


You may want collect_call_path for this, we have an abstraction for this exact thing, but it stores structured components (like a vector of the dot-separated pieces) rather than a String.

Could it be that the function was removed or renamed? I can't find it anymore.

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

charliermarsh · 2024-01-21T15:27:55Z

Thank you! Left some comments -- feel free to ask questions.

mimre25 · 2024-01-25T13:37:03Z

Do you have any suggestion on how to set up the work environment?

In general I think rust's incremental builds are a good idea, but damn they eat up like 10+gb for this project 😬

charliermarsh · 2024-01-25T15:03:15Z

Awesome, I should be able to review and merge this today!

mimre25 · 2024-01-25T17:07:04Z

@charliermarsh please hold off from merging, I still want to remote the to_name_str function and use collect_call_path instead.

Also, in flake8-bugbear there is currently the motion going on to make this an optional rule (disabled per default) as there are some false positives stemming from the python standard library - See the discussion in PyCQA/flake8-bugbear#455

charliermarsh · 2024-01-25T17:22:39Z

@mimre25 - Roger that!

charliermarsh · 2024-01-25T17:30:36Z

@mimre25 - I'm going to convert to draft for now just to help manage our review workflow.

MichaReiser · 2024-04-08T07:00:35Z

I close because it is stale. Please re-open if you plan to follow up on the remaining work.

mimre25 · 2024-04-11T15:15:21Z

Alright, I finally got around to finish this.

This is now on parity with flake8-bugbear's B909 (it was renamed).

Main new features in the new commits are described in PyCQA/flake8-bugbear#454.
Further, the rule now allows mutations before an unconditional break.

charliermarsh · 2024-04-11T15:30:08Z

Thanks! I'll take a look.

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

charliermarsh · 2024-04-11T18:27:26Z

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

+    }
+
+    /// Handle, e.g., `items += [1]`.
+    fn handle_aug_assign(&mut self, range: TextRange, target: &Expr) {


All these &Box<Expr> were changed to &Expr.

charliermarsh · 2024-04-11T18:27:52Z

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

+    }
+
+    /// Handle, e.g., `items[0] = 1`.
+    fn handle_assign(&mut self, range: TextRange, targets: &[Expr]) {


All these &Vec<Expr> were changed to &[Expr]. You almost always want the latter -- it's a lot more flexible, because any reference to a vector can be passed in, but so can other kinds of slices.

github-actions · 2024-04-11T19:00:35Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pypa/setuptools (error)

Cannot overwrite a value (at line 26, column 2)

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+5 -0 violations, +0 -0 fixes in 3 projects; 1 project error; 40 projects unchanged)

apache/airflow (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/models/taskinstance.py:3758:17: B909 Mutation to loop iterable `new_dict` during iteration
+ airflow/providers/openlineage/sqlparser.py:353:13: B909 Mutation to loop iterable `tables` during iteration

python/mypy (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ mypy/build.py:3200:21: B909 Mutation to loop iterable `new` during iteration

scikit-build/scikit-build (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ tests/pytest_helpers.py:49:13: B909 Mutation to loop iterable `expected_content` during iteration
+ tests/pytest_helpers.py:92:13: B909 Mutation to loop iterable `expected_content` during iteration

pypa/setuptools (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

Cannot overwrite a value (at line 26, column 2)

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
B909	5	5	0	0	0

charliermarsh · 2024-04-11T19:16:18Z

There are some false positives in here. For example:

for auth_method in authentication_methods_dict:
    authentication_methods_dict[auth_method] = True

That's probably the most common?

…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:

feat(rules): Implement flake8-bugbear B038 rule

70fb218

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.

mimre25 mentioned this pull request Jan 19, 2024

Rule: Flake8-bugbear B038 " detect changes to iterable object of loop" #9511

Closed

charliermarsh self-requested a review January 19, 2024 16:15

charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 19, 2024

charliermarsh reviewed Jan 21, 2024

View reviewed changes