diff --git a/crates/ruff/resources/test/fixtures/flake8_pie/PIE802.py b/crates/ruff/resources/test/fixtures/flake8_pie/PIE802.py new file mode 100644 index 0000000000000..52ed90aa21a80 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pie/PIE802.py @@ -0,0 +1,10 @@ +# no error +all((x.id for x in bar)) +all(x.id for x in bar) +all(x.id for x in bar) +any(x.id for x in bar) +any({x.id for x in bar}) + +# PIE 802 +any([x.id for x in bar]) +all([x.id for x in bar]) diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 19cc9a551d495..c73826ef4a9ad 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -2491,6 +2491,13 @@ where if self.settings.rules.enabled(&Rule::UnnecessaryDictKwargs) { flake8_pie::rules::no_unnecessary_dict_kwargs(self, expr, keywords); } + if self + .settings + .rules + .enabled(&Rule::UnnecessaryComprehensionAnyAll) + { + flake8_pie::rules::unnecessary_comprehension_any_all(self, expr, func, args); + } // flake8-bandit if self.settings.rules.enabled(&Rule::ExecBuiltin) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index be7fb612b5a8d..39a01475cb6d7 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -516,6 +516,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Flake8Pie, "794") => Rule::DupeClassFieldDefinitions, (Flake8Pie, "796") => Rule::PreferUniqueEnums, (Flake8Pie, "800") => Rule::UnnecessarySpread, + (Flake8Pie, "802") => Rule::UnnecessaryComprehensionAnyAll, (Flake8Pie, "804") => Rule::UnnecessaryDictKwargs, (Flake8Pie, "807") => Rule::PreferListBuiltin, (Flake8Pie, "810") => Rule::SingleStartsEndsWith, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 4ddd0953e111c..9544de5517cb3 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -491,6 +491,7 @@ ruff_macros::register_rules!( rules::flake8_pie::rules::UnnecessaryDictKwargs, rules::flake8_pie::rules::PreferListBuiltin, rules::flake8_pie::rules::SingleStartsEndsWith, + rules::flake8_pie::rules::UnnecessaryComprehensionAnyAll, // flake8-commas rules::flake8_commas::rules::TrailingCommaMissing, rules::flake8_commas::rules::TrailingCommaOnBareTupleProhibited, diff --git a/crates/ruff/src/rules/flake8_pie/fixes.rs b/crates/ruff/src/rules/flake8_pie/fixes.rs new file mode 100644 index 0000000000000..c4fbe901a8a04 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pie/fixes.rs @@ -0,0 +1,45 @@ +use anyhow::{bail, Result}; +use libcst_native::{Codegen, CodegenState, Expression, GeneratorExp}; + +use crate::ast::types::Range; +use crate::cst::matchers::{match_expr, match_module}; +use crate::fix::Fix; +use crate::source_code::{Locator, Stylist}; + +/// (PIE802) Convert `[i for i in a]` into `i for i in a` +pub fn fix_unnecessary_comprehension_any_all( + locator: &Locator, + stylist: &Stylist, + expr: &rustpython_parser::ast::Expr, +) -> Result { + // Expr(ListComp) -> Expr(GeneratorExp) + let module_text = locator.slice(&Range::from_located(expr)); + let mut tree = match_module(module_text)?; + let mut body = match_expr(&mut tree)?; + + let Expression::ListComp(list_comp) = &body.value else { + bail!( + "Expected Expression::ListComp" + ); + }; + + body.value = Expression::GeneratorExp(Box::new(GeneratorExp { + elt: list_comp.elt.clone(), + for_in: list_comp.for_in.clone(), + lpar: list_comp.lpar.clone(), + rpar: list_comp.rpar.clone(), + })); + + let mut state = CodegenState { + default_newline: stylist.line_ending(), + default_indent: stylist.indentation(), + ..CodegenState::default() + }; + tree.codegen(&mut state); + + Ok(Fix::replacement( + state.to_string(), + expr.location, + expr.end_location.unwrap(), + )) +} diff --git a/crates/ruff/src/rules/flake8_pie/mod.rs b/crates/ruff/src/rules/flake8_pie/mod.rs index 21040fd1f0f88..654e6f83c95a0 100644 --- a/crates/ruff/src/rules/flake8_pie/mod.rs +++ b/crates/ruff/src/rules/flake8_pie/mod.rs @@ -1,4 +1,5 @@ //! Rules from [flake8-pie](https://pypi.org/project/flake8-pie/). +mod fixes; pub(crate) mod rules; #[cfg(test)] @@ -20,6 +21,7 @@ mod tests { #[test_case(Rule::UnnecessarySpread, Path::new("PIE800.py"); "PIE800")] #[test_case(Rule::PreferListBuiltin, Path::new("PIE807.py"); "PIE807")] #[test_case(Rule::PreferUniqueEnums, Path::new("PIE796.py"); "PIE796")] + #[test_case(Rule::UnnecessaryComprehensionAnyAll, Path::new("PIE802.py"); "PIE802")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_pie/rules.rs b/crates/ruff/src/rules/flake8_pie/rules.rs index bdaaeea24f912..d68c7adb20dde 100644 --- a/crates/ruff/src/rules/flake8_pie/rules.rs +++ b/crates/ruff/src/rules/flake8_pie/rules.rs @@ -15,6 +15,8 @@ use crate::message::Location; use crate::registry::Diagnostic; use crate::violation::{AlwaysAutofixableViolation, Violation}; +use super::fixes; + define_violation!( pub struct UnnecessaryPass; ); @@ -58,6 +60,50 @@ impl Violation for PreferUniqueEnums { } } +define_violation!( + /// ## What it does + /// Checks for unnecessary list comprehensions passed to `any` and `all`. + /// + /// ## Why is this bad? + /// `any` and `all` take any iterators, including generators. Converting a generator to a list + /// by way of a list comprehension is unnecessary and reduces performance due to the + /// overhead of creating the list. + /// + /// For example, compare the performance of `all` with a list comprehension against that + /// of a generator (~40x faster here): + /// + /// ```python + /// In [1]: %timeit all([i for i in range(1000)]) + /// 8.14 µs ± 25.4 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) + /// + /// In [2]: %timeit all(i for i in range(1000)) + /// 212 ns ± 0.892 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) + /// ``` + /// + /// ## Examples + /// ```python + /// any([x.id for x in bar]) + /// all([x.id for x in bar]) + /// ``` + /// + /// Use instead: + /// ```python + /// any(x.id for x in bar) + /// all(x.id for x in bar) + /// ``` + pub struct UnnecessaryComprehensionAnyAll; +); +impl AlwaysAutofixableViolation for UnnecessaryComprehensionAnyAll { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unnecessary list comprehension.") + } + + fn autofix_title(&self) -> String { + "Remove unnecessary list comprehension".to_string() + } +} + define_violation!( pub struct UnnecessarySpread; ); @@ -282,6 +328,39 @@ pub fn no_unnecessary_spread(checker: &mut Checker, keys: &[Option], value } } +/// PIE802 +pub fn unnecessary_comprehension_any_all( + checker: &mut Checker, + expr: &Expr, + func: &Expr, + args: &[Expr], +) { + if let ExprKind::Name { id, .. } = &func.node { + if (id == "all" || id == "any") && args.len() == 1 { + if !checker.is_builtin(id) { + return; + } + if let ExprKind::ListComp { .. } = args[0].node { + let mut diagnostic = + Diagnostic::new(UnnecessaryComprehensionAnyAll, Range::from_located(expr)); + if checker.patch(diagnostic.kind.rule()) { + match fixes::fix_unnecessary_comprehension_any_all( + checker.locator, + checker.stylist, + &args[0], + ) { + Ok(fix) => { + diagnostic.amend(fix); + } + Err(e) => error!("Failed to generate fix: {e}"), + } + } + checker.diagnostics.push(diagnostic); + } + } + } +} + /// Return `true` if a key is a valid keyword argument name. fn is_valid_kwarg_name(key: &Expr) -> bool { if let ExprKind::Constant { diff --git a/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE802_PIE802.py.snap b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE802_PIE802.py.snap new file mode 100644 index 0000000000000..af293532d639a --- /dev/null +++ b/crates/ruff/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE802_PIE802.py.snap @@ -0,0 +1,39 @@ +--- +source: crates/ruff/src/rules/flake8_pie/mod.rs +expression: diagnostics +--- +- kind: + UnnecessaryComprehensionAnyAll: ~ + location: + row: 9 + column: 0 + end_location: + row: 9 + column: 24 + fix: + content: x.id for x in bar + location: + row: 9 + column: 4 + end_location: + row: 9 + column: 23 + parent: ~ +- kind: + UnnecessaryComprehensionAnyAll: ~ + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 24 + fix: + content: x.id for x in bar + location: + row: 10 + column: 4 + end_location: + row: 10 + column: 23 + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index ece4b58d811f9..4da9a1ac59812 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1762,6 +1762,7 @@ "PIE8", "PIE80", "PIE800", + "PIE802", "PIE804", "PIE807", "PIE81",