Skip to content

Commit

Permalink
[perflint] Add PERF101 with autofix (#5121)
Browse files Browse the repository at this point in the history
## Summary

Adds PERF101 which checks for unnecessary casts to `list` in for loops. 

NOTE: Is not fully equal to its upstream implementation as this
implementation does not flag based on type annotations
(i.e.):
```python
def foo(x: List[str]):
    for y in list(x):
        ...
```

With the current set-up it's quite hard to get the annotation from a
function arg from its binding. Problem is best considered broader than
this implementation.

## Test Plan

Added fixture. 

## Issue links

Refers: #4789

---------

Co-authored-by: Charlie Marsh <[email protected]>
  • Loading branch information
qdegraaf and charliermarsh authored Jun 22, 2023
1 parent 50f0edd commit 38e618c
Show file tree
Hide file tree
Showing 8 changed files with 373 additions and 1 deletion.
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

0 comments on commit 38e618c

Please sign in to comment.