[flake8-comprehensions] Fix false positive for C420 with attribute, subscript, or slice assignment targets#19513
Conversation
|
ntBre
added
bug
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks. I think the fix is correct here, but I don't think the new test cases actually trigger the rule.
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!
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Very nice! I was hoping we had a utility for this, but I didn't know about any_over_expr off the top of my head. Nice work!
I had one more tiny nit about test locations, but this is good to go otherwise.
There was a problem hiding this comment.
Should we move these to C420_3.py?
| } | ||
|
|
||
| // 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
danparizher
changed the title
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) ...
2 participants
Summary
Fixes #19511