[refurb] Make list-reverse-copy an unsafe fix
#12303
Conversation
charliermarsh
changed the title
list-reverse-copy an unsafe fixrefurb] Make list-reverse-copy an unsafe fix
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| FURB187 | 2 | 0 | 0 | 0 | 2 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +0 -2 fixes in 1 projects; 53 projects unchanged)
apache/airflow (+0 -0 violations, +0 -2 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ airflow/jobs/backfill_job_runner.py:943:13: FURB187 Use of assignment of `reversed` on list `dagrun_infos` - airflow/jobs/backfill_job_runner.py:943:13: FURB187 [*] Use of assignment of `reversed` on list `dagrun_infos`
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| FURB187 | 2 | 0 | 0 | 0 | 2 |
There was a problem hiding this comment.
Fair enough. Do we have other rules that apply fixes that mutate an object in place? These would also need to be marked as unsafe. It's not that we have to do this now. I'm mainly trying to understand the implication of marking such fixes as unsafe.
We might be able to get better at this long-term with Red Knot, if it has a way to reason whether a binding is local (not an argument), and has no aliases (CC: @carljm )
|
Yeah, it's possible that we could make this safe in some cases. Maybe if... there's only one binding, and there are no reads yet, and that binding is an assignment...? |
|
Yes, it should be possible to find the cases where this is safe. They might turn out to not be very common in practice. I personally just don't like this lint rule at all, because in-place mutation is just fundamentally different, and that difference will be relevant in many practical cases. But making the autofix unsafe seems like a reasonable option. |
Summary
I don't know that there's more to do here. We could consider not raising the violation at all for arguments, but that would have some false negatives and could also be surprising to users.
Closes #12267.