Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove discard, remove, and pop allowance for loop-iterator-mutation #12365

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

charliermarsh
Copy link
Member

Summary

Pretty sure this should still be an error, but also, I think I added this because of ecosystem CI? So want to see what pops up.

Closes #12164.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jul 17, 2024
Copy link
Contributor

github-actions bot commented Jul 17, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+17 -0 violations, +0 -0 fixes in 8 projects; 46 projects unchanged)

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

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

+ tests/providers/amazon/aws/executors/batch/test_batch_executor.py:653:17: B909 Mutation to loop iterable `os.environ` during iteration

bokeh/bokeh (+1 -0 violations, +0 -0 fixes)

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

+ src/bokeh/sphinxext/bokeh_sampledata_xref.py:206:36: B909 Mutation to loop iterable `refs` during iteration

milvus-io/pymilvus (+4 -0 violations, +0 -0 fixes)

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

+ examples/example_bulkwriter.py:371:17: B909 Mutation to loop iterable `task_ids` during iteration
+ examples/example_bulkwriter.py:374:17: B909 Mutation to loop iterable `task_ids` during iteration
+ pymilvus/orm/prepare.py:67:17: B909 Mutation to loop iterable `tmp_fields` during iteration
+ pymilvus/orm/schema.py:524:13: B909 Mutation to loop iterable `tmp_fields` during iteration

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

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

+ tests/conftest.py:304:17: B909 Mutation to loop iterable `os.environ` during iteration

rotki/rotki (+2 -0 violations, +0 -0 fixes)

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

+ rotkehlchen/api/rest.py:526:25: B909 Mutation to loop iterable `self.rotkehlchen.api_task_greenlets` during iteration
+ rotkehlchen/chain/evm/decoding/decoder.py:919:21: B909 Mutation to loop iterable `action_items` during iteration

zulip/zulip (+1 -0 violations, +0 -0 fixes)

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

+ zerver/lib/events.py:922:21: B909 Mutation to loop iterable during iteration

indico/indico (+5 -0 violations, +0 -0 fixes)

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

+ indico/modules/events/models/persons.py:330:17: B909 Mutation to loop iterable `other.event_links` during iteration
+ indico/modules/events/models/persons.py:342:17: B909 Mutation to loop iterable `other.abstract_links` during iteration
+ indico/modules/events/models/persons.py:354:17: B909 Mutation to loop iterable `other.contribution_links` during iteration
+ indico/modules/events/models/persons.py:363:17: B909 Mutation to loop iterable `other.subcontribution_links` during iteration
+ indico/modules/events/models/persons.py:372:17: B909 Mutation to loop iterable `other.session_block_links` during iteration

pytest-dev/pytest (+2 -0 violations, +0 -0 fixes)

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

+ src/_pytest/pytester.py:310:17: B909 Mutation to loop iterable `self.calls` during iteration
+ src/_pytest/recwarn.py:214:24: B909 Mutation to loop iterable `self._list` during iteration

Changes by rule (1 rules affected)

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

@charliermarsh
Copy link
Member Author

@carljm - Would you mind taking a look at the ecosystem checks and judging whether they're false positives?

@carljm
Copy link
Contributor

carljm commented Jul 17, 2024

The airflow and poetry cases would technically be introducing a false positive; their code works fine. The reason is that os.environ is not actually a dict, it's an instance of an _Environ class that has different behavior: its __iter__ method captures the current set of keys as a list snapshot, and iterates that list. So calling .pop() during iteration doesn't change the list you are actually iterating over. Though this is a bit subtle: what airflow is doing is OK because they pop the current element; if they were to pop some other key, that key could still show up later in the iteration (because it's still in the list snapshot of keys that was taken before iteration), and that could cause issues. Personally, I'd be comfortable with issuing this "false" positive: os.environ is meant to act like a dict, and I think it's reasonable (and not hard) to just avoid this iffy pattern.

The pymilvus case looks like buggy code to me that will cause some items to be skipped in iteration (not to mention being quadratic-time.) The lint should fire on this.

I can't easily evaluate the indico case because I don't know SQLAlchemy well enough to know what data structure it uses for db relationship backrefs, which is what it looks like they are iterating over there, and what iteration behavior that data structure has. The code might be buggy, or it might be more like the os.environ case, depending how exactly that data structure handles iteration. I would certainly be more comfortable with that code (and it would likely be a lot more efficient) if it instead collected a list of things to remove and then filtered them in a second pass.

On the whole, I don't personally feel like these ecosystem results are a convincing reason not to broaden the rule. The patterns caught by this expanded rule are not safe in the common case of builtin data structures. They might be safe, in some cases, for some custom data structures that implement iteration differently.

@charliermarsh
Copy link
Member Author

Awesome, thank you!

@charliermarsh charliermarsh enabled auto-merge (squash) July 17, 2024 17:38
@charliermarsh charliermarsh merged commit 1435b0f into main Jul 17, 2024
18 checks passed
@charliermarsh charliermarsh deleted the charlie/B909 branch July 17, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule B909 (loop-iterator-mutation) doesn't detect many kinds of list mutation
2 participants