[flake8-comprehensions] Fix false positive for C420 with attribute, subscript, or slice assignment targets
#19513
Conversation
|
crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C420_3.py
Outdated
Show resolved
Hide resolved
..._linter/src/rules/flake8_comprehensions/rules/unnecessary_dict_comprehension_for_iterable.rs
Outdated
Show resolved
Hide resolved
| if matches!( | ||
| generator.target, | ||
| Expr::Attribute(_) | Expr::Subscript(_) | Expr::Slice(_) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I didn't realize this initially, but we may need a simple Visitor here. This only checks the top-level expression, but the problem persists if the attribute, subscript, or slice is a sub-expression. As a simple example, we can embed the C.a case in a tuple:
class C: a = None
{(C.a,): None for (C.a,) in "abc"}
print(C.a)and it still prints c, but the target expression itself is no longer an attribute.
I feel like top-level slices and attributes should already be pretty rare, so I could probably be convinced that the cost of a Visitor isn't worth it, but we should at least document our decision and add a known-false-positive test case if we decide not to go for it.
There was a problem hiding this comment.
I'm curious to see how much it would cost. Going to commit to take a look at the performance, feel free to revert if you think it isn't worth it 🙂
There was a problem hiding this comment.
Benchmarks look good, no change!
There was a problem hiding this comment.
Should we move these to C420_3.py?
| @@ -96,6 +96,12 @@ pub(crate) fn unnecessary_dict_comprehension_for_iterable( | |||
| return; | |||
| } | |||
|
|
|||
| // Don't suggest `dict.fromkeys` if the target contains side-effecting expressions | |||
| // (attributes, subscripts, or slices) at the top level. | |||
There was a problem hiding this comment.
| // (attributes, subscripts, or slices) at the top level. | |
| // (attributes, subscripts, or slices). |
One more nit, I guess :) I think "at the top level" was only true before
flake8_comprehensions] Fix false positive for C420 with attribute, subscript, or slice assignment targetsflake8-comprehensions] Fix false positive for C420 with attribute, subscript, or slice assignment targets
* main: (31 commits) Add AIR301 rule (#17707) Avoid underflow in default ranges before a BOM (#19839) Update actions/download-artifact digest to de96f46 (#19852) Update docker/login-action action to v3.5.0 (#19860) Update rui314/setup-mold digest to 7344740 (#19853) Update cargo-bins/cargo-binstall action to v1.14.4 (#19855) Update actions/cache action to v4.2.4 (#19854) Update Rust crate hashbrown to v0.15.5 (#19858) Update Rust crate camino to v1.1.11 (#19857) Update Rust crate proc-macro2 to v1.0.96 (#19859) Update dependency ruff to v0.12.8 (#19856) SIM905: Fix handling of U+001C..U+001F whitespace (#19849) RUF064: offer a safe fix for multi-digit zeros (#19847) Clean up unused rendering code in `ruff_linter` (#19832) [ty] Add Salsa caching to `TupleType::to_class_type` (#19840) [ty] Handle cycles when finding implicit attributes (#19833) [ty] fix goto-definition on imports (#19834) [ty] Implement stdlib stub mapping (#19529) [`flake8-comprehensions`] Fix false positive for `C420` with attribute, subscript, or slice assignment targets (#19513) [ty] Implement module-level `__getattr__` support (#19791) ...
Summary
Fixes #19511