From deb99c33ff339f22186a661d6c24015194bbf15b Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 15 Jun 2023 15:08:17 +0200 Subject: [PATCH 1/6] Add PERF101, add first two cases --- .../test/fixtures/perflint/PERF101.py | 41 ++++++ .../test/fixtures/perflint/PERFTEST.py | 12 ++ crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/perflint/mod.rs | 1 + crates/ruff/src/rules/perflint/rules/mod.rs | 2 + .../perflint/rules/unnecessary_list_cast.rs | 100 +++++++++++++++ ...rflint__tests__PERF101_PERF101.py.snap.new | 121 ++++++++++++++++++ ruff.schema.json | 1 + 9 files changed, 282 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/perflint/PERF101.py create mode 100644 crates/ruff/resources/test/fixtures/perflint/PERFTEST.py create mode 100644 crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs create mode 100644 crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF101.py b/crates/ruff/resources/test/fixtures/perflint/PERF101.py new file mode 100644 index 0000000000000..2dba2ba1908a1 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/perflint/PERF101.py @@ -0,0 +1,41 @@ +from typing import List, Dict, Any + +foo_tuple = (1, 2, 3) +foo_list = [1, 2, 3] +foo_set = {1, 2, 3} +foo_dict = {1: 2, 3: 4} + + +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 + + +def foo(items: List[int]): + for item in list(items): # PERF101 + pass + + +def bar(items: Dict[str, Any]): + for item in list(items): # Ok + pass + + +for i in list(foo_dict): # Ok + pass + diff --git a/crates/ruff/resources/test/fixtures/perflint/PERFTEST.py b/crates/ruff/resources/test/fixtures/perflint/PERFTEST.py new file mode 100644 index 0000000000000..d9ae09f710722 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/perflint/PERFTEST.py @@ -0,0 +1,12 @@ +def foo(items: List[int]): + for item in list(items): # PERF101 + pass + + +def bar(items: Dict[str, Any]): + for item in list(items): # Ok + pass + + +a = [1,2,3] +foo(a) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 33e59c3841b11..8e938ffe0462b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -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, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 68409be315be7..3984ca9fd7f15 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -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 diff --git a/crates/ruff/src/rules/perflint/mod.rs b/crates/ruff/src/rules/perflint/mod.rs index c0b0dd894dd09..b39aa84fc9fc4 100644 --- a/crates/ruff/src/rules/perflint/mod.rs +++ b/crates/ruff/src/rules/perflint/mod.rs @@ -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()); diff --git a/crates/ruff/src/rules/perflint/rules/mod.rs b/crates/ruff/src/rules/perflint/rules/mod.rs index a092bb73f812e..81be906bd4bf5 100644 --- a/crates/ruff/src/rules/perflint/rules/mod.rs +++ b/crates/ruff/src/rules/perflint/rules/mod.rs @@ -1,3 +1,5 @@ pub(crate) use incorrect_dict_iterator::{incorrect_dict_iterator, IncorrectDictIterator}; +pub(crate) use unnecessary_list_cast::{unnecessary_list_cast, UnnecessaryListCast}; mod incorrect_dict_iterator; +mod unnecessary_list_cast; diff --git a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs new file mode 100644 index 0000000000000..2f10721522e9b --- /dev/null +++ b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs @@ -0,0 +1,100 @@ +use ruff_text_size::{TextRange, TextSize}; +use rustpython_parser::ast; +use rustpython_parser::ast::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 usages of `list()` on an iterable to iterate over them in a for-loop +/// +/// ## Why is this bad? +/// Using a `list()` call to eagerly iterate over an already iterable type is inefficient as a +/// second list iterator is created, after first iterating the value: +/// +/// ## 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, ..}) = iter else { + return; + }; + + if args.is_empty() { + return; + } + + if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() { + if id != "list" || !checker.semantic().is_builtin("list") { + return; + } + }; + + match &args[0] { + Expr::Tuple(_) | Expr::List(_) | Expr::Set(_) => fix_incorrect_list_cast(checker, *range), + Expr::Name(ast::ExprName { id, .. }) => { + let scope = checker.semantic().scope(); + if let Some(binding_id) = scope.get(id) { + let binding = &checker.semantic().bindings[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]; + match parent { + Stmt::Assign(ast::StmtAssign { value, .. }) + | Stmt::AnnAssign(ast::StmtAnnAssign { + value: Some(value), .. + }) => match value.as_ref() { + Expr::Tuple(_) | Expr::List(_) | Expr::Set(_) => { + fix_incorrect_list_cast(checker, *range); + } + _ => {} + }, + _ => (), + } + } + } + } + } + _ => {} + } +} + +fn fix_incorrect_list_cast(checker: &mut Checker, range: TextRange) { + let mut diagnostic = Diagnostic::new(UnnecessaryListCast, range); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::automatic_edits( + Edit::deletion(range.start(), range.start() + TextSize::from(5)), + [Edit::deletion(range.end() - TextSize::from(1), range.end())], + )); + } + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new new file mode 100644 index 0000000000000..5e4c6c784ee64 --- /dev/null +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new @@ -0,0 +1,121 @@ +--- +source: crates/ruff/src/rules/perflint/mod.rs +assertion_line: 24 +--- +PERF101.py:9:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | + 9 | for i in list(foo_tuple): # PERF101 + | ^^^^^^^^^^^^^^^ PERF101 +10 | pass + | + = help: Remove `list()` cast + +ℹ Fix +6 6 | foo_dict = {1: 2, 3: 4} +7 7 | +8 8 | +9 |-for i in list(foo_tuple): # PERF101 + 9 |+for i in foo_tuple: # PERF101 +10 10 | pass +11 11 | +12 12 | for i in list(foo_list): # PERF101 + +PERF101.py:12:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +10 | pass +11 | +12 | for i in list(foo_list): # PERF101 + | ^^^^^^^^^^^^^^ PERF101 +13 | pass + | + = help: Remove `list()` cast + +ℹ Fix +9 9 | for i in list(foo_tuple): # PERF101 +10 10 | pass +11 11 | +12 |-for i in list(foo_list): # PERF101 + 12 |+for i in foo_list: # PERF101 +13 13 | pass +14 14 | +15 15 | for i in list(foo_set): # PERF101 + +PERF101.py:15:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +13 | pass +14 | +15 | for i in list(foo_set): # PERF101 + | ^^^^^^^^^^^^^ PERF101 +16 | pass + | + = help: Remove `list()` cast + +ℹ Fix +12 12 | for i in list(foo_list): # PERF101 +13 13 | pass +14 14 | +15 |-for i in list(foo_set): # PERF101 + 15 |+for i in foo_set: # PERF101 +16 16 | pass +17 17 | +18 18 | + +PERF101.py:19:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +19 | for i in list((1, 2, 3)): # PERF101 + | ^^^^^^^^^^^^^^^ PERF101 +20 | pass + | + = help: Remove `list()` cast + +ℹ Fix +16 16 | pass +17 17 | +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({1, 2, 3}): # PERF101 + +PERF101.py:25:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +23 | pass +24 | +25 | for i in list({1, 2, 3}): # PERF101 + | ^^^^^^^^^^^^^^^ PERF101 +26 | 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({1, 2, 3}): # PERF101 + 25 |+for i in {1, 2, 3}: # PERF101 +26 26 | pass +27 27 | +28 28 | + + diff --git a/ruff.schema.json b/ruff.schema.json index 77cd975fac15f..964727f148f73 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2062,6 +2062,7 @@ "PERF", "PERF1", "PERF10", + "PERF101", "PERF102", "PGH", "PGH0", From ea3e13630da2ee2bc6d4abb83bedcd7431f21cca Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 15 Jun 2023 15:13:24 +0200 Subject: [PATCH 2/6] Remove testfile --- .../resources/test/fixtures/perflint/PERFTEST.py | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 crates/ruff/resources/test/fixtures/perflint/PERFTEST.py diff --git a/crates/ruff/resources/test/fixtures/perflint/PERFTEST.py b/crates/ruff/resources/test/fixtures/perflint/PERFTEST.py deleted file mode 100644 index d9ae09f710722..0000000000000 --- a/crates/ruff/resources/test/fixtures/perflint/PERFTEST.py +++ /dev/null @@ -1,12 +0,0 @@ -def foo(items: List[int]): - for item in list(items): # PERF101 - pass - - -def bar(items: Dict[str, Any]): - for item in list(items): # Ok - pass - - -a = [1,2,3] -foo(a) From 5ace48a680bb74c4914b55d0400d5a79a7034053 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 15 Jun 2023 16:14:27 +0200 Subject: [PATCH 3/6] Fix docs formatting --- crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs index 2f10721522e9b..20870a73044a6 100644 --- a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs +++ b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs @@ -20,14 +20,14 @@ use crate::registry::AsRule; /// ```python /// items = (1, 2, 3) /// for i in list(items): -/// print(i) +/// print(i) /// ``` /// /// Use instead: /// ```python /// items = (1, 2, 3) /// for i in items: -/// print(i) +/// print(i) /// ``` #[violation] pub struct UnnecessaryListCast; From a2c5b06a719dd275aeba08d61222e5455ab7e9d8 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Wed, 21 Jun 2023 17:18:13 +0200 Subject: [PATCH 4/6] Add few more testcases review snapshot --- .../test/fixtures/perflint/PERF101.py | 18 +-- ...__perflint__tests__PERF101_PERF101.py.snap | 122 ++++++++++++++++++ ...rflint__tests__PERF101_PERF101.py.snap.new | 121 ----------------- 3 files changed, 128 insertions(+), 133 deletions(-) create mode 100644 crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap delete mode 100644 crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF101.py b/crates/ruff/resources/test/fixtures/perflint/PERF101.py index 2dba2ba1908a1..e579723648a16 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF101.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF101.py @@ -1,10 +1,8 @@ -from typing import List, Dict, Any - 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 @@ -26,16 +24,12 @@ pass -def foo(items: List[int]): - for item in list(items): # PERF101 - pass - - -def bar(items: Dict[str, Any]): - for item in list(items): # Ok - pass +for i in list(foo_dict): # Ok + pass -for i in list(foo_dict): # Ok +for i in list(1): # Ok pass +for i in list(foo_int): # Ok + pass diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap new file mode 100644 index 0000000000000..491ac00c06d66 --- /dev/null +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap @@ -0,0 +1,122 @@ +--- +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 | + +PERF101.py:17:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +17 | for i in list((1, 2, 3)): # PERF101 + | ^^^^^^^^^^^^^^^ PERF101 +18 | pass + | + = help: Remove `list()` cast + +ℹ Fix +14 14 | pass +15 15 | +16 16 | +17 |-for i in list((1, 2, 3)): # PERF101 + 17 |+for i in (1, 2, 3): # PERF101 +18 18 | pass +19 19 | +20 20 | for i in list([1, 2, 3]): # PERF101 + +PERF101.py:20:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +18 | pass +19 | +20 | for i in list([1, 2, 3]): # PERF101 + | ^^^^^^^^^^^^^^^ PERF101 +21 | pass + | + = help: Remove `list()` cast + +ℹ Fix +17 17 | for i in list((1, 2, 3)): # PERF101 +18 18 | pass +19 19 | +20 |-for i in list([1, 2, 3]): # PERF101 + 20 |+for i in [1, 2, 3]: # PERF101 +21 21 | pass +22 22 | +23 23 | for i in list({1, 2, 3}): # PERF101 + +PERF101.py:23:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +21 | pass +22 | +23 | for i in list({1, 2, 3}): # PERF101 + | ^^^^^^^^^^^^^^^ PERF101 +24 | pass + | + = help: Remove `list()` cast + +ℹ Fix +20 20 | for i in list([1, 2, 3]): # PERF101 +21 21 | pass +22 22 | +23 |-for i in list({1, 2, 3}): # PERF101 + 23 |+for i in {1, 2, 3}: # PERF101 +24 24 | pass +25 25 | +26 26 | + + diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new deleted file mode 100644 index 5e4c6c784ee64..0000000000000 --- a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap.new +++ /dev/null @@ -1,121 +0,0 @@ ---- -source: crates/ruff/src/rules/perflint/mod.rs -assertion_line: 24 ---- -PERF101.py:9:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it - | - 9 | for i in list(foo_tuple): # PERF101 - | ^^^^^^^^^^^^^^^ PERF101 -10 | pass - | - = help: Remove `list()` cast - -ℹ Fix -6 6 | foo_dict = {1: 2, 3: 4} -7 7 | -8 8 | -9 |-for i in list(foo_tuple): # PERF101 - 9 |+for i in foo_tuple: # PERF101 -10 10 | pass -11 11 | -12 12 | for i in list(foo_list): # PERF101 - -PERF101.py:12:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it - | -10 | pass -11 | -12 | for i in list(foo_list): # PERF101 - | ^^^^^^^^^^^^^^ PERF101 -13 | pass - | - = help: Remove `list()` cast - -ℹ Fix -9 9 | for i in list(foo_tuple): # PERF101 -10 10 | pass -11 11 | -12 |-for i in list(foo_list): # PERF101 - 12 |+for i in foo_list: # PERF101 -13 13 | pass -14 14 | -15 15 | for i in list(foo_set): # PERF101 - -PERF101.py:15:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it - | -13 | pass -14 | -15 | for i in list(foo_set): # PERF101 - | ^^^^^^^^^^^^^ PERF101 -16 | pass - | - = help: Remove `list()` cast - -ℹ Fix -12 12 | for i in list(foo_list): # PERF101 -13 13 | pass -14 14 | -15 |-for i in list(foo_set): # PERF101 - 15 |+for i in foo_set: # PERF101 -16 16 | pass -17 17 | -18 18 | - -PERF101.py:19:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it - | -19 | for i in list((1, 2, 3)): # PERF101 - | ^^^^^^^^^^^^^^^ PERF101 -20 | pass - | - = help: Remove `list()` cast - -ℹ Fix -16 16 | pass -17 17 | -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({1, 2, 3}): # PERF101 - -PERF101.py:25:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it - | -23 | pass -24 | -25 | for i in list({1, 2, 3}): # PERF101 - | ^^^^^^^^^^^^^^^ PERF101 -26 | 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({1, 2, 3}): # PERF101 - 25 |+for i in {1, 2, 3}: # PERF101 -26 26 | pass -27 27 | -28 28 | - - From 4f3900fe875da6698a0e63443f56f7054345efa9 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 22 Jun 2023 00:24:28 +0200 Subject: [PATCH 5/6] Update autofix to use range of iterable --- .../test/fixtures/perflint/PERF101.py | 9 +++++ .../perflint/rules/unnecessary_list_cast.rs | 34 +++++++++++++----- ...__perflint__tests__PERF101_PERF101.py.snap | 36 ++++++++++++++++++- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF101.py b/crates/ruff/resources/test/fixtures/perflint/PERF101.py index e579723648a16..8cc608baeeeb2 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF101.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF101.py @@ -23,6 +23,15 @@ for i in list({1, 2, 3}): # PERF101 pass +for i in list( + { + 1, + 2, + 3, + } +): + pass + for i in list(foo_dict): # Ok pass diff --git a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs index 20870a73044a6..3c17b7639838c 100644 --- a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs +++ b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs @@ -1,4 +1,4 @@ -use ruff_text_size::{TextRange, TextSize}; +use ruff_text_size::TextRange; use rustpython_parser::ast; use rustpython_parser::ast::Expr; @@ -45,7 +45,7 @@ impl AlwaysAutofixableViolation for UnnecessaryListCast { /// PERF101 pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { - let Expr::Call(ast::ExprCall{ func, args, range, ..}) = iter else { + let Expr::Call(ast::ExprCall{ func, args, range: list_range, ..}) = iter else { return; }; @@ -60,8 +60,20 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { }; match &args[0] { - Expr::Tuple(_) | Expr::List(_) | Expr::Set(_) => fix_incorrect_list_cast(checker, *range), - Expr::Name(ast::ExprName { id, .. }) => { + Expr::Tuple(ast::ExprTuple { + range: iter_range, .. + }) + | Expr::List(ast::ExprList { + range: iter_range, .. + }) + | Expr::Set(ast::ExprSet { + range: iter_range, .. + }) => fix_incorrect_list_cast(checker, *list_range, *iter_range), + 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().bindings[binding_id]; @@ -74,7 +86,7 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { value: Some(value), .. }) => match value.as_ref() { Expr::Tuple(_) | Expr::List(_) | Expr::Set(_) => { - fix_incorrect_list_cast(checker, *range); + fix_incorrect_list_cast(checker, *list_range, *iterable_range); } _ => {} }, @@ -88,12 +100,16 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { } } -fn fix_incorrect_list_cast(checker: &mut Checker, range: TextRange) { - let mut diagnostic = Diagnostic::new(UnnecessaryListCast, range); +fn fix_incorrect_list_cast( + checker: &mut Checker, + list_range: TextRange, + iterable_range: TextRange, +) { + let mut diagnostic = Diagnostic::new(UnnecessaryListCast, list_range); if checker.patch(diagnostic.kind.rule()) { diagnostic.set_fix(Fix::automatic_edits( - Edit::deletion(range.start(), range.start() + TextSize::from(5)), - [Edit::deletion(range.end() - TextSize::from(1), range.end())], + Edit::deletion(list_range.start(), iterable_range.start()), + [Edit::deletion(iterable_range.end(), list_range.end())], )); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap index 491ac00c06d66..5fb91b54eca21 100644 --- a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap @@ -117,6 +117,40 @@ PERF101.py:23:10: PERF101 [*] Do not cast an iterable to `list` before iterating 23 |+for i in {1, 2, 3}: # PERF101 24 24 | pass 25 25 | -26 26 | +26 26 | for i in list( + +PERF101.py:26:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it + | +24 | pass +25 | +26 | for i in list( + | __________^ +27 | | { +28 | | 1, +29 | | 2, +30 | | 3, +31 | | } +32 | | ): + | |_^ PERF101 +33 | pass + | + = help: Remove `list()` cast + +ℹ Fix +23 23 | for i in list({1, 2, 3}): # PERF101 +24 24 | pass +25 25 | +26 |-for i in list( +27 |- { + 26 |+for i in { +28 27 | 1, +29 28 | 2, +30 29 | 3, +31 |- } +32 |-): + 30 |+ }: +33 31 | pass +34 32 | +35 33 | From fed86de687a9c193c2c4944cea39a1463ae48789 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 22 Jun 2023 16:31:53 -0400 Subject: [PATCH 6/6] Tweaks --- .../test/fixtures/perflint/PERF101.py | 12 +- crates/ruff/src/rules/perflint/rules/mod.rs | 4 +- .../perflint/rules/unnecessary_list_cast.rs | 89 +++++----- ...__perflint__tests__PERF101_PERF101.py.snap | 157 ++++++++++-------- 4 files changed, 155 insertions(+), 107 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF101.py b/crates/ruff/resources/test/fixtures/perflint/PERF101.py index 8cc608baeeeb2..6123e6d6526bf 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF101.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF101.py @@ -13,7 +13,6 @@ for i in list(foo_set): # PERF101 pass - for i in list((1, 2, 3)): # PERF101 pass @@ -32,13 +31,22 @@ ): 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 diff --git a/crates/ruff/src/rules/perflint/rules/mod.rs b/crates/ruff/src/rules/perflint/rules/mod.rs index 81be906bd4bf5..cc35c428b08fa 100644 --- a/crates/ruff/src/rules/perflint/rules/mod.rs +++ b/crates/ruff/src/rules/perflint/rules/mod.rs @@ -1,5 +1,5 @@ -pub(crate) use incorrect_dict_iterator::{incorrect_dict_iterator, IncorrectDictIterator}; -pub(crate) use unnecessary_list_cast::{unnecessary_list_cast, UnnecessaryListCast}; +pub(crate) use incorrect_dict_iterator::*; +pub(crate) use unnecessary_list_cast::*; mod incorrect_dict_iterator; mod unnecessary_list_cast; diff --git a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs index 3c17b7639838c..57f2f49caa28e 100644 --- a/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs +++ b/crates/ruff/src/rules/perflint/rules/unnecessary_list_cast.rs @@ -1,6 +1,5 @@ use ruff_text_size::TextRange; -use rustpython_parser::ast; -use rustpython_parser::ast::Expr; +use rustpython_parser::ast::{self, Expr}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; @@ -10,11 +9,15 @@ use crate::checkers::ast::Checker; use crate::registry::AsRule; /// ## What it does -/// Checks for explicit usages of `list()` on an iterable to iterate over them in a for-loop +/// 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 is inefficient as a -/// second list iterator is created, after first iterating the value: +/// 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 @@ -49,26 +52,37 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { return; }; - if args.is_empty() { + if args.len() != 1 { return; } - if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() { - if id != "list" || !checker.semantic().is_builtin("list") { - 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: iter_range, .. + range: iterable_range, + .. }) | Expr::List(ast::ExprList { - range: iter_range, .. + range: iterable_range, + .. }) | Expr::Set(ast::ExprSet { - range: iter_range, .. - }) => fix_incorrect_list_cast(checker, *list_range, *iter_range), + 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, @@ -76,21 +90,27 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { }) => { let scope = checker.semantic().scope(); if let Some(binding_id) = scope.get(id) { - let binding = &checker.semantic().bindings[binding_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]; - match parent { - Stmt::Assign(ast::StmtAssign { value, .. }) - | Stmt::AnnAssign(ast::StmtAnnAssign { - value: Some(value), .. - }) => match value.as_ref() { - Expr::Tuple(_) | Expr::List(_) | Expr::Set(_) => { - fix_incorrect_list_cast(checker, *list_range, *iterable_range); + 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); + } } } } @@ -100,17 +120,10 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) { } } -fn fix_incorrect_list_cast( - checker: &mut Checker, - list_range: TextRange, - iterable_range: TextRange, -) { - let mut diagnostic = Diagnostic::new(UnnecessaryListCast, list_range); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(Fix::automatic_edits( - Edit::deletion(list_range.start(), iterable_range.start()), - [Edit::deletion(iterable_range.end(), list_range.end())], - )); - } - 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())], + ) } diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap index 5fb91b54eca21..54a17b4c4fa5f 100644 --- a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF101_PERF101.py.snap @@ -59,98 +59,125 @@ PERF101.py:13:10: PERF101 [*] Do not cast an iterable to `list` before iterating 13 |+for i in foo_set: # PERF101 14 14 | pass 15 15 | -16 16 | +16 16 | for i in list((1, 2, 3)): # PERF101 -PERF101.py:17:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it +PERF101.py:16:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it | -17 | for i in list((1, 2, 3)): # PERF101 +14 | pass +15 | +16 | for i in list((1, 2, 3)): # PERF101 | ^^^^^^^^^^^^^^^ PERF101 -18 | pass +17 | pass | = help: Remove `list()` cast ℹ Fix +13 13 | for i in list(foo_set): # PERF101 14 14 | pass 15 15 | -16 16 | -17 |-for i in list((1, 2, 3)): # PERF101 - 17 |+for i in (1, 2, 3): # PERF101 -18 18 | pass -19 19 | -20 20 | for i in list([1, 2, 3]): # PERF101 - -PERF101.py:20:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it +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 | -18 | pass -19 | -20 | for i in list([1, 2, 3]): # PERF101 +17 | pass +18 | +19 | for i in list([1, 2, 3]): # PERF101 | ^^^^^^^^^^^^^^^ PERF101 -21 | pass +20 | pass | = help: Remove `list()` cast ℹ Fix -17 17 | for i in list((1, 2, 3)): # PERF101 -18 18 | pass -19 19 | -20 |-for i in list([1, 2, 3]): # PERF101 - 20 |+for i in [1, 2, 3]: # PERF101 -21 21 | pass -22 22 | -23 23 | for i in list({1, 2, 3}): # PERF101 - -PERF101.py:23:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it +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 | -21 | pass -22 | -23 | for i in list({1, 2, 3}): # PERF101 +20 | pass +21 | +22 | for i in list({1, 2, 3}): # PERF101 | ^^^^^^^^^^^^^^^ PERF101 -24 | pass +23 | pass | = help: Remove `list()` cast ℹ Fix -20 20 | for i in list([1, 2, 3]): # PERF101 -21 21 | pass -22 22 | -23 |-for i in list({1, 2, 3}): # PERF101 - 23 |+for i in {1, 2, 3}: # PERF101 -24 24 | pass -25 25 | -26 26 | for i in list( - -PERF101.py:26:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it +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 | -24 | pass -25 | -26 | for i in list( +32 | pass +33 | +34 | for i in list( # Comment | __________^ -27 | | { -28 | | 1, -29 | | 2, -30 | | 3, -31 | | } -32 | | ): +35 | | {1, 2, 3} +36 | | ): # PERF101 | |_^ PERF101 -33 | pass +37 | pass | = help: Remove `list()` cast ℹ Fix -23 23 | for i in list({1, 2, 3}): # PERF101 -24 24 | pass -25 25 | -26 |-for i in list( -27 |- { - 26 |+for i in { -28 27 | 1, -29 28 | 2, -30 29 | 3, -31 |- } -32 |-): - 30 |+ }: -33 31 | pass -34 32 | -35 33 | +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