From aebf8a9c7e9b60cc789346b36c2d41a910050e5d Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 5 Jul 2023 16:55:12 -0500 Subject: [PATCH 01/18] Create GetFirstElementOfList rule --- .../resources/test/fixtures/ruff/RUF015.py | 0 crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/codes.rs | 1 + .../ruff/rules/get_first_element_of_list.rs | 156 ++++++++++++++++++ crates/ruff/src/rules/ruff/rules/mod.rs | 2 + 5 files changed, 162 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/ruff/RUF015.py create mode 100644 crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index be8aa68af6aff..d00748deb92d4 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2219,6 +2219,9 @@ where if self.enabled(Rule::UncapitalizedEnvironmentVariables) { flake8_simplify::rules::use_capital_environment_variables(self, expr); } + if self.enabled(Rule::GetFirstElementOfList) { + ruff::rules::get_first_element_of_list(self, value, slice); + } pandas_vet::rules::subscript(self, value, expr); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index b73d2caa44619..674a798cb6c23 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -778,6 +778,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "013") => (RuleGroup::Unspecified, rules::ruff::rules::ImplicitOptional), #[cfg(feature = "unreachable-code")] (Ruff, "014") => (RuleGroup::Nursery, rules::ruff::rules::UnreachableCode), + (Ruff, "015") => (RuleGroup::Unspecified, rules::ruff::rules::GetFirstElementOfList), (Ruff, "100") => (RuleGroup::Unspecified, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Unspecified, rules::ruff::rules::InvalidPyprojectToml), diff --git a/crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs b/crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs new file mode 100644 index 0000000000000..5399dbe119a09 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs @@ -0,0 +1,156 @@ +use num_traits::ToPrimitive; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_text_size::TextRange; +use rustpython_parser::ast::{self, Constant, Expr}; + +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Ensures that instead of creating a new list and indexing into it to find the first element of a +/// collection (e.g., `list(...)[0]`), Python iterators are used. +/// +/// Why is this bad? +/// Creating a new list of great size can involve significant memory/speed concerns. Python's `next(iter(...))` +/// pattern can be used in lieu of creating a new list. This pattern will lazily fetch the first +/// element of the collection, avoiding the memory overhead involved with new list allocation. +/// `next(iter(...))` also is much faster since the list itself doesn't get initialized at once. +/// +/// ## Example +/// ```python +/// x = range(1000000000000) +/// return list(x)[0] +/// ``` +/// +/// Use instead: +/// ```python +/// x = range(1000000000000) +/// return next(iter(x)) +/// ``` +/// +/// ## References +/// - [Iterators and Iterables in Python: Run Efficient +/// Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) +#[violation] +pub struct GetFirstElementOfList { + arg: String, +} + +impl GetFirstElementOfList { + pub(crate) fn new(arg: String) -> Self { + Self { arg } + } +} + +impl AlwaysAutofixableViolation for GetFirstElementOfList { + #[derive_message_formats] + fn message(&self) -> String { + format!( + "Prefer `next(iter({}))` over `list({})[0]` or equivalent list comprehension", + self.arg, self.arg + ) + } + + fn autofix_title(&self) -> String { + format!( + "Convert `list({})[0]` or equivalent list comprehension call to `next(iter({}))`", + self.arg, self.arg + ) + } +} + +/// RUF015 +pub(crate) fn get_first_element_of_list(checker: &mut Checker, call: &Expr, slice: &Expr) { + let Some(ListComponents { func_range, subscript_range, iter_name }) = decompose_list_expr(checker, call, slice) else { + return; + }; + + let subscript_range = + TextRange::at(func_range.start(), func_range.len() + subscript_range.len()); + let mut diagnostic = Diagnostic::new( + GetFirstElementOfList::new(iter_name.to_string()), + subscript_range, + ); + + if checker.patch(diagnostic.kind.rule()) { + // diagnostic.set_fix(Fix::automatic(Edit::range_replacement(content, range))); + } + + checker.diagnostics.push(diagnostic); +} + +/// Simple record struct that represents the components required for a list creation. +struct ListComponents<'a> { + /// The [`TextRange`] for the actual list creation - either `list(x)` or `[i for i in x]` + func_range: &'a TextRange, + /// The subscript's (e.g., `[0]`) [`TextRange`] + subscript_range: &'a TextRange, + /// The name of the iterable - the "x" in `list(x)` and `[i for i in x]` + iter_name: &'a str, +} + +impl<'a> ListComponents<'a> { + fn new(func_range: &'a TextRange, slice_range: &'a TextRange, arg_name: &'a str) -> Self { + Self { + func_range, + subscript_range: slice_range, + iter_name: arg_name, + } + } +} + +// Decompose an [`Expr`] into the parts relevant for the diagnostic. If the [`Expr`] in question +// isn't a list, return None +fn decompose_list_expr<'a>( + checker: &mut Checker, + expr: &'a Expr, + slice: &'a Expr, +) -> Option> { + // Ensure slice is at 0 + let Expr::Constant(ast::ExprConstant{ value: Constant::Int(slice_index), range: slice_range, .. }) = slice else { + return None; + }; + if slice_index.to_i64() != Some(0i64) { + return None; + } + + // Decompose. + let list_components = match expr { + Expr::Call(ast::ExprCall { + func, range, args, .. + }) => { + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return None; + }; + if !(id == "list" && checker.semantic().is_builtin("list")) { + return None; + } + + let Some(Expr::Name(ast::ExprName { id: arg_name, .. })) = args.first() else { + return None; + }; + + Some(ListComponents::new(range, slice_range, arg_name)) + } + Expr::ListComp(ast::ExprListComp { + range, generators, .. + }) => { + // If there's more than 1 generator, we can't safely say that it's invalid. For + // example, `[(i, j) for i in x for j in y][0]` + if generators.len() != 1 { + return None; + } + + let generator = &generators[0]; + let Expr::Name(ast::ExprName { id: arg_name, .. }) = &generator.iter else { + return None; + }; + + Some(ListComponents::new(range, slice_range, arg_name.as_str())) + } + _ => None, + }; + + list_components +} diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index f79f5fafd7652..3ca5b2cf34f8b 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -3,6 +3,7 @@ pub(crate) use asyncio_dangling_task::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use function_call_in_dataclass_default::*; +pub(crate) use get_first_element_of_list::*; pub(crate) use implicit_optional::*; pub(crate) use invalid_pyproject_toml::*; pub(crate) use mutable_class_default::*; @@ -19,6 +20,7 @@ mod collection_literal_concatenation; mod confusables; mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; +mod get_first_element_of_list; mod helpers; mod implicit_optional; mod invalid_pyproject_toml; From 236a23f65219dcf3cf1d9347facd8e8d1469b5fe Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 5 Jul 2023 17:00:35 -0500 Subject: [PATCH 02/18] Rename rule to UnnecessaryListAllocationForFirstElement --- crates/ruff/src/checkers/ast/mod.rs | 4 ++-- crates/ruff/src/codes.rs | 2 +- crates/ruff/src/rules/ruff/rules/mod.rs | 4 ++-- ...sary_list_allocation_for_first_element.rs} | 24 +++++++++++-------- 4 files changed, 19 insertions(+), 15 deletions(-) rename crates/ruff/src/rules/ruff/rules/{get_first_element_of_list.rs => unnecessary_list_allocation_for_first_element.rs} (85%) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index d00748deb92d4..de932d981860f 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2219,8 +2219,8 @@ where if self.enabled(Rule::UncapitalizedEnvironmentVariables) { flake8_simplify::rules::use_capital_environment_variables(self, expr); } - if self.enabled(Rule::GetFirstElementOfList) { - ruff::rules::get_first_element_of_list(self, value, slice); + if self.enabled(Rule::UnnecessaryListAllocationForFirstElement) { + ruff::rules::unnecessary_list_allocation_for_first_element(self, value, slice); } pandas_vet::rules::subscript(self, value, expr); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 674a798cb6c23..8de023011b0cd 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -778,7 +778,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "013") => (RuleGroup::Unspecified, rules::ruff::rules::ImplicitOptional), #[cfg(feature = "unreachable-code")] (Ruff, "014") => (RuleGroup::Nursery, rules::ruff::rules::UnreachableCode), - (Ruff, "015") => (RuleGroup::Unspecified, rules::ruff::rules::GetFirstElementOfList), + (Ruff, "015") => (RuleGroup::Unspecified, rules::ruff::rules::UnnecessaryListAllocationForFirstElement), (Ruff, "100") => (RuleGroup::Unspecified, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Unspecified, rules::ruff::rules::InvalidPyprojectToml), diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index 3ca5b2cf34f8b..b290a05ae56a3 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -3,13 +3,13 @@ pub(crate) use asyncio_dangling_task::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use function_call_in_dataclass_default::*; -pub(crate) use get_first_element_of_list::*; pub(crate) use implicit_optional::*; pub(crate) use invalid_pyproject_toml::*; pub(crate) use mutable_class_default::*; pub(crate) use mutable_dataclass_default::*; pub(crate) use pairwise_over_zipped::*; pub(crate) use static_key_dict_comprehension::*; +pub(crate) use unnecessary_list_allocation_for_first_element::*; #[cfg(feature = "unreachable-code")] pub(crate) use unreachable::*; pub(crate) use unused_noqa::*; @@ -20,7 +20,6 @@ mod collection_literal_concatenation; mod confusables; mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; -mod get_first_element_of_list; mod helpers; mod implicit_optional; mod invalid_pyproject_toml; @@ -28,6 +27,7 @@ mod mutable_class_default; mod mutable_dataclass_default; mod pairwise_over_zipped; mod static_key_dict_comprehension; +mod unnecessary_list_allocation_for_first_element; #[cfg(feature = "unreachable-code")] pub(crate) mod unreachable; mod unused_noqa; diff --git a/crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs similarity index 85% rename from crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs rename to crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 5399dbe119a09..408845e0e0b05 100644 --- a/crates/ruff/src/rules/ruff/rules/get_first_element_of_list.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -33,17 +33,17 @@ use crate::registry::AsRule; /// - [Iterators and Iterables in Python: Run Efficient /// Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) #[violation] -pub struct GetFirstElementOfList { +pub struct UnnecessaryListAllocationForFirstElement { arg: String, } -impl GetFirstElementOfList { +impl UnnecessaryListAllocationForFirstElement { pub(crate) fn new(arg: String) -> Self { Self { arg } } } -impl AlwaysAutofixableViolation for GetFirstElementOfList { +impl AlwaysAutofixableViolation for UnnecessaryListAllocationForFirstElement { #[derive_message_formats] fn message(&self) -> String { format!( @@ -61,26 +61,30 @@ impl AlwaysAutofixableViolation for GetFirstElementOfList { } /// RUF015 -pub(crate) fn get_first_element_of_list(checker: &mut Checker, call: &Expr, slice: &Expr) { +pub(crate) fn unnecessary_list_allocation_for_first_element( + checker: &mut Checker, + call: &Expr, + slice: &Expr, +) { let Some(ListComponents { func_range, subscript_range, iter_name }) = decompose_list_expr(checker, call, slice) else { return; }; - let subscript_range = - TextRange::at(func_range.start(), func_range.len() + subscript_range.len()); + let range = TextRange::at(func_range.start(), func_range.len() + subscript_range.len()); let mut diagnostic = Diagnostic::new( - GetFirstElementOfList::new(iter_name.to_string()), - subscript_range, + UnnecessaryListAllocationForFirstElement::new(iter_name.to_string()), + range, ); if checker.patch(diagnostic.kind.rule()) { - // diagnostic.set_fix(Fix::automatic(Edit::range_replacement(content, range))); + let replacement = format!("next(iter({}))", iter_name); + diagnostic.set_fix(Fix::suggested(Edit::range_replacement(replacement, range))); } checker.diagnostics.push(diagnostic); } -/// Simple record struct that represents the components required for a list creation. +/// Lighweight record struct that represents the components required for a list creation. struct ListComponents<'a> { /// The [`TextRange`] for the actual list creation - either `list(x)` or `[i for i in x]` func_range: &'a TextRange, From ab41a985a5c12dcec19fe35cc7c5429a314bef9d Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 5 Jul 2023 18:24:04 -0500 Subject: [PATCH 03/18] Handle slices --- .../resources/test/fixtures/ruff/RUF015.py | 19 ++++ crates/ruff/src/checkers/ast/mod.rs | 11 +- ...ssary_list_allocation_for_first_element.rs | 100 +++++++++--------- 3 files changed, 76 insertions(+), 54 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py index e69de29bb2d1d..f87cf43898cc0 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF015.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF015.py @@ -0,0 +1,19 @@ +x = range(10) + +# RUF015 +list(x)[0] +list(x)[:1] + +# RUF015 +[i for i in x][0] +[i for i in x][:1] + +y = range(10) +# Fine - multiple generators +[i + j for i in x for j in y][0] + +# Fine - not indexing at 0 +list(x)[1] +list(x)[1:] +[i for i in x][1] +[i for i in x][1:] diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index de932d981860f..c9681236a237f 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2139,7 +2139,12 @@ where // Pre-visit. match expr { - Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + Expr::Subscript(ast::ExprSubscript { + value, + slice, + range, + .. + }) => { // Ex) Optional[...], Union[...] if self.any_enabled(&[ Rule::FutureRewritableTypeAnnotation, @@ -2220,7 +2225,9 @@ where flake8_simplify::rules::use_capital_environment_variables(self, expr); } if self.enabled(Rule::UnnecessaryListAllocationForFirstElement) { - ruff::rules::unnecessary_list_allocation_for_first_element(self, value, slice); + ruff::rules::unnecessary_list_allocation_for_first_element( + self, value, slice, range, + ); } pandas_vet::rules::subscript(self, value, expr); diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 408845e0e0b05..86f6569bf40a6 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -1,5 +1,5 @@ use num_traits::ToPrimitive; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_text_size::TextRange; use rustpython_parser::ast::{self, Constant, Expr}; @@ -65,65 +65,36 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( checker: &mut Checker, call: &Expr, slice: &Expr, + subscript_range: &TextRange, ) { - let Some(ListComponents { func_range, subscript_range, iter_name }) = decompose_list_expr(checker, call, slice) else { + if !indexes_first_element(slice) { + return; + } + let Some(iter_name) = get_iterable_name(checker, call) else { return; }; - let range = TextRange::at(func_range.start(), func_range.len() + subscript_range.len()); let mut diagnostic = Diagnostic::new( UnnecessaryListAllocationForFirstElement::new(iter_name.to_string()), - range, + *subscript_range, ); if checker.patch(diagnostic.kind.rule()) { let replacement = format!("next(iter({}))", iter_name); - diagnostic.set_fix(Fix::suggested(Edit::range_replacement(replacement, range))); + diagnostic.set_fix(Fix::suggested(Edit::range_replacement( + replacement, + *subscript_range, + ))); } checker.diagnostics.push(diagnostic); } -/// Lighweight record struct that represents the components required for a list creation. -struct ListComponents<'a> { - /// The [`TextRange`] for the actual list creation - either `list(x)` or `[i for i in x]` - func_range: &'a TextRange, - /// The subscript's (e.g., `[0]`) [`TextRange`] - subscript_range: &'a TextRange, - /// The name of the iterable - the "x" in `list(x)` and `[i for i in x]` - iter_name: &'a str, -} - -impl<'a> ListComponents<'a> { - fn new(func_range: &'a TextRange, slice_range: &'a TextRange, arg_name: &'a str) -> Self { - Self { - func_range, - subscript_range: slice_range, - iter_name: arg_name, - } - } -} - -// Decompose an [`Expr`] into the parts relevant for the diagnostic. If the [`Expr`] in question -// isn't a list, return None -fn decompose_list_expr<'a>( - checker: &mut Checker, - expr: &'a Expr, - slice: &'a Expr, -) -> Option> { - // Ensure slice is at 0 - let Expr::Constant(ast::ExprConstant{ value: Constant::Int(slice_index), range: slice_range, .. }) = slice else { - return None; - }; - if slice_index.to_i64() != Some(0i64) { - return None; - } - +/// Fetch the name of the iterable from a list expression. +fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a String> { // Decompose. - let list_components = match expr { - Expr::Call(ast::ExprCall { - func, range, args, .. - }) => { + let name = match expr { + Expr::Call(ast::ExprCall { func, args, .. }) => { let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { return None; }; @@ -135,13 +106,11 @@ fn decompose_list_expr<'a>( return None; }; - Some(ListComponents::new(range, slice_range, arg_name)) + Some(arg_name) } - Expr::ListComp(ast::ExprListComp { - range, generators, .. - }) => { - // If there's more than 1 generator, we can't safely say that it's invalid. For - // example, `[(i, j) for i in x for j in y][0]` + Expr::ListComp(ast::ExprListComp { generators, .. }) => { + // If there's more than 1 generator, we can't safely say that it fits the diagnostic conditions - + // for example, `[i + j for i in x for j in y][0]` if generators.len() != 1 { return None; } @@ -151,10 +120,37 @@ fn decompose_list_expr<'a>( return None; }; - Some(ListComponents::new(range, slice_range, arg_name.as_str())) + Some(arg_name) } _ => None, }; - list_components + name +} + +fn indexes_first_element(expr: &Expr) -> bool { + match expr { + Expr::Constant(ast::ExprConstant { .. }) => get_index_value(expr) == Some(0i64), + Expr::Slice(ast::ExprSlice { lower, upper, .. }) => { + let lower_index = lower.as_ref().and_then(|l| get_index_value(&l)); + let upper_index = upper.as_ref().and_then(|u| get_index_value(&u)); + + if lower_index.is_none() || lower_index == Some(0i64) { + return upper_index == Some(1i64); + } else { + return false; + } + } + _ => false, + } +} + +fn get_index_value(expr: &Expr) -> Option { + match expr { + Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) => value.to_i64(), + _ => None, + } } From 29d35198bde36c702786d984aab4eb8939186db6 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 5 Jul 2023 20:00:39 -0500 Subject: [PATCH 04/18] Handle non-Name `elts` in Expr::ListComp --- .../resources/test/fixtures/ruff/RUF015.py | 28 ++- crates/ruff/src/rules/ruff/mod.rs | 1 + ...ssary_list_allocation_for_first_element.rs | 76 +++++--- ..._rules__ruff__tests__RUF015_RUF015.py.snap | 170 ++++++++++++++++++ ruff.schema.json | 3 +- test.py | 1 + 6 files changed, 248 insertions(+), 31 deletions(-) create mode 100644 crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap create mode 100644 test.py diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py index f87cf43898cc0..8967253afa33d 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF015.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF015.py @@ -3,17 +3,31 @@ # RUF015 list(x)[0] list(x)[:1] - -# RUF015 +list(x)[:1:1] +list(x)[:1:2] [i for i in x][0] [i for i in x][:1] +[i for i in x][:1:1] +[i for i in x][:1:2] -y = range(10) -# Fine - multiple generators -[i + j for i in x for j in y][0] - -# Fine - not indexing at 0 +# Fine - not indexing (solely) the first element +list(x) list(x)[1] +list(x)[-1] list(x)[1:] +list(x)[:3:2] +list(x)[::2] +[i for i in x] [i for i in x][1] +[i for i in x][-1] [i for i in x][1:] +[i for i in x][:3:2] +[i for i in x][::2] + +# Fine - modifies the first element +[i + 1 for i in x][0] + +y = range(10) +# Fine - multiple generators +[i + j for i in x for j in y][0] + diff --git a/crates/ruff/src/rules/ruff/mod.rs b/crates/ruff/src/rules/ruff/mod.rs index 336a65c7e1957..262dcaea88ed9 100644 --- a/crates/ruff/src/rules/ruff/mod.rs +++ b/crates/ruff/src/rules/ruff/mod.rs @@ -30,6 +30,7 @@ mod tests { #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008.py"))] #[test_case(Rule::PairwiseOverZipped, Path::new("RUF007.py"))] #[test_case(Rule::StaticKeyDictComprehension, Path::new("RUF011.py"))] + #[test_case(Rule::UnnecessaryListAllocationForFirstElement, Path::new("RUF015.py"))] #[cfg_attr( feature = "unreachable-code", test_case(Rule::UnreachableCode, Path::new("RUF014.py")) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 86f6569bf40a6..d4d0c913eb7f4 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -90,12 +90,12 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( checker.diagnostics.push(diagnostic); } -/// Fetch the name of the iterable from a list expression. -fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a String> { - // Decompose. - let name = match expr { +/// Fetch the name of the iterable from a list expression if the expression returns an unmodified list +/// which can be sliced into. +fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a str> { + match expr { Expr::Call(ast::ExprCall { func, args, .. }) => { - let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + let Some(id) = get_name_id(func.as_ref()) else { return None; }; if !(id == "list" && checker.semantic().is_builtin("list")) { @@ -106,46 +106,69 @@ fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a St return None; }; - Some(arg_name) + Some(arg_name.as_str()) } - Expr::ListComp(ast::ExprListComp { generators, .. }) => { + Expr::ListComp(ast::ExprListComp { + elt, generators, .. + }) => { + // If the `elt` field is anything other than a [`Expr::Name`], we can't be sure that it + // doesn't modify the elements of the underlying iterator - for example, `[i + 1 for i in x][0]`. + if !matches!(elt.as_ref(), Expr::Name(ast::ExprName { .. })) { + return None; + } + // If there's more than 1 generator, we can't safely say that it fits the diagnostic conditions - - // for example, `[i + j for i in x for j in y][0]` + // for example, `[(i, j) for i in x for j in y][0]`. if generators.len() != 1 { return None; } let generator = &generators[0]; - let Expr::Name(ast::ExprName { id: arg_name, .. }) = &generator.iter else { + let Some(arg_name) = get_name_id(&generator.iter) else { return None; }; - Some(arg_name) } _ => None, - }; - - name + } } +/// Check that the slice [`Expr`] is functionally equivalent to slicing into the first element. fn indexes_first_element(expr: &Expr) -> bool { match expr { - Expr::Constant(ast::ExprConstant { .. }) => get_index_value(expr) == Some(0i64), - Expr::Slice(ast::ExprSlice { lower, upper, .. }) => { - let lower_index = lower.as_ref().and_then(|l| get_index_value(&l)); - let upper_index = upper.as_ref().and_then(|u| get_index_value(&u)); - - if lower_index.is_none() || lower_index == Some(0i64) { - return upper_index == Some(1i64); - } else { - return false; + Expr::Constant(ast::ExprConstant { .. }) => { + get_effective_index(expr).unwrap_or(0i64) == 0i64 + } + Expr::Slice(ast::ExprSlice { + step: step_value, + lower: lower_index, + upper: upper_index, + .. + }) => { + let lower = lower_index + .as_ref() + .and_then(|l| get_effective_index(l)) + .unwrap_or(0i64); + let upper = upper_index + .as_ref() + .and_then(|u| get_effective_index(u)) + .unwrap_or(i64::MAX); + let step = step_value + .as_ref() + .and_then(|s| get_effective_index(s)) + .unwrap_or(1i64); + + if lower == 0i64 && upper <= step { + return true; } + + false } _ => false, } } -fn get_index_value(expr: &Expr) -> Option { +fn get_effective_index(expr: &Expr) -> Option { match expr { Expr::Constant(ast::ExprConstant { value: Constant::Int(value), @@ -154,3 +177,10 @@ fn get_index_value(expr: &Expr) -> Option { _ => None, } } + +fn get_name_id(expr: &Expr) -> Option<&str> { + match expr { + Expr::Name(ast::ExprName { id, .. }) => Some(id), + _ => None, + } +} diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap new file mode 100644 index 0000000000000..03b99faa4f717 --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap @@ -0,0 +1,170 @@ +--- +source: crates/ruff/src/rules/ruff/mod.rs +--- +RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +3 | # RUF015 +4 | list(x)[0] + | ^^^^^^^^^^ RUF015 +5 | list(x)[:1] +6 | list(x)[:1:1] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +1 1 | x = range(10) +2 2 | +3 3 | # RUF015 +4 |-list(x)[0] + 4 |+next(iter(x)) +5 5 | list(x)[:1] +6 6 | list(x)[:1:1] +7 7 | list(x)[:1:2] + +RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +3 | # RUF015 +4 | list(x)[0] +5 | list(x)[:1] + | ^^^^^^^^^^^ RUF015 +6 | list(x)[:1:1] +7 | list(x)[:1:2] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +2 2 | +3 3 | # RUF015 +4 4 | list(x)[0] +5 |-list(x)[:1] + 5 |+next(iter(x)) +6 6 | list(x)[:1:1] +7 7 | list(x)[:1:2] +8 8 | [i for i in x][0] + +RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +4 | list(x)[0] +5 | list(x)[:1] +6 | list(x)[:1:1] + | ^^^^^^^^^^^^^ RUF015 +7 | list(x)[:1:2] +8 | [i for i in x][0] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +3 3 | # RUF015 +4 4 | list(x)[0] +5 5 | list(x)[:1] +6 |-list(x)[:1:1] + 6 |+next(iter(x)) +7 7 | list(x)[:1:2] +8 8 | [i for i in x][0] +9 9 | [i for i in x][:1] + +RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +5 | list(x)[:1] +6 | list(x)[:1:1] +7 | list(x)[:1:2] + | ^^^^^^^^^^^^^ RUF015 +8 | [i for i in x][0] +9 | [i for i in x][:1] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +4 4 | list(x)[0] +5 5 | list(x)[:1] +6 6 | list(x)[:1:1] +7 |-list(x)[:1:2] + 7 |+next(iter(x)) +8 8 | [i for i in x][0] +9 9 | [i for i in x][:1] +10 10 | [i for i in x][:1:1] + +RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | + 6 | list(x)[:1:1] + 7 | list(x)[:1:2] + 8 | [i for i in x][0] + | ^^^^^^^^^^^^^^^^^ RUF015 + 9 | [i for i in x][:1] +10 | [i for i in x][:1:1] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +5 5 | list(x)[:1] +6 6 | list(x)[:1:1] +7 7 | list(x)[:1:2] +8 |-[i for i in x][0] + 8 |+next(iter(x)) +9 9 | [i for i in x][:1] +10 10 | [i for i in x][:1:1] +11 11 | [i for i in x][:1:2] + +RUF015.py:9:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | + 7 | list(x)[:1:2] + 8 | [i for i in x][0] + 9 | [i for i in x][:1] + | ^^^^^^^^^^^^^^^^^^ RUF015 +10 | [i for i in x][:1:1] +11 | [i for i in x][:1:2] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +6 6 | list(x)[:1:1] +7 7 | list(x)[:1:2] +8 8 | [i for i in x][0] +9 |-[i for i in x][:1] + 9 |+next(iter(x)) +10 10 | [i for i in x][:1:1] +11 11 | [i for i in x][:1:2] +12 12 | + +RUF015.py:10:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | + 8 | [i for i in x][0] + 9 | [i for i in x][:1] +10 | [i for i in x][:1:1] + | ^^^^^^^^^^^^^^^^^^^^ RUF015 +11 | [i for i in x][:1:2] + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +7 7 | list(x)[:1:2] +8 8 | [i for i in x][0] +9 9 | [i for i in x][:1] +10 |-[i for i in x][:1:1] + 10 |+next(iter(x)) +11 11 | [i for i in x][:1:2] +12 12 | +13 13 | # Fine - not indexing (solely) the first element + +RUF015.py:11:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | + 9 | [i for i in x][:1] +10 | [i for i in x][:1:1] +11 | [i for i in x][:1:2] + | ^^^^^^^^^^^^^^^^^^^^ RUF015 +12 | +13 | # Fine - not indexing (solely) the first element + | + = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + +ℹ Suggested fix +8 8 | [i for i in x][0] +9 9 | [i for i in x][:1] +10 10 | [i for i in x][:1:1] +11 |-[i for i in x][:1:2] + 11 |+next(iter(x)) +12 12 | +13 13 | # Fine - not indexing (solely) the first element +14 14 | list(x) + + diff --git a/ruff.schema.json b/ruff.schema.json index f283f253b8865..ecbae5dc94cfe 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2401,6 +2401,7 @@ "RUF012", "RUF013", "RUF014", + "RUF015", "RUF1", "RUF10", "RUF100", @@ -2690,4 +2691,4 @@ "type": "string" } } -} \ No newline at end of file +} diff --git a/test.py b/test.py new file mode 100644 index 0000000000000..9f45b9da84075 --- /dev/null +++ b/test.py @@ -0,0 +1 @@ +"hello {}".format("wordl", ) From ee56d3fb2c4d173c89be9cc619c0aeaf7b1101fb Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Wed, 5 Jul 2023 20:05:53 -0500 Subject: [PATCH 05/18] Clippy --- crates/ruff/src/checkers/ast/mod.rs | 2 +- .../unnecessary_list_allocation_for_first_element.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c9681236a237f..665ea8c444964 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2226,7 +2226,7 @@ where } if self.enabled(Rule::UnnecessaryListAllocationForFirstElement) { ruff::rules::unnecessary_list_allocation_for_first_element( - self, value, slice, range, + self, value, slice, *range, ); } diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index d4d0c913eb7f4..5cc5796d18985 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -65,7 +65,7 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( checker: &mut Checker, call: &Expr, slice: &Expr, - subscript_range: &TextRange, + subscript_range: TextRange, ) { if !indexes_first_element(slice) { return; @@ -76,14 +76,14 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( let mut diagnostic = Diagnostic::new( UnnecessaryListAllocationForFirstElement::new(iter_name.to_string()), - *subscript_range, + subscript_range, ); if checker.patch(diagnostic.kind.rule()) { - let replacement = format!("next(iter({}))", iter_name); + let replacement = format!("next(iter({iter_name}))"); diagnostic.set_fix(Fix::suggested(Edit::range_replacement( replacement, - *subscript_range, + subscript_range, ))); } From 9a19cac7431464affae7fc6c9d15a7cf65474334 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 6 Jul 2023 09:12:01 -0500 Subject: [PATCH 06/18] Implement reviewer comments --- crates/ruff/src/checkers/ast/mod.rs | 11 ++--------- ...cessary_list_allocation_for_first_element.rs | 17 +++++++---------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 665ea8c444964..ad03ae0929642 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2139,12 +2139,7 @@ where // Pre-visit. match expr { - Expr::Subscript(ast::ExprSubscript { - value, - slice, - range, - .. - }) => { + subscript @ Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { // Ex) Optional[...], Union[...] if self.any_enabled(&[ Rule::FutureRewritableTypeAnnotation, @@ -2225,9 +2220,7 @@ where flake8_simplify::rules::use_capital_environment_variables(self, expr); } if self.enabled(Rule::UnnecessaryListAllocationForFirstElement) { - ruff::rules::unnecessary_list_allocation_for_first_element( - self, value, slice, *range, - ); + ruff::rules::unnecessary_list_allocation_for_first_element(self, subscript); } pandas_vet::rules::subscript(self, value, expr); diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 5cc5796d18985..5a110f8ce92a6 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -1,7 +1,6 @@ use num_traits::ToPrimitive; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_text_size::TextRange; use rustpython_parser::ast::{self, Constant, Expr}; use crate::checkers::ast::Checker; @@ -63,28 +62,26 @@ impl AlwaysAutofixableViolation for UnnecessaryListAllocationForFirstElement { /// RUF015 pub(crate) fn unnecessary_list_allocation_for_first_element( checker: &mut Checker, - call: &Expr, - slice: &Expr, - subscript_range: TextRange, + subscript: &Expr, ) { + let Expr::Subscript(ast::ExprSubscript { value, slice, range, .. }) = subscript else { + return; + }; if !indexes_first_element(slice) { return; } - let Some(iter_name) = get_iterable_name(checker, call) else { + let Some(iter_name) = get_iterable_name(checker, value) else { return; }; let mut diagnostic = Diagnostic::new( UnnecessaryListAllocationForFirstElement::new(iter_name.to_string()), - subscript_range, + *range, ); if checker.patch(diagnostic.kind.rule()) { let replacement = format!("next(iter({iter_name}))"); - diagnostic.set_fix(Fix::suggested(Edit::range_replacement( - replacement, - subscript_range, - ))); + diagnostic.set_fix(Fix::suggested(Edit::range_replacement(replacement, *range))); } checker.diagnostics.push(diagnostic); From 1f554415652ab852df85d040ccc6e95a7f172197 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 6 Jul 2023 09:56:03 -0500 Subject: [PATCH 07/18] Add Markdown header to docs --- .../ruff/rules/unnecessary_list_allocation_for_first_element.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 5a110f8ce92a6..886da94a551d9 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -10,7 +10,7 @@ use crate::registry::AsRule; /// Ensures that instead of creating a new list and indexing into it to find the first element of a /// collection (e.g., `list(...)[0]`), Python iterators are used. /// -/// Why is this bad? +/// ## Why is this bad? /// Creating a new list of great size can involve significant memory/speed concerns. Python's `next(iter(...))` /// pattern can be used in lieu of creating a new list. This pattern will lazily fetch the first /// element of the collection, avoiding the memory overhead involved with new list allocation. From c2db9510fc0a02f63da6d45589f31801e9803c5d Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 6 Jul 2023 13:56:07 -0500 Subject: [PATCH 08/18] Wrap fix in [] if slicing rather than indexing --- ...ssary_list_allocation_for_first_element.rs | 55 +++++++++++-------- ..._rules__ruff__tests__RUF015_RUF015.py.snap | 12 ++-- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 886da94a551d9..c21b43de7e853 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -1,6 +1,7 @@ use num_traits::ToPrimitive; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::contains_effect; use rustpython_parser::ast::{self, Constant, Expr}; use crate::checkers::ast::Checker; @@ -67,7 +68,9 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( let Expr::Subscript(ast::ExprSubscript { value, slice, range, .. }) = subscript else { return; }; - if !indexes_first_element(slice) { + + let (indexes_first_element, in_slice) = classify_subscript(slice); + if !indexes_first_element { return; } let Some(iter_name) = get_iterable_name(checker, value) else { @@ -80,7 +83,12 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( ); if checker.patch(diagnostic.kind.rule()) { - let replacement = format!("next(iter({iter_name}))"); + let replacement = if in_slice { + format!("[next(iter({iter_name}))]") + } else { + format!("next(iter({iter_name}))") + }; + diagnostic.set_fix(Fix::suggested(Edit::range_replacement(replacement, *range))); } @@ -130,11 +138,14 @@ fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a st } } -/// Check that the slice [`Expr`] is functionally equivalent to slicing into the first element. -fn indexes_first_element(expr: &Expr) -> bool { +/// Check that the slice [`Expr`] is functionally equivalent to slicing into the first element. The +/// first `bool` checks that the element is in fact first, the second checks if it's a slice or an +/// index. +fn classify_subscript(expr: &Expr) -> (bool, bool) { match expr { Expr::Constant(ast::ExprConstant { .. }) => { - get_effective_index(expr).unwrap_or(0i64) == 0i64 + let effective_index = get_effective_index(expr); + (acts_as_zero(effective_index), false) } Expr::Slice(ast::ExprSlice { step: step_value, @@ -142,26 +153,22 @@ fn indexes_first_element(expr: &Expr) -> bool { upper: upper_index, .. }) => { - let lower = lower_index - .as_ref() - .and_then(|l| get_effective_index(l)) - .unwrap_or(0i64); - let upper = upper_index - .as_ref() - .and_then(|u| get_effective_index(u)) - .unwrap_or(i64::MAX); - let step = step_value - .as_ref() - .and_then(|s| get_effective_index(s)) - .unwrap_or(1i64); - - if lower == 0i64 && upper <= step { - return true; + let lower = lower_index.as_ref().and_then(|l| get_effective_index(l)); + let upper = upper_index.as_ref().and_then(|u| get_effective_index(u)); + let step = step_value.as_ref().and_then(|s| get_effective_index(s)); + + let in_slice = upper.is_some() || step.is_some(); + if acts_as_zero(lower) { + if upper.unwrap_or(i64::MAX) > step.unwrap_or(1i64) { + return (false, in_slice); + } + + return (true, in_slice); } - false + (false, in_slice) } - _ => false, + _ => (false, false), } } @@ -181,3 +188,7 @@ fn get_name_id(expr: &Expr) -> Option<&str> { _ => None, } } + +fn acts_as_zero(i: Option) -> bool { + i.is_none() || i == Some(0i64) +} diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap index 03b99faa4f717..0f0cdf05b2f7f 100644 --- a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap @@ -37,7 +37,7 @@ RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 3 3 | # RUF015 4 4 | list(x)[0] 5 |-list(x)[:1] - 5 |+next(iter(x)) + 5 |+[next(iter(x))] 6 6 | list(x)[:1:1] 7 7 | list(x)[:1:2] 8 8 | [i for i in x][0] @@ -58,7 +58,7 @@ RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 4 4 | list(x)[0] 5 5 | list(x)[:1] 6 |-list(x)[:1:1] - 6 |+next(iter(x)) + 6 |+[next(iter(x))] 7 7 | list(x)[:1:2] 8 8 | [i for i in x][0] 9 9 | [i for i in x][:1] @@ -79,7 +79,7 @@ RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 5 5 | list(x)[:1] 6 6 | list(x)[:1:1] 7 |-list(x)[:1:2] - 7 |+next(iter(x)) + 7 |+[next(iter(x))] 8 8 | [i for i in x][0] 9 9 | [i for i in x][:1] 10 10 | [i for i in x][:1:1] @@ -121,7 +121,7 @@ RUF015.py:9:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 7 7 | list(x)[:1:2] 8 8 | [i for i in x][0] 9 |-[i for i in x][:1] - 9 |+next(iter(x)) + 9 |+[next(iter(x))] 10 10 | [i for i in x][:1:1] 11 11 | [i for i in x][:1:2] 12 12 | @@ -141,7 +141,7 @@ RUF015.py:10:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 8 8 | [i for i in x][0] 9 9 | [i for i in x][:1] 10 |-[i for i in x][:1:1] - 10 |+next(iter(x)) + 10 |+[next(iter(x))] 11 11 | [i for i in x][:1:2] 12 12 | 13 13 | # Fine - not indexing (solely) the first element @@ -162,7 +162,7 @@ RUF015.py:11:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 9 9 | [i for i in x][:1] 10 10 | [i for i in x][:1:1] 11 |-[i for i in x][:1:2] - 11 |+next(iter(x)) + 11 |+[next(iter(x))] 12 12 | 13 13 | # Fine - not indexing (solely) the first element 14 14 | list(x) From f2fbf2c44e73e55bf8a714d5104f90e2bce071d9 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 6 Jul 2023 14:19:12 -0500 Subject: [PATCH 09/18] Add test --- crates/ruff/resources/test/fixtures/ruff/RUF015.py | 2 ++ .../ruff/rules/unnecessary_list_allocation_for_first_element.rs | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py index 8967253afa33d..b7b3112fae9f0 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF015.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF015.py @@ -17,12 +17,14 @@ list(x)[1:] list(x)[:3:2] list(x)[::2] +list(x)[::] [i for i in x] [i for i in x][1] [i for i in x][-1] [i for i in x][1:] [i for i in x][:3:2] [i for i in x][::2] +[i for i in x][::] # Fine - modifies the first element [i + 1 for i in x][0] diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index c21b43de7e853..0db37f03653ef 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -1,7 +1,6 @@ use num_traits::ToPrimitive; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::contains_effect; use rustpython_parser::ast::{self, Constant, Expr}; use crate::checkers::ast::Checker; From f582b48e91921f65a3103bfe82359b90bb3f26f1 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Thu, 6 Jul 2023 16:31:31 -0500 Subject: [PATCH 10/18] Fitler `ifs` from ListComp expressions --- crates/ruff/resources/test/fixtures/ruff/RUF015.py | 4 +++- .../rules/unnecessary_list_allocation_for_first_element.rs | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py index b7b3112fae9f0..ca269c6d8b57a 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF015.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF015.py @@ -26,8 +26,10 @@ [i for i in x][::2] [i for i in x][::] -# Fine - modifies the first element +# Fine - doesn't mirror the underlying list [i + 1 for i in x][0] +[i for i in x if i > 5][0] +[(i, i + 1) for i in x][0] y = range(10) # Fine - multiple generators diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 0db37f03653ef..3835b69600ffa 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -128,6 +128,11 @@ fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a st } let generator = &generators[0]; + // Ignore if there's an `if` statement in the comprehension, since it filters the list. + if !generator.ifs.is_empty() { + return None; + } + let Some(arg_name) = get_name_id(&generator.iter) else { return None; }; From 4f4b2c8e340d05a76398bb729f34346b1a669a59 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Fri, 7 Jul 2023 20:33:58 -0500 Subject: [PATCH 11/18] Restrict visibility to crate --- .../ruff/rules/unnecessary_list_allocation_for_first_element.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs index 3835b69600ffa..90172e05ec884 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs @@ -32,7 +32,7 @@ use crate::registry::AsRule; /// - [Iterators and Iterables in Python: Run Efficient /// Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) #[violation] -pub struct UnnecessaryListAllocationForFirstElement { +pub(crate) struct UnnecessaryListAllocationForFirstElement { arg: String, } From f1126c8932542deb78164334596da18a316316c8 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sun, 9 Jul 2023 10:51:06 -0500 Subject: [PATCH 12/18] Genericize generator name extraction --- .../resources/test/fixtures/ruff/RUF015.py | 9 +- crates/ruff/src/checkers/ast/mod.rs | 4 +- crates/ruff/src/codes.rs | 2 +- crates/ruff/src/rules/ruff/mod.rs | 5 +- crates/ruff/src/rules/ruff/rules/mod.rs | 4 +- ..._iterable_allocation_for_first_element.rs} | 175 ++++++----- ..._rules__ruff__tests__RUF015_RUF015.py.snap | 286 ++++++++++++++---- 7 files changed, 344 insertions(+), 141 deletions(-) rename crates/ruff/src/rules/ruff/rules/{unnecessary_list_allocation_for_first_element.rs => unnecessary_iterable_allocation_for_first_element.rs} (56%) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py index ca269c6d8b57a..a8ce58f2dc039 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF015.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF015.py @@ -5,6 +5,14 @@ list(x)[:1] list(x)[:1:1] list(x)[:1:2] +tuple(x)[0] +tuple(x)[:1] +tuple(x)[:1:1] +tuple(x)[:1:2] +list(i for i in x)[0] +list(i for i in x)[:1] +list(i for i in x)[:1:1] +list(i for i in x)[:1:2] [i for i in x][0] [i for i in x][:1] [i for i in x][:1:1] @@ -34,4 +42,3 @@ y = range(10) # Fine - multiple generators [i + j for i in x for j in y][0] - diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index ad03ae0929642..36b9ff49bfc04 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2219,8 +2219,8 @@ where if self.enabled(Rule::UncapitalizedEnvironmentVariables) { flake8_simplify::rules::use_capital_environment_variables(self, expr); } - if self.enabled(Rule::UnnecessaryListAllocationForFirstElement) { - ruff::rules::unnecessary_list_allocation_for_first_element(self, subscript); + if self.enabled(Rule::UnnecessaryIterableAllocationForFirstElement) { + ruff::rules::unnecessary_iterable_allocation_for_first_element(self, subscript); } pandas_vet::rules::subscript(self, value, expr); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 8de023011b0cd..1e2971891cfaa 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -778,7 +778,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "013") => (RuleGroup::Unspecified, rules::ruff::rules::ImplicitOptional), #[cfg(feature = "unreachable-code")] (Ruff, "014") => (RuleGroup::Nursery, rules::ruff::rules::UnreachableCode), - (Ruff, "015") => (RuleGroup::Unspecified, rules::ruff::rules::UnnecessaryListAllocationForFirstElement), + (Ruff, "015") => (RuleGroup::Unspecified, rules::ruff::rules::UnnecessaryIterableAllocationForFirstElement), (Ruff, "100") => (RuleGroup::Unspecified, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Unspecified, rules::ruff::rules::InvalidPyprojectToml), diff --git a/crates/ruff/src/rules/ruff/mod.rs b/crates/ruff/src/rules/ruff/mod.rs index 262dcaea88ed9..1b11784cabd58 100644 --- a/crates/ruff/src/rules/ruff/mod.rs +++ b/crates/ruff/src/rules/ruff/mod.rs @@ -30,7 +30,10 @@ mod tests { #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008.py"))] #[test_case(Rule::PairwiseOverZipped, Path::new("RUF007.py"))] #[test_case(Rule::StaticKeyDictComprehension, Path::new("RUF011.py"))] - #[test_case(Rule::UnnecessaryListAllocationForFirstElement, Path::new("RUF015.py"))] + #[test_case( + Rule::UnnecessaryIterableAllocationForFirstElement, + Path::new("RUF015.py") + )] #[cfg_attr( feature = "unreachable-code", test_case(Rule::UnreachableCode, Path::new("RUF014.py")) diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index b290a05ae56a3..e6654f1204d71 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -9,7 +9,7 @@ pub(crate) use mutable_class_default::*; pub(crate) use mutable_dataclass_default::*; pub(crate) use pairwise_over_zipped::*; pub(crate) use static_key_dict_comprehension::*; -pub(crate) use unnecessary_list_allocation_for_first_element::*; +pub(crate) use unnecessary_iterable_allocation_for_first_element::*; #[cfg(feature = "unreachable-code")] pub(crate) use unreachable::*; pub(crate) use unused_noqa::*; @@ -27,7 +27,7 @@ mod mutable_class_default; mod mutable_dataclass_default; mod pairwise_over_zipped; mod static_key_dict_comprehension; -mod unnecessary_list_allocation_for_first_element; +mod unnecessary_iterable_allocation_for_first_element; #[cfg(feature = "unreachable-code")] pub(crate) mod unreachable; mod unused_noqa; diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs similarity index 56% rename from crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs rename to crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs index 90172e05ec884..938da41a29093 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_list_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs @@ -1,7 +1,7 @@ use num_traits::ToPrimitive; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use rustpython_parser::ast::{self, Constant, Expr}; +use rustpython_parser::ast::{self, Comprehension, Constant, Expr}; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -32,17 +32,17 @@ use crate::registry::AsRule; /// - [Iterators and Iterables in Python: Run Efficient /// Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) #[violation] -pub(crate) struct UnnecessaryListAllocationForFirstElement { +pub(crate) struct UnnecessaryIterableAllocationForFirstElement { arg: String, } -impl UnnecessaryListAllocationForFirstElement { +impl UnnecessaryIterableAllocationForFirstElement { pub(crate) fn new(arg: String) -> Self { Self { arg } } } -impl AlwaysAutofixableViolation for UnnecessaryListAllocationForFirstElement { +impl AlwaysAutofixableViolation for UnnecessaryIterableAllocationForFirstElement { #[derive_message_formats] fn message(&self) -> String { format!( @@ -52,15 +52,30 @@ impl AlwaysAutofixableViolation for UnnecessaryListAllocationForFirstElement { } fn autofix_title(&self) -> String { - format!( - "Convert `list({})[0]` or equivalent list comprehension call to `next(iter({}))`", - self.arg, self.arg - ) + format!("Replace with `next(iter({}))`", self.arg) + } +} + +/// Contains information about a [`Expr::Subscript`] to determine if and how it accesses the first element +/// of an iterable. +struct ClassifiedSubscript { + /// If the subscript accesses the first element of the iterable + indexes_first_element: bool, + /// If the subscript is a slice (e.g., `[:1]`) + is_slice: bool, +} + +impl ClassifiedSubscript { + fn new(indexes_first_element: bool, is_slice: bool) -> Self { + Self { + indexes_first_element, + is_slice, + } } } /// RUF015 -pub(crate) fn unnecessary_list_allocation_for_first_element( +pub(crate) fn unnecessary_iterable_allocation_for_first_element( checker: &mut Checker, subscript: &Expr, ) { @@ -68,21 +83,25 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( return; }; - let (indexes_first_element, in_slice) = classify_subscript(slice); + let ClassifiedSubscript { + indexes_first_element, + is_slice, + } = classify_subscript(slice); if !indexes_first_element { return; } + let Some(iter_name) = get_iterable_name(checker, value) else { return; }; let mut diagnostic = Diagnostic::new( - UnnecessaryListAllocationForFirstElement::new(iter_name.to_string()), + UnnecessaryIterableAllocationForFirstElement::new(iter_name.to_string()), *range, ); if checker.patch(diagnostic.kind.rule()) { - let replacement = if in_slice { + let replacement = if is_slice { format!("[next(iter({iter_name}))]") } else { format!("next(iter({iter_name}))") @@ -94,62 +113,14 @@ pub(crate) fn unnecessary_list_allocation_for_first_element( checker.diagnostics.push(diagnostic); } -/// Fetch the name of the iterable from a list expression if the expression returns an unmodified list -/// which can be sliced into. -fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a str> { - match expr { - Expr::Call(ast::ExprCall { func, args, .. }) => { - let Some(id) = get_name_id(func.as_ref()) else { - return None; - }; - if !(id == "list" && checker.semantic().is_builtin("list")) { - return None; - } - - let Some(Expr::Name(ast::ExprName { id: arg_name, .. })) = args.first() else { - return None; - }; - - Some(arg_name.as_str()) - } - Expr::ListComp(ast::ExprListComp { - elt, generators, .. - }) => { - // If the `elt` field is anything other than a [`Expr::Name`], we can't be sure that it - // doesn't modify the elements of the underlying iterator - for example, `[i + 1 for i in x][0]`. - if !matches!(elt.as_ref(), Expr::Name(ast::ExprName { .. })) { - return None; - } - - // If there's more than 1 generator, we can't safely say that it fits the diagnostic conditions - - // for example, `[(i, j) for i in x for j in y][0]`. - if generators.len() != 1 { - return None; - } - - let generator = &generators[0]; - // Ignore if there's an `if` statement in the comprehension, since it filters the list. - if !generator.ifs.is_empty() { - return None; - } - - let Some(arg_name) = get_name_id(&generator.iter) else { - return None; - }; - Some(arg_name) - } - _ => None, - } -} - /// Check that the slice [`Expr`] is functionally equivalent to slicing into the first element. The /// first `bool` checks that the element is in fact first, the second checks if it's a slice or an /// index. -fn classify_subscript(expr: &Expr) -> (bool, bool) { +fn classify_subscript(expr: &Expr) -> ClassifiedSubscript { match expr { Expr::Constant(ast::ExprConstant { .. }) => { let effective_index = get_effective_index(expr); - (acts_as_zero(effective_index), false) + ClassifiedSubscript::new(matches!(effective_index, None | Some(0)), false) } Expr::Slice(ast::ExprSlice { step: step_value, @@ -161,31 +132,79 @@ fn classify_subscript(expr: &Expr) -> (bool, bool) { let upper = upper_index.as_ref().and_then(|u| get_effective_index(u)); let step = step_value.as_ref().and_then(|s| get_effective_index(s)); - let in_slice = upper.is_some() || step.is_some(); - if acts_as_zero(lower) { + let is_slice = upper.is_some() || step.is_some(); + if matches!(lower, None | Some(0)) { if upper.unwrap_or(i64::MAX) > step.unwrap_or(1i64) { - return (false, in_slice); + return ClassifiedSubscript::new(false, is_slice); } - return (true, in_slice); + return ClassifiedSubscript::new(true, is_slice); } - (false, in_slice) + ClassifiedSubscript::new(false, is_slice) } - _ => (false, false), + _ => ClassifiedSubscript::new(false, false), } } -fn get_effective_index(expr: &Expr) -> Option { +/// Fetch the name of the iterable from an expression if the expression returns an unmodified list +/// which can be sliced into. +fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a str> { match expr { - Expr::Constant(ast::ExprConstant { - value: Constant::Int(value), - .. - }) => value.to_i64(), + Expr::Call(ast::ExprCall { func, args, .. }) => { + let Some(id) = get_name_id(func.as_ref()) else { + return None; + }; + if !((id == "list" && checker.semantic().is_builtin("list")) + || (id == "tuple" && checker.semantic().is_builtin("tuple"))) + { + return None; + } + + match args.first() { + Some(Expr::Name(ast::ExprName { id: arg_name, .. })) => Some(arg_name.as_str()), + Some(Expr::GeneratorExp(ast::ExprGeneratorExp { + elt, generators, .. + })) => get_generator_iterable(elt, generators), + _ => None, + } + } + Expr::ListComp(ast::ExprListComp { + elt, generators, .. + }) => get_generator_iterable(elt, generators), _ => None, } } +fn get_generator_iterable<'a>( + elt: &'a Expr, + generators: &'a Vec, +) -> Option<&'a str> { + // If the `elt` field is anything other than a [`Expr::Name`], we can't be sure that it + // doesn't modify the elements of the underlying iterator - for example, `[i + 1 for i in x][0]`. + if !matches!(elt, Expr::Name(ast::ExprName { .. })) { + return None; + } + + // If there's more than 1 generator, we can't safely say that it fits the diagnostic conditions - + // for example, `[(i, j) for i in x for j in y][0]`. + if generators.len() != 1 { + return None; + } + + let generator = &generators[0]; + // Ignore if there's an `if` statement in the comprehension, since it filters the list. + if !generator.ifs.is_empty() { + return None; + } + + let Some(arg_name) = get_name_id(&generator.iter) else { + return None; + }; + + Some(arg_name) +} + fn get_name_id(expr: &Expr) -> Option<&str> { match expr { Expr::Name(ast::ExprName { id, .. }) => Some(id), @@ -193,6 +212,12 @@ fn get_name_id(expr: &Expr) -> Option<&str> { } } -fn acts_as_zero(i: Option) -> bool { - i.is_none() || i == Some(0i64) +fn get_effective_index(expr: &Expr) -> Option { + match expr { + Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) => value.to_i64(), + _ => None, + } } diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap index 0f0cdf05b2f7f..258548c85896c 100644 --- a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap @@ -9,7 +9,7 @@ RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 5 | list(x)[:1] 6 | list(x)[:1:1] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 1 1 | x = range(10) @@ -30,7 +30,7 @@ RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 | list(x)[:1:1] 7 | list(x)[:1:2] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 2 2 | @@ -40,7 +40,7 @@ RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 5 |+[next(iter(x))] 6 6 | list(x)[:1:1] 7 7 | list(x)[:1:2] -8 8 | [i for i in x][0] +8 8 | tuple(x)[0] RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension | @@ -49,9 +49,9 @@ RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 | list(x)[:1:1] | ^^^^^^^^^^^^^ RUF015 7 | list(x)[:1:2] -8 | [i for i in x][0] +8 | tuple(x)[0] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 3 3 | # RUF015 @@ -60,8 +60,8 @@ RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 |-list(x)[:1:1] 6 |+[next(iter(x))] 7 7 | list(x)[:1:2] -8 8 | [i for i in x][0] -9 9 | [i for i in x][:1] +8 8 | tuple(x)[0] +9 9 | tuple(x)[:1] RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension | @@ -69,10 +69,10 @@ RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 | list(x)[:1:1] 7 | list(x)[:1:2] | ^^^^^^^^^^^^^ RUF015 -8 | [i for i in x][0] -9 | [i for i in x][:1] +8 | tuple(x)[0] +9 | tuple(x)[:1] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 4 4 | list(x)[0] @@ -80,91 +80,259 @@ RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 6 | list(x)[:1:1] 7 |-list(x)[:1:2] 7 |+[next(iter(x))] -8 8 | [i for i in x][0] -9 9 | [i for i in x][:1] -10 10 | [i for i in x][:1:1] +8 8 | tuple(x)[0] +9 9 | tuple(x)[:1] +10 10 | tuple(x)[:1:1] RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension | 6 | list(x)[:1:1] 7 | list(x)[:1:2] - 8 | [i for i in x][0] - | ^^^^^^^^^^^^^^^^^ RUF015 - 9 | [i for i in x][:1] -10 | [i for i in x][:1:1] + 8 | tuple(x)[0] + | ^^^^^^^^^^^ RUF015 + 9 | tuple(x)[:1] +10 | tuple(x)[:1:1] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 5 5 | list(x)[:1] 6 6 | list(x)[:1:1] 7 7 | list(x)[:1:2] -8 |-[i for i in x][0] +8 |-tuple(x)[0] 8 |+next(iter(x)) -9 9 | [i for i in x][:1] -10 10 | [i for i in x][:1:1] -11 11 | [i for i in x][:1:2] +9 9 | tuple(x)[:1] +10 10 | tuple(x)[:1:1] +11 11 | tuple(x)[:1:2] RUF015.py:9:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension | 7 | list(x)[:1:2] - 8 | [i for i in x][0] - 9 | [i for i in x][:1] - | ^^^^^^^^^^^^^^^^^^ RUF015 -10 | [i for i in x][:1:1] -11 | [i for i in x][:1:2] + 8 | tuple(x)[0] + 9 | tuple(x)[:1] + | ^^^^^^^^^^^^ RUF015 +10 | tuple(x)[:1:1] +11 | tuple(x)[:1:2] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 6 6 | list(x)[:1:1] 7 7 | list(x)[:1:2] -8 8 | [i for i in x][0] -9 |-[i for i in x][:1] +8 8 | tuple(x)[0] +9 |-tuple(x)[:1] 9 |+[next(iter(x))] -10 10 | [i for i in x][:1:1] -11 11 | [i for i in x][:1:2] -12 12 | +10 10 | tuple(x)[:1:1] +11 11 | tuple(x)[:1:2] +12 12 | list(i for i in x)[0] RUF015.py:10:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension | - 8 | [i for i in x][0] - 9 | [i for i in x][:1] -10 | [i for i in x][:1:1] - | ^^^^^^^^^^^^^^^^^^^^ RUF015 -11 | [i for i in x][:1:2] + 8 | tuple(x)[0] + 9 | tuple(x)[:1] +10 | tuple(x)[:1:1] + | ^^^^^^^^^^^^^^ RUF015 +11 | tuple(x)[:1:2] +12 | list(i for i in x)[0] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix 7 7 | list(x)[:1:2] -8 8 | [i for i in x][0] -9 9 | [i for i in x][:1] -10 |-[i for i in x][:1:1] +8 8 | tuple(x)[0] +9 9 | tuple(x)[:1] +10 |-tuple(x)[:1:1] 10 |+[next(iter(x))] -11 11 | [i for i in x][:1:2] -12 12 | -13 13 | # Fine - not indexing (solely) the first element +11 11 | tuple(x)[:1:2] +12 12 | list(i for i in x)[0] +13 13 | list(i for i in x)[:1] RUF015.py:11:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension | - 9 | [i for i in x][:1] -10 | [i for i in x][:1:1] -11 | [i for i in x][:1:2] - | ^^^^^^^^^^^^^^^^^^^^ RUF015 -12 | -13 | # Fine - not indexing (solely) the first element + 9 | tuple(x)[:1] +10 | tuple(x)[:1:1] +11 | tuple(x)[:1:2] + | ^^^^^^^^^^^^^^ RUF015 +12 | list(i for i in x)[0] +13 | list(i for i in x)[:1] | - = help: Convert `list(x)[0]` or equivalent list comprehension call to `next(iter(x))` + = help: Replace with `next(iter(x))` ℹ Suggested fix -8 8 | [i for i in x][0] -9 9 | [i for i in x][:1] -10 10 | [i for i in x][:1:1] -11 |-[i for i in x][:1:2] +8 8 | tuple(x)[0] +9 9 | tuple(x)[:1] +10 10 | tuple(x)[:1:1] +11 |-tuple(x)[:1:2] 11 |+[next(iter(x))] -12 12 | -13 13 | # Fine - not indexing (solely) the first element -14 14 | list(x) +12 12 | list(i for i in x)[0] +13 13 | list(i for i in x)[:1] +14 14 | list(i for i in x)[:1:1] + +RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +10 | tuple(x)[:1:1] +11 | tuple(x)[:1:2] +12 | list(i for i in x)[0] + | ^^^^^^^^^^^^^^^^^^^^^ RUF015 +13 | list(i for i in x)[:1] +14 | list(i for i in x)[:1:1] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +9 9 | tuple(x)[:1] +10 10 | tuple(x)[:1:1] +11 11 | tuple(x)[:1:2] +12 |-list(i for i in x)[0] + 12 |+next(iter(x)) +13 13 | list(i for i in x)[:1] +14 14 | list(i for i in x)[:1:1] +15 15 | list(i for i in x)[:1:2] + +RUF015.py:13:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +11 | tuple(x)[:1:2] +12 | list(i for i in x)[0] +13 | list(i for i in x)[:1] + | ^^^^^^^^^^^^^^^^^^^^^^ RUF015 +14 | list(i for i in x)[:1:1] +15 | list(i for i in x)[:1:2] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +10 10 | tuple(x)[:1:1] +11 11 | tuple(x)[:1:2] +12 12 | list(i for i in x)[0] +13 |-list(i for i in x)[:1] + 13 |+[next(iter(x))] +14 14 | list(i for i in x)[:1:1] +15 15 | list(i for i in x)[:1:2] +16 16 | [i for i in x][0] + +RUF015.py:14:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +12 | list(i for i in x)[0] +13 | list(i for i in x)[:1] +14 | list(i for i in x)[:1:1] + | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF015 +15 | list(i for i in x)[:1:2] +16 | [i for i in x][0] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +11 11 | tuple(x)[:1:2] +12 12 | list(i for i in x)[0] +13 13 | list(i for i in x)[:1] +14 |-list(i for i in x)[:1:1] + 14 |+[next(iter(x))] +15 15 | list(i for i in x)[:1:2] +16 16 | [i for i in x][0] +17 17 | [i for i in x][:1] + +RUF015.py:15:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +13 | list(i for i in x)[:1] +14 | list(i for i in x)[:1:1] +15 | list(i for i in x)[:1:2] + | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF015 +16 | [i for i in x][0] +17 | [i for i in x][:1] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +12 12 | list(i for i in x)[0] +13 13 | list(i for i in x)[:1] +14 14 | list(i for i in x)[:1:1] +15 |-list(i for i in x)[:1:2] + 15 |+[next(iter(x))] +16 16 | [i for i in x][0] +17 17 | [i for i in x][:1] +18 18 | [i for i in x][:1:1] + +RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +14 | list(i for i in x)[:1:1] +15 | list(i for i in x)[:1:2] +16 | [i for i in x][0] + | ^^^^^^^^^^^^^^^^^ RUF015 +17 | [i for i in x][:1] +18 | [i for i in x][:1:1] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +13 13 | list(i for i in x)[:1] +14 14 | list(i for i in x)[:1:1] +15 15 | list(i for i in x)[:1:2] +16 |-[i for i in x][0] + 16 |+next(iter(x)) +17 17 | [i for i in x][:1] +18 18 | [i for i in x][:1:1] +19 19 | [i for i in x][:1:2] + +RUF015.py:17:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +15 | list(i for i in x)[:1:2] +16 | [i for i in x][0] +17 | [i for i in x][:1] + | ^^^^^^^^^^^^^^^^^^ RUF015 +18 | [i for i in x][:1:1] +19 | [i for i in x][:1:2] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +14 14 | list(i for i in x)[:1:1] +15 15 | list(i for i in x)[:1:2] +16 16 | [i for i in x][0] +17 |-[i for i in x][:1] + 17 |+[next(iter(x))] +18 18 | [i for i in x][:1:1] +19 19 | [i for i in x][:1:2] +20 20 | + +RUF015.py:18:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +16 | [i for i in x][0] +17 | [i for i in x][:1] +18 | [i for i in x][:1:1] + | ^^^^^^^^^^^^^^^^^^^^ RUF015 +19 | [i for i in x][:1:2] + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +15 15 | list(i for i in x)[:1:2] +16 16 | [i for i in x][0] +17 17 | [i for i in x][:1] +18 |-[i for i in x][:1:1] + 18 |+[next(iter(x))] +19 19 | [i for i in x][:1:2] +20 20 | +21 21 | # Fine - not indexing (solely) the first element + +RUF015.py:19:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension + | +17 | [i for i in x][:1] +18 | [i for i in x][:1:1] +19 | [i for i in x][:1:2] + | ^^^^^^^^^^^^^^^^^^^^ RUF015 +20 | +21 | # Fine - not indexing (solely) the first element + | + = help: Replace with `next(iter(x))` + +ℹ Suggested fix +16 16 | [i for i in x][0] +17 17 | [i for i in x][:1] +18 18 | [i for i in x][:1:1] +19 |-[i for i in x][:1:2] + 19 |+[next(iter(x))] +20 20 | +21 21 | # Fine - not indexing (solely) the first element +22 22 | list(x) From 3b1513af9561ae8f5d132b10d2b93de44543bc05 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sun, 9 Jul 2023 14:30:50 -0500 Subject: [PATCH 13/18] Remove scratch file --- test.py | 1 - 1 file changed, 1 deletion(-) delete mode 100644 test.py diff --git a/test.py b/test.py deleted file mode 100644 index 9f45b9da84075..0000000000000 --- a/test.py +++ /dev/null @@ -1 +0,0 @@ -"hello {}".format("wordl", ) From 4497b80fce4482efef2062ef509cd652c771d39f Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sun, 9 Jul 2023 16:16:01 -0500 Subject: [PATCH 14/18] Fix schema --- ruff.schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruff.schema.json b/ruff.schema.json index ecbae5dc94cfe..d4f890fcfc685 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2691,4 +2691,4 @@ "type": "string" } } -} +} \ No newline at end of file From 8a62d9306f3f78c0cff34c11bd44170aa37a1164 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Mon, 10 Jul 2023 06:46:40 -0500 Subject: [PATCH 15/18] Add slice to message --- ...y_iterable_allocation_for_first_element.rs | 38 +++++++++---- ..._rules__ruff__tests__RUF015_RUF015.py.snap | 56 +++++++++---------- 2 files changed, 54 insertions(+), 40 deletions(-) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs index 938da41a29093..8c7a1ca82c67f 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs @@ -34,25 +34,40 @@ use crate::registry::AsRule; #[violation] pub(crate) struct UnnecessaryIterableAllocationForFirstElement { arg: String, + contains_slice: bool, } impl UnnecessaryIterableAllocationForFirstElement { - pub(crate) fn new(arg: String) -> Self { - Self { arg } + pub(crate) fn new(arg: String, contains_slice: bool) -> Self { + Self { + arg, + contains_slice, + } } } impl AlwaysAutofixableViolation for UnnecessaryIterableAllocationForFirstElement { #[derive_message_formats] fn message(&self) -> String { - format!( - "Prefer `next(iter({}))` over `list({})[0]` or equivalent list comprehension", - self.arg, self.arg - ) + if self.contains_slice { + format!( + "Prefer `[next(iter({}))]` over `list({})[:1]` or equivalent list comprehension/slice", + self.arg, self.arg + ) + } else { + format!( + "Prefer `next(iter({}))` over `list({})[0]` or equivalent list comprehension/slice", + self.arg, self.arg + ) + } } fn autofix_title(&self) -> String { - format!("Replace with `next(iter({}))`", self.arg) + if self.contains_slice { + format!("Replace with `[next(iter({}))]", self.arg) + } else { + format!("Replace with `next(iter({}))`", self.arg) + } } } @@ -96,7 +111,7 @@ pub(crate) fn unnecessary_iterable_allocation_for_first_element( }; let mut diagnostic = Diagnostic::new( - UnnecessaryIterableAllocationForFirstElement::new(iter_name.to_string()), + UnnecessaryIterableAllocationForFirstElement::new(iter_name.to_string(), is_slice), *range, ); @@ -132,16 +147,15 @@ fn classify_subscript(expr: &Expr) -> ClassifiedSubscript { let upper = upper_index.as_ref().and_then(|u| get_effective_index(u)); let step = step_value.as_ref().and_then(|s| get_effective_index(s)); - let is_slice = upper.is_some() || step.is_some(); if matches!(lower, None | Some(0)) { if upper.unwrap_or(i64::MAX) > step.unwrap_or(1i64) { - return ClassifiedSubscript::new(false, is_slice); + return ClassifiedSubscript::new(false, true); } - return ClassifiedSubscript::new(true, is_slice); + return ClassifiedSubscript::new(true, true); } - ClassifiedSubscript::new(false, is_slice) + ClassifiedSubscript::new(false, true) } _ => ClassifiedSubscript::new(false, false), } diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap index 258548c85896c..293611f8b3c15 100644 --- a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/ruff/mod.rs --- -RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice | 3 | # RUF015 4 | list(x)[0] @@ -21,7 +21,7 @@ RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 6 | list(x)[:1:1] 7 7 | list(x)[:1:2] -RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:5:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 3 | # RUF015 4 | list(x)[0] @@ -30,7 +30,7 @@ RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 | list(x)[:1:1] 7 | list(x)[:1:2] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 2 2 | @@ -42,7 +42,7 @@ RUF015.py:5:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 7 7 | list(x)[:1:2] 8 8 | tuple(x)[0] -RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:6:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 4 | list(x)[0] 5 | list(x)[:1] @@ -51,7 +51,7 @@ RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 7 | list(x)[:1:2] 8 | tuple(x)[0] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 3 3 | # RUF015 @@ -63,7 +63,7 @@ RUF015.py:6:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 8 8 | tuple(x)[0] 9 9 | tuple(x)[:1] -RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:7:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 5 | list(x)[:1] 6 | list(x)[:1:1] @@ -72,7 +72,7 @@ RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 8 | tuple(x)[0] 9 | tuple(x)[:1] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 4 4 | list(x)[0] @@ -84,7 +84,7 @@ RUF015.py:7:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 9 9 | tuple(x)[:1] 10 10 | tuple(x)[:1:1] -RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice | 6 | list(x)[:1:1] 7 | list(x)[:1:2] @@ -105,7 +105,7 @@ RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 10 10 | tuple(x)[:1:1] 11 11 | tuple(x)[:1:2] -RUF015.py:9:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:9:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 7 | list(x)[:1:2] 8 | tuple(x)[0] @@ -114,7 +114,7 @@ RUF015.py:9:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 10 | tuple(x)[:1:1] 11 | tuple(x)[:1:2] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 6 6 | list(x)[:1:1] @@ -126,7 +126,7 @@ RUF015.py:9:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 11 11 | tuple(x)[:1:2] 12 12 | list(i for i in x)[0] -RUF015.py:10:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:10:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 8 | tuple(x)[0] 9 | tuple(x)[:1] @@ -135,7 +135,7 @@ RUF015.py:10:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 11 | tuple(x)[:1:2] 12 | list(i for i in x)[0] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 7 7 | list(x)[:1:2] @@ -147,7 +147,7 @@ RUF015.py:10:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 12 12 | list(i for i in x)[0] 13 13 | list(i for i in x)[:1] -RUF015.py:11:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:11:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 9 | tuple(x)[:1] 10 | tuple(x)[:1:1] @@ -156,7 +156,7 @@ RUF015.py:11:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 12 | list(i for i in x)[0] 13 | list(i for i in x)[:1] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 8 8 | tuple(x)[0] @@ -168,7 +168,7 @@ RUF015.py:11:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 13 13 | list(i for i in x)[:1] 14 14 | list(i for i in x)[:1:1] -RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice | 10 | tuple(x)[:1:1] 11 | tuple(x)[:1:2] @@ -189,7 +189,7 @@ RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 14 14 | list(i for i in x)[:1:1] 15 15 | list(i for i in x)[:1:2] -RUF015.py:13:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:13:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 11 | tuple(x)[:1:2] 12 | list(i for i in x)[0] @@ -198,7 +198,7 @@ RUF015.py:13:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 14 | list(i for i in x)[:1:1] 15 | list(i for i in x)[:1:2] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 10 10 | tuple(x)[:1:1] @@ -210,7 +210,7 @@ RUF015.py:13:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 15 15 | list(i for i in x)[:1:2] 16 16 | [i for i in x][0] -RUF015.py:14:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:14:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 12 | list(i for i in x)[0] 13 | list(i for i in x)[:1] @@ -219,7 +219,7 @@ RUF015.py:14:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 15 | list(i for i in x)[:1:2] 16 | [i for i in x][0] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 11 11 | tuple(x)[:1:2] @@ -231,7 +231,7 @@ RUF015.py:14:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 16 16 | [i for i in x][0] 17 17 | [i for i in x][:1] -RUF015.py:15:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:15:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 13 | list(i for i in x)[:1] 14 | list(i for i in x)[:1:1] @@ -240,7 +240,7 @@ RUF015.py:15:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 16 | [i for i in x][0] 17 | [i for i in x][:1] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 12 12 | list(i for i in x)[0] @@ -252,7 +252,7 @@ RUF015.py:15:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 17 17 | [i for i in x][:1] 18 18 | [i for i in x][:1:1] -RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice | 14 | list(i for i in x)[:1:1] 15 | list(i for i in x)[:1:2] @@ -273,7 +273,7 @@ RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 18 18 | [i for i in x][:1:1] 19 19 | [i for i in x][:1:2] -RUF015.py:17:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:17:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 15 | list(i for i in x)[:1:2] 16 | [i for i in x][0] @@ -282,7 +282,7 @@ RUF015.py:17:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 18 | [i for i in x][:1:1] 19 | [i for i in x][:1:2] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 14 14 | list(i for i in x)[:1:1] @@ -294,7 +294,7 @@ RUF015.py:17:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 19 19 | [i for i in x][:1:2] 20 20 | -RUF015.py:18:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:18:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 16 | [i for i in x][0] 17 | [i for i in x][:1] @@ -302,7 +302,7 @@ RUF015.py:18:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen | ^^^^^^^^^^^^^^^^^^^^ RUF015 19 | [i for i in x][:1:2] | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 15 15 | list(i for i in x)[:1:2] @@ -314,7 +314,7 @@ RUF015.py:18:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 20 20 | 21 21 | # Fine - not indexing (solely) the first element -RUF015.py:19:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension +RUF015.py:19:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice | 17 | [i for i in x][:1] 18 | [i for i in x][:1:1] @@ -323,7 +323,7 @@ RUF015.py:19:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 20 | 21 | # Fine - not indexing (solely) the first element | - = help: Replace with `next(iter(x))` + = help: Replace with `[next(iter(x))] ℹ Suggested fix 16 16 | [i for i in x][0] From e0e082e38d1bc3d2fe0fa66eb4185979421fa7a0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 10 Jul 2023 11:46:12 -0400 Subject: [PATCH 16/18] Nits --- ...y_iterable_allocation_for_first_element.rs | 68 ++++++++----------- 1 file changed, 27 insertions(+), 41 deletions(-) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs index 8c7a1ca82c67f..cda4ec5f5dcfa 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs @@ -1,6 +1,7 @@ use num_traits::ToPrimitive; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::SemanticModel; use rustpython_parser::ast::{self, Comprehension, Constant, Expr}; use crate::checkers::ast::Checker; @@ -71,12 +72,12 @@ impl AlwaysAutofixableViolation for UnnecessaryIterableAllocationForFirstElement } } -/// Contains information about a [`Expr::Subscript`] to determine if and how it accesses the first element -/// of an iterable. +/// Contains information about a [`Expr::Subscript`] to determine if and how it accesses the first +/// element of an iterable. struct ClassifiedSubscript { - /// If the subscript accesses the first element of the iterable + /// `true` if the subscript accesses the first element of the iterable. indexes_first_element: bool, - /// If the subscript is a slice (e.g., `[:1]`) + /// `true` if the subscript is a slice (e.g., `[:1]`). is_slice: bool, } @@ -106,7 +107,7 @@ pub(crate) fn unnecessary_iterable_allocation_for_first_element( return; } - let Some(iter_name) = get_iterable_name(checker, value) else { + let Some(iter_name) = iterable_name(value, checker.semantic()) else { return; }; @@ -134,7 +135,7 @@ pub(crate) fn unnecessary_iterable_allocation_for_first_element( fn classify_subscript(expr: &Expr) -> ClassifiedSubscript { match expr { Expr::Constant(ast::ExprConstant { .. }) => { - let effective_index = get_effective_index(expr); + let effective_index = effective_index(expr); ClassifiedSubscript::new(matches!(effective_index, None | Some(0)), false) } Expr::Slice(ast::ExprSlice { @@ -143,9 +144,9 @@ fn classify_subscript(expr: &Expr) -> ClassifiedSubscript { upper: upper_index, .. }) => { - let lower = lower_index.as_ref().and_then(|l| get_effective_index(l)); - let upper = upper_index.as_ref().and_then(|u| get_effective_index(u)); - let step = step_value.as_ref().and_then(|s| get_effective_index(s)); + let lower = lower_index.as_ref().and_then(|l| effective_index(l)); + let upper = upper_index.as_ref().and_then(|u| effective_index(u)); + let step = step_value.as_ref().and_then(|s| effective_index(s)); if matches!(lower, None | Some(0)) { if upper.unwrap_or(i64::MAX) > step.unwrap_or(1i64) { @@ -163,14 +164,13 @@ fn classify_subscript(expr: &Expr) -> ClassifiedSubscript { /// Fetch the name of the iterable from an expression if the expression returns an unmodified list /// which can be sliced into. -fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a str> { +fn iterable_name<'a>(expr: &'a Expr, model: &SemanticModel) -> Option<&'a str> { match expr { Expr::Call(ast::ExprCall { func, args, .. }) => { - let Some(id) = get_name_id(func.as_ref()) else { - return None; - }; - if !((id == "list" && checker.semantic().is_builtin("list")) - || (id == "tuple" && checker.semantic().is_builtin("tuple"))) + let ast::ExprName { id, .. } = func.as_name_expr()?; + + if !((id == "list" && model.is_builtin("list")) + || (id == "tuple" && model.is_builtin("tuple"))) { return None; } @@ -179,54 +179,40 @@ fn get_iterable_name<'a>(checker: &mut Checker, expr: &'a Expr) -> Option<&'a st Some(Expr::Name(ast::ExprName { id: arg_name, .. })) => Some(arg_name.as_str()), Some(Expr::GeneratorExp(ast::ExprGeneratorExp { elt, generators, .. - })) => get_generator_iterable(elt, generators), + })) => generator_iterable(elt, generators), _ => None, } } Expr::ListComp(ast::ExprListComp { elt, generators, .. - }) => get_generator_iterable(elt, generators), + }) => generator_iterable(elt, generators), _ => None, } } -fn get_generator_iterable<'a>( - elt: &'a Expr, - generators: &'a Vec, -) -> Option<&'a str> { +fn generator_iterable<'a>(elt: &'a Expr, generators: &'a Vec) -> Option<&'a str> { // If the `elt` field is anything other than a [`Expr::Name`], we can't be sure that it - // doesn't modify the elements of the underlying iterator - for example, `[i + 1 for i in x][0]`. - if !matches!(elt, Expr::Name(ast::ExprName { .. })) { + // doesn't modify the elements of the underlying iterator (e.g., `[i + 1 for i in x][0]`). + if !elt.is_name_expr() { return None; } - // If there's more than 1 generator, we can't safely say that it fits the diagnostic conditions - - // for example, `[(i, j) for i in x for j in y][0]`. - if generators.len() != 1 { + // If there's more than 1 generator, we can't safely say that it fits the diagnostic conditions + // (e.g., `[(i, j) for i in x for j in y][0]`). + let [generator] = generators.as_slice() else { return None; - } + }; - let generator = &generators[0]; // Ignore if there's an `if` statement in the comprehension, since it filters the list. if !generator.ifs.is_empty() { return None; } - let Some(arg_name) = get_name_id(&generator.iter) else { - return None; - }; - - Some(arg_name) -} - -fn get_name_id(expr: &Expr) -> Option<&str> { - match expr { - Expr::Name(ast::ExprName { id, .. }) => Some(id), - _ => None, - } + let ast::ExprName { id, .. } = generator.iter.as_name_expr()?; + Some(id.as_str()) } -fn get_effective_index(expr: &Expr) -> Option { +fn effective_index(expr: &Expr) -> Option { match expr { Expr::Constant(ast::ExprConstant { value: Constant::Int(value), From a5aed76d248cf67515f3098f9c773797450b145c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 10 Jul 2023 11:49:15 -0400 Subject: [PATCH 17/18] Tweak docs --- ...y_iterable_allocation_for_first_element.rs | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs index cda4ec5f5dcfa..0e398fdecfa8c 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs @@ -8,36 +8,41 @@ use crate::checkers::ast::Checker; use crate::registry::AsRule; /// ## What it does -/// Ensures that instead of creating a new list and indexing into it to find the first element of a -/// collection (e.g., `list(...)[0]`), Python iterators are used. +/// Checks for uses of `list(...)[0]` that can be replaced with +/// `next(iter(...))`. /// /// ## Why is this bad? -/// Creating a new list of great size can involve significant memory/speed concerns. Python's `next(iter(...))` -/// pattern can be used in lieu of creating a new list. This pattern will lazily fetch the first -/// element of the collection, avoiding the memory overhead involved with new list allocation. -/// `next(iter(...))` also is much faster since the list itself doesn't get initialized at once. +/// Calling `list(...)` will create a new list of the entire collection, which +/// can be very expensive for large collections. If you only need the first +/// element of the collection, you can use `next(iter(...))` to lazily fetch +/// the first element without creating a new list. +/// +/// Note that migrating from `list(...)[0]` to `next(iter(...))` can change +/// the behavior of your program in two ways: first, `list(...)` will eagerly +/// evaluate the entire collection, while `next(iter(...))` will only evaluate +/// the first element, so any side effects from evaluating the collection will +/// be delayed. Second, `list(...)[0]` will raise `IndexError` if the +/// collection is empty, while `next(iter(...))` will raise `StopIteration`. /// /// ## Example /// ```python -/// x = range(1000000000000) -/// return list(x)[0] +/// head = list(range(1000000000000))[0] /// ``` /// /// Use instead: /// ```python -/// x = range(1000000000000) -/// return next(iter(x)) +/// head = next(iter(range(1000000000000))) /// ``` /// /// ## References -/// - [Iterators and Iterables in Python: Run Efficient -/// Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) +/// - [Iterators and Iterables in Python: Run Efficient Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) #[violation] pub(crate) struct UnnecessaryIterableAllocationForFirstElement { arg: String, contains_slice: bool, } +/// RUF015 impl UnnecessaryIterableAllocationForFirstElement { pub(crate) fn new(arg: String, contains_slice: bool) -> Self { Self { From 0baa60574884e0d3c383de32dca920caa61abb4e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 10 Jul 2023 12:19:50 -0400 Subject: [PATCH 18/18] Tweak some logic --- .../resources/test/fixtures/ruff/RUF015.py | 6 +- ...y_iterable_allocation_for_first_element.rs | 194 +++++++++--------- ..._rules__ruff__tests__RUF015_RUF015.py.snap | 38 ++-- 3 files changed, 123 insertions(+), 115 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF015.py b/crates/ruff/resources/test/fixtures/ruff/RUF015.py index a8ce58f2dc039..e19602de8dce6 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF015.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF015.py @@ -18,7 +18,7 @@ [i for i in x][:1:1] [i for i in x][:1:2] -# Fine - not indexing (solely) the first element +# OK (not indexing (solely) the first element) list(x) list(x)[1] list(x)[-1] @@ -34,11 +34,11 @@ [i for i in x][::2] [i for i in x][::] -# Fine - doesn't mirror the underlying list +# OK (doesn't mirror the underlying list) [i + 1 for i in x][0] [i for i in x if i > 5][0] [(i, i + 1) for i in x][0] +# OK (multiple generators) y = range(10) -# Fine - multiple generators [i + j for i in x for j in y][0] diff --git a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs index 0e398fdecfa8c..62f2cca6244db 100644 --- a/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs +++ b/crates/ruff/src/rules/ruff/rules/unnecessary_iterable_allocation_for_first_element.rs @@ -1,8 +1,10 @@ -use num_traits::ToPrimitive; +use num_bigint::BigInt; +use num_traits::{One, Zero}; +use rustpython_parser::ast::{self, Comprehension, Constant, Expr}; + use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_semantic::SemanticModel; -use rustpython_parser::ast::{self, Comprehension, Constant, Expr}; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -18,11 +20,13 @@ use crate::registry::AsRule; /// the first element without creating a new list. /// /// Note that migrating from `list(...)[0]` to `next(iter(...))` can change -/// the behavior of your program in two ways: first, `list(...)` will eagerly -/// evaluate the entire collection, while `next(iter(...))` will only evaluate -/// the first element, so any side effects from evaluating the collection will -/// be delayed. Second, `list(...)[0]` will raise `IndexError` if the -/// collection is empty, while `next(iter(...))` will raise `StopIteration`. +/// the behavior of your program in two ways: +/// +/// 1. First, `list(...)` will eagerly evaluate the entire collection, while +/// `next(iter(...))` will only evaluate the first element. As such, any +/// side effects that occur during iteration will be delayed. +/// 2. Second, `list(...)[0]` will raise `IndexError` if the collection is +/// empty, while `next(iter(...))` will raise `StopIteration`. /// /// ## Example /// ```python @@ -38,59 +42,35 @@ use crate::registry::AsRule; /// - [Iterators and Iterables in Python: Run Efficient Iterations](https://realpython.com/python-iterators-iterables/#when-to-use-an-iterator-in-python) #[violation] pub(crate) struct UnnecessaryIterableAllocationForFirstElement { - arg: String, - contains_slice: bool, -} - -/// RUF015 -impl UnnecessaryIterableAllocationForFirstElement { - pub(crate) fn new(arg: String, contains_slice: bool) -> Self { - Self { - arg, - contains_slice, - } - } + iterable: String, + subscript_kind: HeadSubscriptKind, } impl AlwaysAutofixableViolation for UnnecessaryIterableAllocationForFirstElement { #[derive_message_formats] fn message(&self) -> String { - if self.contains_slice { - format!( - "Prefer `[next(iter({}))]` over `list({})[:1]` or equivalent list comprehension/slice", - self.arg, self.arg - ) - } else { - format!( - "Prefer `next(iter({}))` over `list({})[0]` or equivalent list comprehension/slice", - self.arg, self.arg - ) + let UnnecessaryIterableAllocationForFirstElement { + iterable, + subscript_kind, + } = self; + match subscript_kind { + HeadSubscriptKind::Index => { + format!("Prefer `next(iter({iterable}))` over `list({iterable})[0]`") + } + HeadSubscriptKind::Slice => { + format!("Prefer `[next(iter({iterable}))]` over `list({iterable})[:1]`") + } } } fn autofix_title(&self) -> String { - if self.contains_slice { - format!("Replace with `[next(iter({}))]", self.arg) - } else { - format!("Replace with `next(iter({}))`", self.arg) - } - } -} - -/// Contains information about a [`Expr::Subscript`] to determine if and how it accesses the first -/// element of an iterable. -struct ClassifiedSubscript { - /// `true` if the subscript accesses the first element of the iterable. - indexes_first_element: bool, - /// `true` if the subscript is a slice (e.g., `[:1]`). - is_slice: bool, -} - -impl ClassifiedSubscript { - fn new(indexes_first_element: bool, is_slice: bool) -> Self { - Self { - indexes_first_element, - is_slice, + let UnnecessaryIterableAllocationForFirstElement { + iterable, + subscript_kind, + } = self; + match subscript_kind { + HeadSubscriptKind::Index => format!("Replace with `next(iter({iterable}))`"), + HeadSubscriptKind::Slice => format!("Replace with `[next(iter({iterable}))]"), } } } @@ -100,70 +80,90 @@ pub(crate) fn unnecessary_iterable_allocation_for_first_element( checker: &mut Checker, subscript: &Expr, ) { - let Expr::Subscript(ast::ExprSubscript { value, slice, range, .. }) = subscript else { + let Expr::Subscript(ast::ExprSubscript { + value, + slice, + range, + .. + }) = subscript + else { return; }; - let ClassifiedSubscript { - indexes_first_element, - is_slice, - } = classify_subscript(slice); - if !indexes_first_element { + let Some(subscript_kind) = classify_subscript(slice) else { return; - } + }; - let Some(iter_name) = iterable_name(value, checker.semantic()) else { + let Some(iterable) = iterable_name(value, checker.semantic()) else { return; }; let mut diagnostic = Diagnostic::new( - UnnecessaryIterableAllocationForFirstElement::new(iter_name.to_string(), is_slice), + UnnecessaryIterableAllocationForFirstElement { + iterable: iterable.to_string(), + subscript_kind, + }, *range, ); if checker.patch(diagnostic.kind.rule()) { - let replacement = if is_slice { - format!("[next(iter({iter_name}))]") - } else { - format!("next(iter({iter_name}))") + let replacement = match subscript_kind { + HeadSubscriptKind::Index => format!("next(iter({iterable}))"), + HeadSubscriptKind::Slice => format!("[next(iter({iterable}))]"), }; - diagnostic.set_fix(Fix::suggested(Edit::range_replacement(replacement, *range))); } checker.diagnostics.push(diagnostic); } +/// A subscript slice that represents the first element of a list. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum HeadSubscriptKind { + /// The subscript is an index (e.g., `[0]`). + Index, + /// The subscript is a slice (e.g., `[:1]`). + Slice, +} + /// Check that the slice [`Expr`] is functionally equivalent to slicing into the first element. The /// first `bool` checks that the element is in fact first, the second checks if it's a slice or an /// index. -fn classify_subscript(expr: &Expr) -> ClassifiedSubscript { +fn classify_subscript(expr: &Expr) -> Option { match expr { - Expr::Constant(ast::ExprConstant { .. }) => { - let effective_index = effective_index(expr); - ClassifiedSubscript::new(matches!(effective_index, None | Some(0)), false) - } - Expr::Slice(ast::ExprSlice { - step: step_value, - lower: lower_index, - upper: upper_index, + Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), .. + }) if value.is_zero() => Some(HeadSubscriptKind::Index), + Expr::Slice(ast::ExprSlice { + step, lower, upper, .. }) => { - let lower = lower_index.as_ref().and_then(|l| effective_index(l)); - let upper = upper_index.as_ref().and_then(|u| effective_index(u)); - let step = step_value.as_ref().and_then(|s| effective_index(s)); + // Avoid, e.g., `list(...)[:2]` + let upper = upper.as_ref()?; + let upper = as_int(upper)?; + if !upper.is_one() { + return None; + } - if matches!(lower, None | Some(0)) { - if upper.unwrap_or(i64::MAX) > step.unwrap_or(1i64) { - return ClassifiedSubscript::new(false, true); + // Avoid, e.g., `list(...)[2:]`. + if let Some(lower) = lower.as_ref() { + let lower = as_int(lower)?; + if !lower.is_zero() { + return None; } + } - return ClassifiedSubscript::new(true, true); + // Avoid, e.g., `list(...)[::-1]` + if let Some(step) = step.as_ref() { + let step = as_int(step)?; + if step < upper { + return None; + } } - ClassifiedSubscript::new(false, true) + Some(HeadSubscriptKind::Slice) } - _ => ClassifiedSubscript::new(false, false), + _ => None, } } @@ -174,9 +174,11 @@ fn iterable_name<'a>(expr: &'a Expr, model: &SemanticModel) -> Option<&'a str> { Expr::Call(ast::ExprCall { func, args, .. }) => { let ast::ExprName { id, .. } = func.as_name_expr()?; - if !((id == "list" && model.is_builtin("list")) - || (id == "tuple" && model.is_builtin("tuple"))) - { + if !matches!(id.as_str(), "tuple" | "list") { + return None; + } + + if !model.is_builtin(id.as_str()) { return None; } @@ -195,6 +197,8 @@ fn iterable_name<'a>(expr: &'a Expr, model: &SemanticModel) -> Option<&'a str> { } } +/// Given a comprehension, returns the name of the iterable over which it iterates, if it's +/// a simple comprehension (e.g., `x` for `[i for i in x]`). fn generator_iterable<'a>(elt: &'a Expr, generators: &'a Vec) -> Option<&'a str> { // If the `elt` field is anything other than a [`Expr::Name`], we can't be sure that it // doesn't modify the elements of the underlying iterator (e.g., `[i + 1 for i in x][0]`). @@ -217,12 +221,16 @@ fn generator_iterable<'a>(elt: &'a Expr, generators: &'a Vec) -> Some(id.as_str()) } -fn effective_index(expr: &Expr) -> Option { - match expr { - Expr::Constant(ast::ExprConstant { - value: Constant::Int(value), - .. - }) => value.to_i64(), - _ => None, +/// If an expression is a constant integer, returns the value of that integer; otherwise, +/// returns `None`. +fn as_int(expr: &Expr) -> Option<&BigInt> { + if let Expr::Constant(ast::ExprConstant { + value: Constant::Int(value), + .. + }) = expr + { + Some(value) + } else { + None } } diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap index 293611f8b3c15..4eb011f0fe48a 100644 --- a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF015_RUF015.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/ruff/mod.rs --- -RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice +RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` | 3 | # RUF015 4 | list(x)[0] @@ -21,7 +21,7 @@ RUF015.py:4:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 6 6 | list(x)[:1:1] 7 7 | list(x)[:1:2] -RUF015.py:5:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:5:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 3 | # RUF015 4 | list(x)[0] @@ -42,7 +42,7 @@ RUF015.py:5:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equival 7 7 | list(x)[:1:2] 8 8 | tuple(x)[0] -RUF015.py:6:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:6:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 4 | list(x)[0] 5 | list(x)[:1] @@ -63,7 +63,7 @@ RUF015.py:6:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equival 8 8 | tuple(x)[0] 9 9 | tuple(x)[:1] -RUF015.py:7:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:7:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 5 | list(x)[:1] 6 | list(x)[:1:1] @@ -84,7 +84,7 @@ RUF015.py:7:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equival 9 9 | tuple(x)[:1] 10 10 | tuple(x)[:1:1] -RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice +RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` | 6 | list(x)[:1:1] 7 | list(x)[:1:2] @@ -105,7 +105,7 @@ RUF015.py:8:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent 10 10 | tuple(x)[:1:1] 11 11 | tuple(x)[:1:2] -RUF015.py:9:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:9:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 7 | list(x)[:1:2] 8 | tuple(x)[0] @@ -126,7 +126,7 @@ RUF015.py:9:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equival 11 11 | tuple(x)[:1:2] 12 12 | list(i for i in x)[0] -RUF015.py:10:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:10:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 8 | tuple(x)[0] 9 | tuple(x)[:1] @@ -147,7 +147,7 @@ RUF015.py:10:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 12 12 | list(i for i in x)[0] 13 13 | list(i for i in x)[:1] -RUF015.py:11:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:11:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 9 | tuple(x)[:1] 10 | tuple(x)[:1:1] @@ -168,7 +168,7 @@ RUF015.py:11:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 13 13 | list(i for i in x)[:1] 14 14 | list(i for i in x)[:1:1] -RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice +RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` | 10 | tuple(x)[:1:1] 11 | tuple(x)[:1:2] @@ -189,7 +189,7 @@ RUF015.py:12:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 14 14 | list(i for i in x)[:1:1] 15 15 | list(i for i in x)[:1:2] -RUF015.py:13:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:13:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 11 | tuple(x)[:1:2] 12 | list(i for i in x)[0] @@ -210,7 +210,7 @@ RUF015.py:13:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 15 15 | list(i for i in x)[:1:2] 16 16 | [i for i in x][0] -RUF015.py:14:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:14:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 12 | list(i for i in x)[0] 13 | list(i for i in x)[:1] @@ -231,7 +231,7 @@ RUF015.py:14:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 16 16 | [i for i in x][0] 17 17 | [i for i in x][:1] -RUF015.py:15:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:15:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 13 | list(i for i in x)[:1] 14 | list(i for i in x)[:1:1] @@ -252,7 +252,7 @@ RUF015.py:15:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 17 17 | [i for i in x][:1] 18 18 | [i for i in x][:1:1] -RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalent list comprehension/slice +RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` | 14 | list(i for i in x)[:1:1] 15 | list(i for i in x)[:1:2] @@ -273,7 +273,7 @@ RUF015.py:16:1: RUF015 [*] Prefer `next(iter(x))` over `list(x)[0]` or equivalen 18 18 | [i for i in x][:1:1] 19 19 | [i for i in x][:1:2] -RUF015.py:17:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:17:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 15 | list(i for i in x)[:1:2] 16 | [i for i in x][0] @@ -294,7 +294,7 @@ RUF015.py:17:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 19 19 | [i for i in x][:1:2] 20 20 | -RUF015.py:18:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:18:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 16 | [i for i in x][0] 17 | [i for i in x][:1] @@ -312,16 +312,16 @@ RUF015.py:18:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 18 |+[next(iter(x))] 19 19 | [i for i in x][:1:2] 20 20 | -21 21 | # Fine - not indexing (solely) the first element +21 21 | # OK (not indexing (solely) the first element) -RUF015.py:19:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equivalent list comprehension/slice +RUF015.py:19:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` | 17 | [i for i in x][:1] 18 | [i for i in x][:1:1] 19 | [i for i in x][:1:2] | ^^^^^^^^^^^^^^^^^^^^ RUF015 20 | -21 | # Fine - not indexing (solely) the first element +21 | # OK (not indexing (solely) the first element) | = help: Replace with `[next(iter(x))] @@ -332,7 +332,7 @@ RUF015.py:19:1: RUF015 [*] Prefer `[next(iter(x))]` over `list(x)[:1]` or equiva 19 |-[i for i in x][:1:2] 19 |+[next(iter(x))] 20 20 | -21 21 | # Fine - not indexing (solely) the first element +21 21 | # OK (not indexing (solely) the first element) 22 22 | list(x)