Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize PIE807 to handle dict literals #8608

Merged
merged 7 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE807.py
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}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,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),

Expand Down
21 changes: 20 additions & 1 deletion crates/ruff_linter/src/rules/flake8_pie/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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());
Expand All @@ -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 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_pie/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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::*;
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
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.
/// If [preview] mode is enabled, then we will also look for lambdas
/// that can be replaced with the `dict` builtin.
///
/// ## Why is this bad?
/// Using container builtins 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)
/// baz: dict[str, int] = field(default_factory=dict)
/// ```
///
///
/// If [preview]
/// ## 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(&'static str);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could make this an enum instead, but not sure it's worthwhile compared to just storing the builtin inline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to make it an enum to follow the pattern we use elsewhere, even though it's really unimportant.


impl Violation for ReimplementedContainerBuiltin {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Prefer `{}` over useless lambda", self.0)
}

fn fix_title(&self) -> Option<String> {
Some(format!("Replace with lambda with `{}`", self.0))
}
}

/// PIE807
pub(crate) fn reimplemented_container_builtin(checker: &mut Checker, expr: &ExprLambda) {
let ExprLambda {
parameters,
body,
range: _,
} = expr;

if parameters.is_none() {
let builtin = match body.as_ref() {
Expr::List(ast::ExprList { elts, .. }) if elts.is_empty() => Some("list"),
Expr::Dict(ast::ExprDict { values, .. })
if values.is_empty() & checker.settings.preview.is_enabled() =>
{
Some("dict")
}
_ => None,
};
if let Some(builtin) = builtin {
let mut diagnostic =
Diagnostic::new(ReimplementedContainerBuiltin(builtin), expr.range());
if checker.semantic().is_builtin(builtin) {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
builtin.to_string(),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 |


Loading
Loading