-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Generalize PIE807 to handle dict literals (#8608)
## Summary
PIE807 will rewrite `lambda: []` to `list` -- AFAICT though, the same
rationale also applies to dicts, so I've modified the code to also
rewrite `lambda: {}` to `dict`.
Two things I'm not sure about:
* Should this go to a new rule? This no longer actually matches the
behavior of flake8-pie, and while I think thematically it makes sense to
be part of the same rule, we could make it a standalone rule (but if so,
where should I put it and what error code should I use)?
* If we want a single rule, are there backwards compatibility concerns
with the rule name change (from `reimplemented_list_builtin` to
`reimplemented_container_builtin`?
## Test Plan
Added snapshot tests of the functionality.- Loading branch information
Showing
9 changed files
with
304 additions
and
111 deletions.
There are no files selected for viewing
16 changes: 13 additions & 3 deletions
16
crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE807.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,37 @@ | ||
| @dataclass | ||
| class Foo: | ||
| foo: List[str] = field(default_factory=lambda: []) # PIE807 | ||
| bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807 | ||
|
|
||
|
|
||
| class FooTable(BaseTable): | ||
| bar = fields.ListField(default=lambda: []) # PIE807 | ||
| foo = fields.ListField(default=lambda: []) # PIE807 | ||
| bar = fields.ListField(default=lambda: {}) # PIE807 | ||
|
|
||
|
|
||
| class FooTable(BaseTable): | ||
| bar = fields.ListField(lambda: []) # PIE807 | ||
| foo = fields.ListField(lambda: []) # PIE807 | ||
| bar = fields.ListField(default=lambda: {}) # PIE807 | ||
|
|
||
|
|
||
| @dataclass | ||
| class Foo: | ||
| foo: List[str] = field(default_factory=list) | ||
| bar: Dict[str, int] = field(default_factory=dict) | ||
|
|
||
|
|
||
| class FooTable(BaseTable): | ||
| bar = fields.ListField(list) | ||
| foo = fields.ListField(list) | ||
| bar = fields.ListField(dict) | ||
|
|
||
|
|
||
| lambda *args, **kwargs: [] | ||
| lambda *args, **kwargs: {} | ||
|
|
||
| lambda *args: [] | ||
| lambda *args: {} | ||
|
|
||
| lambda **kwargs: [] | ||
| lambda **kwargs: {} | ||
|
|
||
| lambda: {**unwrap} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
119 changes: 119 additions & 0 deletions
119
crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_container_builtin.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| use ruff_python_ast::{self as ast, Expr, ExprLambda}; | ||
|
|
||
| use ruff_diagnostics::{Diagnostic, Edit, Fix}; | ||
| use ruff_diagnostics::{FixAvailability, Violation}; | ||
| use ruff_macros::{derive_message_formats, violation}; | ||
| use ruff_text_size::Ranged; | ||
|
|
||
| use crate::checkers::ast::Checker; | ||
|
|
||
| /// ## What it does | ||
| /// Checks for lambdas that can be replaced with the `list` builtin. | ||
| /// | ||
| /// In [preview], this rule will also flag lambdas that can be replaced with | ||
| /// the `dict` builtin. | ||
| /// | ||
| /// ## Why is this bad? | ||
| /// Using container builtins are more succinct and idiomatic than wrapping | ||
| /// the literal in a lambda. | ||
| /// | ||
| /// ## Example | ||
| /// ```python | ||
| /// from dataclasses import dataclass, field | ||
| /// | ||
| /// | ||
| /// @dataclass | ||
| /// class Foo: | ||
| /// bar: list[int] = field(default_factory=lambda: []) | ||
| /// ``` | ||
| /// | ||
| /// Use instead: | ||
| /// ```python | ||
| /// from dataclasses import dataclass, field | ||
| /// | ||
| /// | ||
| /// @dataclass | ||
| /// class Foo: | ||
| /// bar: list[int] = field(default_factory=list) | ||
| /// baz: dict[str, int] = field(default_factory=dict) | ||
| /// ``` | ||
| /// | ||
| /// ## References | ||
| /// - [Python documentation: `list`](https://docs.python.org/3/library/functions.html#func-list) | ||
| /// | ||
| /// [preview]: https://docs.astral.sh/ruff/preview/ | ||
| #[violation] | ||
| pub struct ReimplementedContainerBuiltin { | ||
| container: Container, | ||
| } | ||
|
|
||
| impl Violation for ReimplementedContainerBuiltin { | ||
| const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; | ||
|
|
||
| #[derive_message_formats] | ||
| fn message(&self) -> String { | ||
| let Self { container } = self; | ||
| format!("Prefer `{container}` over useless lambda") | ||
| } | ||
|
|
||
| fn fix_title(&self) -> Option<String> { | ||
| let Self { container } = self; | ||
| Some(format!("Replace with `lambda` with `{container}`")) | ||
| } | ||
| } | ||
|
|
||
| /// PIE807 | ||
| pub(crate) fn reimplemented_container_builtin(checker: &mut Checker, expr: &ExprLambda) { | ||
| let ExprLambda { | ||
| parameters, | ||
| body, | ||
| range: _, | ||
| } = expr; | ||
|
|
||
| if parameters.is_none() { | ||
| let container = match body.as_ref() { | ||
| Expr::List(ast::ExprList { elts, .. }) if elts.is_empty() => Some(Container::List), | ||
| Expr::Dict(ast::ExprDict { values, .. }) | ||
| if values.is_empty() & checker.settings.preview.is_enabled() => | ||
| { | ||
| Some(Container::Dict) | ||
| } | ||
| _ => None, | ||
| }; | ||
| if let Some(container) = container { | ||
| let mut diagnostic = | ||
| Diagnostic::new(ReimplementedContainerBuiltin { container }, expr.range()); | ||
| if checker.semantic().is_builtin(container.as_str()) { | ||
| diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( | ||
| container.to_string(), | ||
| expr.range(), | ||
| ))); | ||
| } | ||
| checker.diagnostics.push(diagnostic); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| enum Container { | ||
| List, | ||
| Dict, | ||
| } | ||
|
|
||
| impl Container { | ||
| fn as_str(self) -> &'static str { | ||
| match self { | ||
| Container::List => "list", | ||
| Container::Dict => "dict", | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for Container { | ||
| fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| match self { | ||
| Container::List => fmt.write_str("list"), | ||
| Container::Dict => fmt.write_str("dict"), | ||
| } | ||
| } | ||
| } |
76 changes: 0 additions & 76 deletions
76
crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_list_builtin.rs
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.