diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs index 6bcdb7864e..ad910495a7 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_any_all.rs @@ -73,7 +73,7 @@ pub(crate) fn unnecessary_comprehension_any_all( let (Expr::ListComp(ast::ExprListComp { elt, .. } )| Expr::SetComp(ast::ExprSetComp { elt, .. })) = &args[0] else { return; }; - if is_async_generator(elt) { + if contains_await(elt) { return; } if !checker.semantic().is_builtin(id) { @@ -89,7 +89,7 @@ pub(crate) fn unnecessary_comprehension_any_all( } } -/// Return `true` if the `Expr` contains an `await` expression. -fn is_async_generator(expr: &Expr) -> bool { - any_over_expr(expr, &|expr| matches!(expr, Expr::Await(_))) +/// Return `true` if the [`Expr`] contains an `await` expression. +fn contains_await(expr: &Expr) -> bool { + any_over_expr(expr, &Expr::is_await_expr) } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs b/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs index c306b589a8..fd70c36afb 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs @@ -56,6 +56,156 @@ impl Violation for ReimplementedBuiltin { } } +/// SIM110, SIM111 +pub(crate) fn convert_for_loop_to_any_all( + checker: &mut Checker, + stmt: &Stmt, + sibling: Option<&Stmt>, +) { + // There are two cases to consider: + // - `for` loop with an `else: return True` or `else: return False`. + // - `for` loop followed by `return True` or `return False` + if let Some(loop_info) = return_values_for_else(stmt) + .or_else(|| sibling.and_then(|sibling| return_values_for_siblings(stmt, sibling))) + { + // Check if loop_info.target, loop_info.iter, or loop_info.test contains `await`. + if contains_await(loop_info.target) + || contains_await(loop_info.iter) + || contains_await(loop_info.test) + { + return; + } + if loop_info.return_value && !loop_info.next_return_value { + if checker.enabled(Rule::ReimplementedBuiltin) { + let contents = return_stmt( + "any", + loop_info.test, + loop_info.target, + loop_info.iter, + checker.generator(), + ); + + // Don't flag if the resulting expression would exceed the maximum line length. + let line_start = checker.locator.line_start(stmt.start()); + if LineWidth::new(checker.settings.tab_size) + .add_str(&checker.locator.contents()[TextRange::new(line_start, stmt.start())]) + .add_str(&contents) + > checker.settings.line_length + { + return; + } + + let mut diagnostic = Diagnostic::new( + ReimplementedBuiltin { + repl: contents.clone(), + }, + TextRange::new(stmt.start(), loop_info.terminal), + ); + if checker.patch(diagnostic.kind.rule()) && checker.semantic().is_builtin("any") { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::replacement( + contents, + stmt.start(), + loop_info.terminal, + ))); + } + checker.diagnostics.push(diagnostic); + } + } + + if !loop_info.return_value && loop_info.next_return_value { + if checker.enabled(Rule::ReimplementedBuiltin) { + // Invert the condition. + let test = { + if let Expr::UnaryOp(ast::ExprUnaryOp { + op: Unaryop::Not, + operand, + range: _, + }) = &loop_info.test + { + *operand.clone() + } else if let Expr::Compare(ast::ExprCompare { + left, + ops, + comparators, + range: _, + }) = &loop_info.test + { + if let ([op], [comparator]) = (ops.as_slice(), comparators.as_slice()) { + let op = match op { + Cmpop::Eq => Cmpop::NotEq, + Cmpop::NotEq => Cmpop::Eq, + Cmpop::Lt => Cmpop::GtE, + Cmpop::LtE => Cmpop::Gt, + Cmpop::Gt => Cmpop::LtE, + Cmpop::GtE => Cmpop::Lt, + Cmpop::Is => Cmpop::IsNot, + Cmpop::IsNot => Cmpop::Is, + Cmpop::In => Cmpop::NotIn, + Cmpop::NotIn => Cmpop::In, + }; + let node = ast::ExprCompare { + left: left.clone(), + ops: vec![op], + comparators: vec![comparator.clone()], + range: TextRange::default(), + }; + node.into() + } else { + let node = ast::ExprUnaryOp { + op: Unaryop::Not, + operand: Box::new(loop_info.test.clone()), + range: TextRange::default(), + }; + node.into() + } + } else { + let node = ast::ExprUnaryOp { + op: Unaryop::Not, + operand: Box::new(loop_info.test.clone()), + range: TextRange::default(), + }; + node.into() + } + }; + let contents = return_stmt( + "all", + &test, + loop_info.target, + loop_info.iter, + checker.generator(), + ); + + // Don't flag if the resulting expression would exceed the maximum line length. + let line_start = checker.locator.line_start(stmt.start()); + if LineWidth::new(checker.settings.tab_size) + .add_str(&checker.locator.contents()[TextRange::new(line_start, stmt.start())]) + .add_str(&contents) + > checker.settings.line_length + { + return; + } + + let mut diagnostic = Diagnostic::new( + ReimplementedBuiltin { + repl: contents.clone(), + }, + TextRange::new(stmt.start(), loop_info.terminal), + ); + if checker.patch(diagnostic.kind.rule()) && checker.semantic().is_builtin("all") { + #[allow(deprecated)] + diagnostic.set_fix(Fix::unspecified(Edit::replacement( + contents, + stmt.start(), + loop_info.terminal, + ))); + } + checker.diagnostics.push(diagnostic); + } + } + } +} + struct Loop<'a> { return_value: bool, next_return_value: bool, @@ -225,157 +375,7 @@ fn return_stmt(id: &str, test: &Expr, target: &Expr, iter: &Expr, generator: Gen generator.stmt(&node3.into()) } -/// Return `true` if the `Expr` contains an `await` expression. +/// Return `true` if the [`Expr`] contains an `await` expression. fn contains_await(expr: &Expr) -> bool { - any_over_expr(expr, &|expr| matches!(expr, Expr::Await(_))) -} - -/// SIM110, SIM111 -pub(crate) fn convert_for_loop_to_any_all( - checker: &mut Checker, - stmt: &Stmt, - sibling: Option<&Stmt>, -) { - // There are two cases to consider: - // - `for` loop with an `else: return True` or `else: return False`. - // - `for` loop followed by `return True` or `return False` - if let Some(loop_info) = return_values_for_else(stmt) - .or_else(|| sibling.and_then(|sibling| return_values_for_siblings(stmt, sibling))) - { - // Check if loop_info.target, loop_info.iter, or loop_info.test contains `await`. - if contains_await(loop_info.target) - || contains_await(loop_info.iter) - || contains_await(loop_info.test) - { - return; - } - if loop_info.return_value && !loop_info.next_return_value { - if checker.enabled(Rule::ReimplementedBuiltin) { - let contents = return_stmt( - "any", - loop_info.test, - loop_info.target, - loop_info.iter, - checker.generator(), - ); - - // Don't flag if the resulting expression would exceed the maximum line length. - let line_start = checker.locator.line_start(stmt.start()); - if LineWidth::new(checker.settings.tab_size) - .add_str(&checker.locator.contents()[TextRange::new(line_start, stmt.start())]) - .add_str(&contents) - > checker.settings.line_length - { - return; - } - - let mut diagnostic = Diagnostic::new( - ReimplementedBuiltin { - repl: contents.clone(), - }, - TextRange::new(stmt.start(), loop_info.terminal), - ); - if checker.patch(diagnostic.kind.rule()) && checker.semantic().is_builtin("any") { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::replacement( - contents, - stmt.start(), - loop_info.terminal, - ))); - } - checker.diagnostics.push(diagnostic); - } - } - - if !loop_info.return_value && loop_info.next_return_value { - if checker.enabled(Rule::ReimplementedBuiltin) { - // Invert the condition. - let test = { - if let Expr::UnaryOp(ast::ExprUnaryOp { - op: Unaryop::Not, - operand, - range: _, - }) = &loop_info.test - { - *operand.clone() - } else if let Expr::Compare(ast::ExprCompare { - left, - ops, - comparators, - range: _, - }) = &loop_info.test - { - if let ([op], [comparator]) = (ops.as_slice(), comparators.as_slice()) { - let op = match op { - Cmpop::Eq => Cmpop::NotEq, - Cmpop::NotEq => Cmpop::Eq, - Cmpop::Lt => Cmpop::GtE, - Cmpop::LtE => Cmpop::Gt, - Cmpop::Gt => Cmpop::LtE, - Cmpop::GtE => Cmpop::Lt, - Cmpop::Is => Cmpop::IsNot, - Cmpop::IsNot => Cmpop::Is, - Cmpop::In => Cmpop::NotIn, - Cmpop::NotIn => Cmpop::In, - }; - let node = ast::ExprCompare { - left: left.clone(), - ops: vec![op], - comparators: vec![comparator.clone()], - range: TextRange::default(), - }; - node.into() - } else { - let node = ast::ExprUnaryOp { - op: Unaryop::Not, - operand: Box::new(loop_info.test.clone()), - range: TextRange::default(), - }; - node.into() - } - } else { - let node = ast::ExprUnaryOp { - op: Unaryop::Not, - operand: Box::new(loop_info.test.clone()), - range: TextRange::default(), - }; - node.into() - } - }; - let contents = return_stmt( - "all", - &test, - loop_info.target, - loop_info.iter, - checker.generator(), - ); - - // Don't flag if the resulting expression would exceed the maximum line length. - let line_start = checker.locator.line_start(stmt.start()); - if LineWidth::new(checker.settings.tab_size) - .add_str(&checker.locator.contents()[TextRange::new(line_start, stmt.start())]) - .add_str(&contents) - > checker.settings.line_length - { - return; - } - - let mut diagnostic = Diagnostic::new( - ReimplementedBuiltin { - repl: contents.clone(), - }, - TextRange::new(stmt.start(), loop_info.terminal), - ); - if checker.patch(diagnostic.kind.rule()) && checker.semantic().is_builtin("all") { - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::replacement( - contents, - stmt.start(), - loop_info.terminal, - ))); - } - checker.diagnostics.push(diagnostic); - } - } - } + any_over_expr(expr, &Expr::is_await_expr) } diff --git a/crates/ruff/src/rules/pyupgrade/rules/unpacked_list_comprehension.rs b/crates/ruff/src/rules/pyupgrade/rules/unpacked_list_comprehension.rs index 3ff0a14ffb..9098faf7fb 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unpacked_list_comprehension.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/unpacked_list_comprehension.rs @@ -2,6 +2,7 @@ use rustpython_parser::ast::{self, Expr, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::any_over_expr; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -20,126 +21,45 @@ impl AlwaysAutofixableViolation for UnpackedListComprehension { } } -/// Returns `true` if `expr` contains an `Expr::Await`. -fn contains_await(expr: &Expr) -> bool { - match expr { - Expr::Await(_) => true, - Expr::BoolOp(ast::ExprBoolOp { values, .. }) => values.iter().any(contains_await), - Expr::NamedExpr(ast::ExprNamedExpr { - target, - value, - range: _, - }) => contains_await(target) || contains_await(value), - Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { - contains_await(left) || contains_await(right) - } - Expr::UnaryOp(ast::ExprUnaryOp { operand, .. }) => contains_await(operand), - Expr::Lambda(ast::ExprLambda { body, .. }) => contains_await(body), - Expr::IfExp(ast::ExprIfExp { - test, - body, - orelse, - range: _, - }) => contains_await(test) || contains_await(body) || contains_await(orelse), - Expr::Dict(ast::ExprDict { - keys, - values, - range: _, - }) => keys - .iter() - .flatten() - .chain(values.iter()) - .any(contains_await), - Expr::Set(ast::ExprSet { elts, range: _ }) => elts.iter().any(contains_await), - Expr::ListComp(ast::ExprListComp { elt, .. }) => contains_await(elt), - Expr::SetComp(ast::ExprSetComp { elt, .. }) => contains_await(elt), - Expr::DictComp(ast::ExprDictComp { key, value, .. }) => { - contains_await(key) || contains_await(value) - } - Expr::GeneratorExp(ast::ExprGeneratorExp { elt, .. }) => contains_await(elt), - Expr::Yield(ast::ExprYield { value, range: _ }) => { - value.as_ref().map_or(false, |value| contains_await(value)) - } - Expr::YieldFrom(ast::ExprYieldFrom { value, range: _ }) => contains_await(value), - Expr::Compare(ast::ExprCompare { - left, comparators, .. - }) => contains_await(left) || comparators.iter().any(contains_await), - Expr::Call(ast::ExprCall { - func, - args, - keywords, - range: _, - }) => { - contains_await(func) - || args.iter().any(contains_await) - || keywords - .iter() - .any(|keyword| contains_await(&keyword.value)) - } - Expr::FormattedValue(ast::ExprFormattedValue { - value, format_spec, .. - }) => { - contains_await(value) - || format_spec - .as_ref() - .map_or(false, |value| contains_await(value)) - } - Expr::JoinedStr(ast::ExprJoinedStr { values, range: _ }) => { - values.iter().any(contains_await) - } - Expr::Constant(_) => false, - Expr::Attribute(ast::ExprAttribute { value, .. }) => contains_await(value), - Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { - contains_await(value) || contains_await(slice) - } - Expr::Starred(ast::ExprStarred { value, .. }) => contains_await(value), - Expr::Name(_) => false, - Expr::List(ast::ExprList { elts, .. }) => elts.iter().any(contains_await), - Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().any(contains_await), - Expr::Slice(ast::ExprSlice { - lower, - upper, - step, - range: _, - }) => { - lower.as_ref().map_or(false, |value| contains_await(value)) - || upper.as_ref().map_or(false, |value| contains_await(value)) - || step.as_ref().map_or(false, |value| contains_await(value)) - } - } -} - /// UP027 pub(crate) fn unpacked_list_comprehension(checker: &mut Checker, targets: &[Expr], value: &Expr) { let Some(target) = targets.get(0) else { return; }; - if let Expr::Tuple(_) = target { - if let Expr::ListComp(ast::ExprListComp { - elt, - generators, - range: _, - }) = value - { - if generators.iter().any(|generator| generator.is_async) || contains_await(elt) { - return; - } - let mut diagnostic = Diagnostic::new(UnpackedListComprehension, value.range()); - if checker.patch(diagnostic.kind.rule()) { - let existing = checker.locator.slice(value.range()); - - let mut content = String::with_capacity(existing.len()); - content.push('('); - content.push_str(&existing[1..existing.len() - 1]); - content.push(')'); - #[allow(deprecated)] - diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( - content, - value.range(), - ))); - } - checker.diagnostics.push(diagnostic); - } + if !target.is_tuple_expr() { + return; } + + let Expr::ListComp(ast::ExprListComp { + elt, + generators, + range: _, + }) = value else { + return; + }; + + if generators.iter().any(|generator| generator.is_async) || contains_await(elt) { + return; + } + + let mut diagnostic = Diagnostic::new(UnpackedListComprehension, value.range()); + if checker.patch(diagnostic.kind.rule()) { + let existing = checker.locator.slice(value.range()); + + let mut content = String::with_capacity(existing.len()); + content.push('('); + content.push_str(&existing[1..existing.len() - 1]); + content.push(')'); + diagnostic.set_fix(Fix::suggested(Edit::range_replacement( + content, + value.range(), + ))); + } + checker.diagnostics.push(diagnostic); +} + +/// Return `true` if the [`Expr`] contains an `await` expression. +fn contains_await(expr: &Expr) -> bool { + any_over_expr(expr, &Expr::is_await_expr) }