mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-03 18:28:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Generalize PIE807 to handle dict literals (#8608)

Browse source 
## 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.
This commit is contained in:
Alan Du 2023-11-13 12:55:17 -05:00 • committed by GitHub
parent d574fcd1ac
commit 6f23bdb78f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 304 additions and 111 deletions
Download patch file Download diff file Expand all files Collapse all files

16
crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE807.py vendored
View file

@ -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}

4
crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs
View file

@ -18,8 +18,8 @@ pub(crate) fn deferred_lambdas(checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryLambda) {
pylint::rules::unnecessary_lambda(checker, lambda);
}
if checker.enabled(Rule::ReimplementedListBuiltin) {
flake8_pie::rules::reimplemented_list_builtin(checker, lambda);
if checker.enabled(Rule::ReimplementedContainerBuiltin) {
flake8_pie::rules::reimplemented_container_builtin(checker, lambda);
}
}
}

2
crates/ruff_linter/src/codes.rs
View file

@ -773,7 +773,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pie, "796") => (RuleGroup::Stable, rules::flake8_pie::rules::NonUniqueEnums),
(Flake8Pie, "800") => (RuleGroup::Stable, rules::flake8_pie::rules::UnnecessarySpread),
(Flake8Pie, "804") => (RuleGroup::Stable, rules::flake8_pie::rules::UnnecessaryDictKwargs),
(Flake8Pie, "807") => (RuleGroup::Stable, rules::flake8_pie::rules::ReimplementedListBuiltin),
(Flake8Pie, "807") => (RuleGroup::Stable, rules::flake8_pie::rules::ReimplementedContainerBuiltin),
(Flake8Pie, "808") => (RuleGroup::Stable, rules::flake8_pie::rules::UnnecessaryRangeStart),
(Flake8Pie, "810") => (RuleGroup::Stable, rules::flake8_pie::rules::MultipleStartsEndsWith),

21
crates/ruff_linter/src/rules/flake8_pie/mod.rs
View file

@ -9,6 +9,7 @@ mod tests {
use test_case::test_case;
use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::{assert_messages, settings};
@ -18,7 +19,7 @@ mod tests {
#[test_case(Rule::UnnecessaryRangeStart, Path::new("PIE808.py"))]
#[test_case(Rule::UnnecessaryPass, Path::new("PIE790.py"))]
#[test_case(Rule::UnnecessarySpread, Path::new("PIE800.py"))]
#[test_case(Rule::ReimplementedListBuiltin, Path::new("PIE807.py"))]
#[test_case(Rule::ReimplementedContainerBuiltin, Path::new("PIE807.py"))]
#[test_case(Rule::NonUniqueEnums, Path::new("PIE796.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
@ -29,4 +30,22 @@ mod tests {
assert_messages!(snapshot, diagnostics);
Ok(())
}
#[test_case(Rule::ReimplementedContainerBuiltin, Path::new("PIE807.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("flake8_pie").join(path).as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
}

4
crates/ruff_linter/src/rules/flake8_pie/rules/mod.rs
View file

@ -1,7 +1,7 @@
pub(crate) use duplicate_class_field_definition::*;
pub(crate) use multiple_starts_ends_with::*;
pub(crate) use non_unique_enums::*;
pub(crate) use reimplemented_list_builtin::*;
pub(crate) use reimplemented_container_builtin::*;
pub(crate) use unnecessary_dict_kwargs::*;
pub(crate) use unnecessary_pass::*;
pub(crate) use unnecessary_range_start::*;
@ -10,7 +10,7 @@ pub(crate) use unnecessary_spread::*;
mod duplicate_class_field_definition;
mod multiple_starts_ends_with;
mod non_unique_enums;
mod reimplemented_list_builtin;
mod reimplemented_container_builtin;
mod unnecessary_dict_kwargs;
mod unnecessary_pass;
mod unnecessary_range_start;

119
crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_container_builtin.rs Normal file
View file

@ -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
crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_list_builtin.rs
View file

@ -1,76 +0,0 @@
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.
///
/// ## Why is this bad?
/// Using `list` builtin is more readable.
///
/// ## 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)
/// ```
///
/// ## References
/// - [Python documentation: `list`](https://docs.python.org/3/library/functions.html#func-list)
#[violation]
pub struct ReimplementedListBuiltin;
impl Violation for ReimplementedListBuiltin {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
format!("Prefer `list` over useless lambda")
}
fn fix_title(&self) -> Option<String> {
Some("Replace with `list`".to_string())
}
}
/// PIE807
pub(crate) fn reimplemented_list_builtin(checker: &mut Checker, expr: &ExprLambda) {
let ExprLambda {
parameters,
body,
range: _,
} = expr;
if parameters.is_none() {
if let Expr::List(ast::ExprList { elts, .. }) = body.as_ref() {
if elts.is_empty() {
let mut diagnostic = Diagnostic::new(ReimplementedListBuiltin, expr.range());
if checker.semantic().is_builtin("list") {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
"list".to_string(),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
}
}

55
crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE807_PIE807.py.snap
View file

@ -7,52 +7,55 @@ PIE807.py:3:44: PIE807 [*] Prefer `list` over useless lambda
2 | class Foo:
3 | foo: List[str] = field(default_factory=lambda: []) # PIE807
| ^^^^^^^^^^ PIE807
4 | bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807
|
= help: Replace with `list`
= help: Replace with `lambda` with `list`
ℹ Safe fix
1 1 | @dataclass
2 2 | class Foo:
3 |- foo: List[str] = field(default_factory=lambda: []) # PIE807
3 |+ foo: List[str] = field(default_factory=list) # PIE807
4 4 |
4 4 | bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807
5 5 |
6 6 | class FooTable(BaseTable):
6 6 |
PIE807.py:7:36: PIE807 [*] Prefer `list` over useless lambda
PIE807.py:8:36: PIE807 [*] Prefer `list` over useless lambda
|
6 | class FooTable(BaseTable):
7 | bar = fields.ListField(default=lambda: []) # PIE807
7 | class FooTable(BaseTable):
8 | foo = fields.ListField(default=lambda: []) # PIE807
| ^^^^^^^^^^ PIE807
9 | bar = fields.ListField(default=lambda: {}) # PIE807
|
= help: Replace with `list`
= help: Replace with `lambda` with `list`
ℹ Safe fix
4 4 |
5 5 |
6 6 | class FooTable(BaseTable):
7 |- bar = fields.ListField(default=lambda: []) # PIE807
7 |+ bar = fields.ListField(default=list) # PIE807
8 8 |
9 9 |
10 10 | class FooTable(BaseTable):
6 6 |
7 7 | class FooTable(BaseTable):
8 |- foo = fields.ListField(default=lambda: []) # PIE807
8 |+ foo = fields.ListField(default=list) # PIE807
9 9 | bar = fields.ListField(default=lambda: {}) # PIE807
10 10 |
11 11 |
PIE807.py:11:28: PIE807 [*] Prefer `list` over useless lambda
PIE807.py:13:28: PIE807 [*] Prefer `list` over useless lambda
|
10 | class FooTable(BaseTable):
11 | bar = fields.ListField(lambda: []) # PIE807
12 | class FooTable(BaseTable):
13 | foo = fields.ListField(lambda: []) # PIE807
| ^^^^^^^^^^ PIE807
14 | bar = fields.ListField(default=lambda: {}) # PIE807
|
= help: Replace with `list`
= help: Replace with `lambda` with `list`
ℹ Safe fix
8 8 |
9 9 |
10 10 | class FooTable(BaseTable):
11 |- bar = fields.ListField(lambda: []) # PIE807
11 |+ bar = fields.ListField(list) # PIE807
12 12 |
13 13 |
14 14 | @dataclass
10 10 |
11 11 |
12 12 | class FooTable(BaseTable):
13 |- foo = fields.ListField(lambda: []) # PIE807
13 |+ foo = fields.ListField(list) # PIE807
14 14 | bar = fields.ListField(default=lambda: {}) # PIE807
15 15 |
16 16 |

118
crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__preview__PIE807_PIE807.py.snap Normal file
View file

@ -0,0 +1,118 @@
---
source: crates/ruff_linter/src/rules/flake8_pie/mod.rs
---
PIE807.py:3:44: PIE807 [*] Prefer `list` over useless lambda
|
1 | @dataclass
2 | class Foo:
3 | foo: List[str] = field(default_factory=lambda: []) # PIE807
| ^^^^^^^^^^ PIE807
4 | bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807
|
= help: Replace with `lambda` with `list`
ℹ Safe fix
1 1 | @dataclass
2 2 | class Foo:
3 |- foo: List[str] = field(default_factory=lambda: []) # PIE807
3 |+ foo: List[str] = field(default_factory=list) # PIE807
4 4 | bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807
5 5 |
6 6 |
PIE807.py:4:49: PIE807 [*] Prefer `dict` over useless lambda
|
2 | class Foo:
3 | foo: List[str] = field(default_factory=lambda: []) # PIE807
4 | bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807
| ^^^^^^^^^^ PIE807
|
= help: Replace with `lambda` with `dict`
ℹ Safe fix
1 1 | @dataclass
2 2 | class Foo:
3 3 | foo: List[str] = field(default_factory=lambda: []) # PIE807
4 |- bar: Dict[str, int] = field(default_factory=lambda: {}) # PIE807
4 |+ bar: Dict[str, int] = field(default_factory=dict) # PIE807
5 5 |
6 6 |
7 7 | class FooTable(BaseTable):
PIE807.py:8:36: PIE807 [*] Prefer `list` over useless lambda
|
7 | class FooTable(BaseTable):
8 | foo = fields.ListField(default=lambda: []) # PIE807
| ^^^^^^^^^^ PIE807
9 | bar = fields.ListField(default=lambda: {}) # PIE807
|
= help: Replace with `lambda` with `list`
ℹ Safe fix
5 5 |
6 6 |
7 7 | class FooTable(BaseTable):
8 |- foo = fields.ListField(default=lambda: []) # PIE807
8 |+ foo = fields.ListField(default=list) # PIE807
9 9 | bar = fields.ListField(default=lambda: {}) # PIE807
10 10 |
11 11 |
PIE807.py:9:36: PIE807 [*] Prefer `dict` over useless lambda
|
7 | class FooTable(BaseTable):
8 | foo = fields.ListField(default=lambda: []) # PIE807
9 | bar = fields.ListField(default=lambda: {}) # PIE807
| ^^^^^^^^^^ PIE807
|
= help: Replace with `lambda` with `dict`
ℹ Safe fix
6 6 |
7 7 | class FooTable(BaseTable):
8 8 | foo = fields.ListField(default=lambda: []) # PIE807
9 |- bar = fields.ListField(default=lambda: {}) # PIE807
9 |+ bar = fields.ListField(default=dict) # PIE807
10 10 |
11 11 |
12 12 | class FooTable(BaseTable):
PIE807.py:13:28: PIE807 [*] Prefer `list` over useless lambda
|
12 | class FooTable(BaseTable):
13 | foo = fields.ListField(lambda: []) # PIE807
| ^^^^^^^^^^ PIE807
14 | bar = fields.ListField(default=lambda: {}) # PIE807
|
= help: Replace with `lambda` with `list`
ℹ Safe fix
10 10 |
11 11 |
12 12 | class FooTable(BaseTable):
13 |- foo = fields.ListField(lambda: []) # PIE807
13 |+ foo = fields.ListField(list) # PIE807
14 14 | bar = fields.ListField(default=lambda: {}) # PIE807
15 15 |
16 16 |
PIE807.py:14:36: PIE807 [*] Prefer `dict` over useless lambda
|
12 | class FooTable(BaseTable):
13 | foo = fields.ListField(lambda: []) # PIE807
14 | bar = fields.ListField(default=lambda: {}) # PIE807
| ^^^^^^^^^^ PIE807
|
= help: Replace with `lambda` with `dict`
ℹ Safe fix
11 11 |
12 12 | class FooTable(BaseTable):
13 13 | foo = fields.ListField(lambda: []) # PIE807
14 |- bar = fields.ListField(default=lambda: {}) # PIE807
14 |+ bar = fields.ListField(default=dict) # PIE807
15 15 |
16 16 |
17 17 | @dataclass