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

[perflint] Add PERF101 with autofix #5121

Merged
merged 7 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
52 changes: 52 additions & 0 deletions crates/ruff/resources/test/fixtures/perflint/PERF101.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
foo_tuple = (1, 2, 3)
foo_list = [1, 2, 3]
foo_set = {1, 2, 3}
foo_dict = {1: 2, 3: 4}
foo_int = 123

for i in list(foo_tuple): # PERF101
pass

for i in list(foo_list): # PERF101
pass

for i in list(foo_set): # PERF101
pass

for i in list((1, 2, 3)): # PERF101
pass

for i in list([1, 2, 3]): # PERF101
pass

for i in list({1, 2, 3}): # PERF101
pass

for i in list(
{
1,
2,
3,
}
):
pass

for i in list( # Comment
{1, 2, 3}
): # PERF101
pass

for i in list(foo_dict): # Ok
pass

for i in list(1): # Ok
pass

for i in list(foo_int): # Ok
pass


import itertools

for i in itertools.product(foo_int): # Ok
pass
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1481,6 +1481,9 @@ where
if self.enabled(Rule::IncorrectDictIterator) {
perflint::rules::incorrect_dict_iterator(self, target, iter);
}
if self.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(self, iter);
}
}
Stmt::Try(ast::StmtTry {
body,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Airflow, "001") => (RuleGroup::Unspecified, rules::airflow::rules::AirflowVariableNameTaskIdMismatch),

// perflint
(Perflint, "101") => (RuleGroup::Unspecified, rules::perflint::rules::UnnecessaryListCast),
(Perflint, "102") => (RuleGroup::Unspecified, rules::perflint::rules::IncorrectDictIterator),

// flake8-fixme
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/perflint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ mod tests {
use crate::settings::Settings;
use crate::test::test_path;

#[test_case(Rule::UnnecessaryListCast, Path::new("PERF101.py"))]
#[test_case(Rule::IncorrectDictIterator, Path::new("PERF102.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff/src/rules/perflint/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
pub(crate) use incorrect_dict_iterator::{incorrect_dict_iterator, IncorrectDictIterator};
pub(crate) use incorrect_dict_iterator::*;
pub(crate) use unnecessary_list_cast::*;

mod incorrect_dict_iterator;
mod unnecessary_list_cast;
129 changes: 129 additions & 0 deletions crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
use ruff_text_size::TextRange;
use rustpython_parser::ast::{self, Expr};

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::prelude::Stmt;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

/// ## What it does
/// Checks for explicit casts to `list` on for-loop iterables.
///
/// ## Why is this bad?
/// Using a `list()` call to eagerly iterate over an already-iterable type
/// (like a tuple, list, or set) is inefficient, as it forces Python to create
/// a new list unnecessarily.
///
/// Removing the `list()` call will not change the behavior of the code, but
/// may improve performance.
///
/// ## Example
/// ```python
/// items = (1, 2, 3)
/// for i in list(items):
/// print(i)
/// ```
///
/// Use instead:
/// ```python
/// items = (1, 2, 3)
/// for i in items:
/// print(i)
/// ```
#[violation]
pub struct UnnecessaryListCast;

impl AlwaysAutofixableViolation for UnnecessaryListCast {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not cast an iterable to `list` before iterating over it")
}

fn autofix_title(&self) -> String {
format!("Remove `list()` cast")
}
}

/// PERF101
pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) {
let Expr::Call(ast::ExprCall{ func, args, range: list_range, ..}) = iter else {
return;
};

if args.len() != 1 {
return;
}

let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else{
return;
};

if !(id == "list" && checker.semantic().is_builtin("list")) {
return;
}

match &args[0] {
Expr::Tuple(ast::ExprTuple {
range: iterable_range,
..
})
| Expr::List(ast::ExprList {
range: iterable_range,
..
})
| Expr::Set(ast::ExprSet {
range: iterable_range,
..
}) => {
let mut diagnostic = Diagnostic::new(UnnecessaryListCast, *list_range);
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(remove_cast(*list_range, *iterable_range));
}
checker.diagnostics.push(diagnostic);
}
Expr::Name(ast::ExprName {
id,
range: iterable_range,
..
}) => {
let scope = checker.semantic().scope();
if let Some(binding_id) = scope.get(id) {
let binding = checker.semantic().binding(binding_id);
if binding.kind.is_assignment() || binding.kind.is_named_expr_assignment() {
if let Some(parent_id) = binding.source {
let parent = checker.semantic().stmts[parent_id];
if let Stmt::Assign(ast::StmtAssign { value, .. })
| Stmt::AnnAssign(ast::StmtAnnAssign {
value: Some(value), ..
})
| Stmt::AugAssign(ast::StmtAugAssign { value, .. }) = parent
{
if matches!(
value.as_ref(),
Expr::Tuple(_) | Expr::List(_) | Expr::Set(_)
) {
let mut diagnostic =
Diagnostic::new(UnnecessaryListCast, *list_range);
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(remove_cast(*list_range, *iterable_range));
}
checker.diagnostics.push(diagnostic);
}
}
}
}
}
}
_ => {}
}
}

/// Generate a [`Fix`] to remove a `list` cast from an expression.
fn remove_cast(list_range: TextRange, iterable_range: TextRange) -> Fix {
Fix::automatic_edits(
Edit::deletion(list_range.start(), iterable_range.start()),
[Edit::deletion(iterable_range.end(), list_range.end())],
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
---
source: crates/ruff/src/rules/perflint/mod.rs
---
PERF101.py:7:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
5 | foo_int = 123
6 |
7 | for i in list(foo_tuple): # PERF101
| ^^^^^^^^^^^^^^^ PERF101
8 | pass
|
= help: Remove `list()` cast

ℹ Fix
4 4 | foo_dict = {1: 2, 3: 4}
5 5 | foo_int = 123
6 6 |
7 |-for i in list(foo_tuple): # PERF101
7 |+for i in foo_tuple: # PERF101
8 8 | pass
9 9 |
10 10 | for i in list(foo_list): # PERF101

PERF101.py:10:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
8 | pass
9 |
10 | for i in list(foo_list): # PERF101
| ^^^^^^^^^^^^^^ PERF101
11 | pass
|
= help: Remove `list()` cast

ℹ Fix
7 7 | for i in list(foo_tuple): # PERF101
8 8 | pass
9 9 |
10 |-for i in list(foo_list): # PERF101
10 |+for i in foo_list: # PERF101
11 11 | pass
12 12 |
13 13 | for i in list(foo_set): # PERF101

PERF101.py:13:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
11 | pass
12 |
13 | for i in list(foo_set): # PERF101
| ^^^^^^^^^^^^^ PERF101
14 | pass
|
= help: Remove `list()` cast

ℹ Fix
10 10 | for i in list(foo_list): # PERF101
11 11 | pass
12 12 |
13 |-for i in list(foo_set): # PERF101
13 |+for i in foo_set: # PERF101
14 14 | pass
15 15 |
16 16 | for i in list((1, 2, 3)): # PERF101

PERF101.py:16:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
14 | pass
15 |
16 | for i in list((1, 2, 3)): # PERF101
| ^^^^^^^^^^^^^^^ PERF101
17 | pass
|
= help: Remove `list()` cast

ℹ Fix
13 13 | for i in list(foo_set): # PERF101
14 14 | pass
15 15 |
16 |-for i in list((1, 2, 3)): # PERF101
16 |+for i in (1, 2, 3): # PERF101
17 17 | pass
18 18 |
19 19 | for i in list([1, 2, 3]): # PERF101

PERF101.py:19:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
17 | pass
18 |
19 | for i in list([1, 2, 3]): # PERF101
| ^^^^^^^^^^^^^^^ PERF101
20 | pass
|
= help: Remove `list()` cast

ℹ Fix
16 16 | for i in list((1, 2, 3)): # PERF101
17 17 | pass
18 18 |
19 |-for i in list([1, 2, 3]): # PERF101
19 |+for i in [1, 2, 3]: # PERF101
20 20 | pass
21 21 |
22 22 | for i in list({1, 2, 3}): # PERF101

PERF101.py:22:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
20 | pass
21 |
22 | for i in list({1, 2, 3}): # PERF101
| ^^^^^^^^^^^^^^^ PERF101
23 | pass
|
= help: Remove `list()` cast

ℹ Fix
19 19 | for i in list([1, 2, 3]): # PERF101
20 20 | pass
21 21 |
22 |-for i in list({1, 2, 3}): # PERF101
22 |+for i in {1, 2, 3}: # PERF101
23 23 | pass
24 24 |
25 25 | for i in list(

PERF101.py:25:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
23 | pass
24 |
25 | for i in list(
| __________^
26 | | {
27 | | 1,
28 | | 2,
29 | | 3,
30 | | }
31 | | ):
| |_^ PERF101
32 | pass
|
= help: Remove `list()` cast

ℹ Fix
22 22 | for i in list({1, 2, 3}): # PERF101
23 23 | pass
24 24 |
25 |-for i in list(
26 |- {
25 |+for i in {
27 26 | 1,
28 27 | 2,
29 28 | 3,
30 |- }
31 |-):
29 |+ }:
32 30 | pass
33 31 |
34 32 | for i in list( # Comment

PERF101.py:34:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
32 | pass
33 |
34 | for i in list( # Comment
| __________^
35 | | {1, 2, 3}
36 | | ): # PERF101
| |_^ PERF101
37 | pass
|
= help: Remove `list()` cast

ℹ Fix
31 31 | ):
32 32 | pass
33 33 |
34 |-for i in list( # Comment
35 |- {1, 2, 3}
36 |-): # PERF101
34 |+for i in {1, 2, 3}: # PERF101
37 35 | pass
38 36 |
39 37 | for i in list(foo_dict): # Ok


Loading
Loading