diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 419094fcfb..3ab6021c1a 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -1101,22 +1101,15 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { } } } - Expr::UnaryOp(ast::ExprUnaryOp { - op, - operand, - range: _, - }) => { - let check_not_in = checker.enabled(Rule::NotInTest); - let check_not_is = checker.enabled(Rule::NotIsTest); - if check_not_in || check_not_is { - pycodestyle::rules::not_tests( - checker, - expr, - *op, - operand, - check_not_in, - check_not_is, - ); + Expr::UnaryOp( + unary_op @ ast::ExprUnaryOp { + op, + operand, + range: _, + }, + ) => { + if checker.any_enabled(&[Rule::NotInTest, Rule::NotIsTest]) { + pycodestyle::rules::not_tests(checker, unary_op); } if checker.enabled(Rule::UnaryPrefixIncrementDecrement) { flake8_bugbear::rules::unary_prefix_increment_decrement( @@ -1141,18 +1134,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { range: _, }, ) => { - let check_none_comparisons = checker.enabled(Rule::NoneComparison); - let check_true_false_comparisons = checker.enabled(Rule::TrueFalseComparison); - if check_none_comparisons || check_true_false_comparisons { - pycodestyle::rules::literal_comparisons( - checker, - expr, - left, - ops, - comparators, - check_none_comparisons, - check_true_false_comparisons, - ); + if checker.any_enabled(&[Rule::NoneComparison, Rule::TrueFalseComparison]) { + pycodestyle::rules::literal_comparisons(checker, compare); } if checker.enabled(Rule::IsLiteral) { pyflakes::rules::invalid_literal_comparison(checker, left, ops, comparators, expr); diff --git a/crates/ruff/src/rules/pycodestyle/helpers.rs b/crates/ruff/src/rules/pycodestyle/helpers.rs index eee48f336e..76a752b8f9 100644 --- a/crates/ruff/src/rules/pycodestyle/helpers.rs +++ b/crates/ruff/src/rules/pycodestyle/helpers.rs @@ -10,7 +10,7 @@ pub(super) fn is_ambiguous_name(name: &str) -> bool { name == "l" || name == "I" || name == "O" } -pub(super) fn compare( +pub(super) fn generate_comparison( left: &Expr, ops: &[CmpOp], comparators: &[Expr], diff --git a/crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs b/crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs index 1a75983d89..d16fa579f7 100644 --- a/crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -1,15 +1,15 @@ -use itertools::izip; -use ruff_python_ast::{self as ast, CmpOp, Constant, Expr, Ranged}; use rustc_hash::FxHashMap; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers; use ruff_python_ast::helpers::is_const_none; +use ruff_python_ast::{self as ast, CmpOp, Constant, Expr, Ranged}; use crate::checkers::ast::Checker; +use crate::codes::Rule; use crate::registry::AsRule; -use crate::rules::pycodestyle::helpers::compare; +use crate::rules::pycodestyle::helpers::generate_comparison; #[derive(Debug, PartialEq, Eq, Copy, Clone)] enum EqCmpOp { @@ -130,15 +130,7 @@ impl AlwaysAutofixableViolation for TrueFalseComparison { } /// E711, E712 -pub(crate) fn literal_comparisons( - checker: &mut Checker, - expr: &Expr, - left: &Expr, - ops: &[CmpOp], - comparators: &[Expr], - check_none_comparisons: bool, - check_true_false_comparisons: bool, -) { +pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprCompare) { // Mapping from (bad operator index) to (replacement operator). As we iterate // through the list of operators, we apply "dummy" fixes for each error, // then replace the entire expression at the end with one "real" fix, to @@ -146,15 +138,18 @@ pub(crate) fn literal_comparisons( let mut bad_ops: FxHashMap = FxHashMap::default(); let mut diagnostics: Vec = vec![]; - let op = ops.first().unwrap(); - // Check `left`. - let mut comparator = left; - let next = &comparators[0]; + let mut comparator = compare.left.as_ref(); + let [op, ..] = compare.ops.as_slice() else { + return; + }; + let [next, ..] = compare.comparators.as_slice() else { + return; + }; if !helpers::is_constant_non_singleton(next) { if let Some(op) = EqCmpOp::try_from(*op) { - if check_none_comparisons && is_const_none(comparator) { + if checker.enabled(Rule::NoneComparison) && is_const_none(comparator) { match op { EqCmpOp::Eq => { let diagnostic = Diagnostic::new(NoneComparison(op), comparator.range()); @@ -173,7 +168,7 @@ pub(crate) fn literal_comparisons( } } - if check_true_false_comparisons { + if checker.enabled(Rule::TrueFalseComparison) { if let Expr::Constant(ast::ExprConstant { value: Constant::Bool(value), kind: None, @@ -208,59 +203,66 @@ pub(crate) fn literal_comparisons( } // Check each comparator in order. - for (idx, (op, next)) in izip!(ops, comparators).enumerate() { + for (index, (op, next)) in compare + .ops + .iter() + .zip(compare.comparators.iter()) + .enumerate() + { if helpers::is_constant_non_singleton(comparator) { comparator = next; continue; } - if let Some(op) = EqCmpOp::try_from(*op) { - if check_none_comparisons && is_const_none(next) { + let Some(op) = EqCmpOp::try_from(*op) else { + continue; + }; + + if checker.enabled(Rule::NoneComparison) && is_const_none(next) { + match op { + EqCmpOp::Eq => { + let diagnostic = Diagnostic::new(NoneComparison(op), next.range()); + if checker.patch(diagnostic.kind.rule()) { + bad_ops.insert(index, CmpOp::Is); + } + diagnostics.push(diagnostic); + } + EqCmpOp::NotEq => { + let diagnostic = Diagnostic::new(NoneComparison(op), next.range()); + if checker.patch(diagnostic.kind.rule()) { + bad_ops.insert(index, CmpOp::IsNot); + } + diagnostics.push(diagnostic); + } + } + } + + if checker.enabled(Rule::TrueFalseComparison) { + if let Expr::Constant(ast::ExprConstant { + value: Constant::Bool(value), + kind: None, + range: _, + }) = next + { match op { EqCmpOp::Eq => { - let diagnostic = Diagnostic::new(NoneComparison(op), next.range()); + let diagnostic = + Diagnostic::new(TrueFalseComparison(*value, op), next.range()); if checker.patch(diagnostic.kind.rule()) { - bad_ops.insert(idx, CmpOp::Is); + bad_ops.insert(index, CmpOp::Is); } diagnostics.push(diagnostic); } EqCmpOp::NotEq => { - let diagnostic = Diagnostic::new(NoneComparison(op), next.range()); + let diagnostic = + Diagnostic::new(TrueFalseComparison(*value, op), next.range()); if checker.patch(diagnostic.kind.rule()) { - bad_ops.insert(idx, CmpOp::IsNot); + bad_ops.insert(index, CmpOp::IsNot); } diagnostics.push(diagnostic); } } } - - if check_true_false_comparisons { - if let Expr::Constant(ast::ExprConstant { - value: Constant::Bool(value), - kind: None, - range: _, - }) = next - { - match op { - EqCmpOp::Eq => { - let diagnostic = - Diagnostic::new(TrueFalseComparison(*value, op), next.range()); - if checker.patch(diagnostic.kind.rule()) { - bad_ops.insert(idx, CmpOp::Is); - } - diagnostics.push(diagnostic); - } - EqCmpOp::NotEq => { - let diagnostic = - Diagnostic::new(TrueFalseComparison(*value, op), next.range()); - if checker.patch(diagnostic.kind.rule()) { - bad_ops.insert(idx, CmpOp::IsNot); - } - diagnostics.push(diagnostic); - } - } - } - } } comparator = next; @@ -270,17 +272,19 @@ pub(crate) fn literal_comparisons( // `noqa`, but another doesn't, both will be removed here. if !bad_ops.is_empty() { // Replace the entire comparison expression. - let ops = ops + let ops = compare + .ops .iter() .enumerate() .map(|(idx, op)| bad_ops.get(&idx).unwrap_or(op)) .copied() .collect::>(); - let content = compare(left, &ops, comparators, checker.locator()); + let content = + generate_comparison(&compare.left, &ops, &compare.comparators, checker.locator()); for diagnostic in &mut diagnostics { diagnostic.set_fix(Fix::suggested(Edit::range_replacement( content.to_string(), - expr.range(), + compare.range(), ))); } } diff --git a/crates/ruff/src/rules/pycodestyle/rules/not_tests.rs b/crates/ruff/src/rules/pycodestyle/rules/not_tests.rs index e77c1ebfa1..0ee126d046 100644 --- a/crates/ruff/src/rules/pycodestyle/rules/not_tests.rs +++ b/crates/ruff/src/rules/pycodestyle/rules/not_tests.rs @@ -1,11 +1,10 @@ -use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged, UnaryOp}; - use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged}; use crate::checkers::ast::Checker; -use crate::registry::AsRule; -use crate::rules::pycodestyle::helpers::compare; +use crate::registry::{AsRule, Rule}; +use crate::rules::pycodestyle::helpers::generate_comparison; /// ## What it does /// Checks for negative comparison using `not {foo} in {bar}`. @@ -74,54 +73,46 @@ impl AlwaysAutofixableViolation for NotIsTest { } /// E713, E714 -pub(crate) fn not_tests( - checker: &mut Checker, - expr: &Expr, - op: UnaryOp, - operand: &Expr, - check_not_in: bool, - check_not_is: bool, -) { - if matches!(op, UnaryOp::Not) { - if let Expr::Compare(ast::ExprCompare { - left, - ops, - comparators, - range: _, - }) = operand - { - if !matches!(&ops[..], [CmpOp::In | CmpOp::Is]) { - return; - } - for op in ops { - match op { - CmpOp::In => { - if check_not_in { - let mut diagnostic = Diagnostic::new(NotInTest, operand.range()); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(Fix::automatic(Edit::range_replacement( - compare(left, &[CmpOp::NotIn], comparators, checker.locator()), - expr.range(), - ))); - } - checker.diagnostics.push(diagnostic); - } - } - CmpOp::Is => { - if check_not_is { - let mut diagnostic = Diagnostic::new(NotIsTest, operand.range()); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(Fix::automatic(Edit::range_replacement( - compare(left, &[CmpOp::IsNot], comparators, checker.locator()), - expr.range(), - ))); - } - checker.diagnostics.push(diagnostic); - } - } - _ => {} +pub(crate) fn not_tests(checker: &mut Checker, unary_op: &ast::ExprUnaryOp) { + if !unary_op.op.is_not() { + return; + } + + let Expr::Compare(ast::ExprCompare { + left, + ops, + comparators, + range: _, + }) = unary_op.operand.as_ref() + else { + return; + }; + + match ops.as_slice() { + [CmpOp::In] => { + if checker.enabled(Rule::NotInTest) { + let mut diagnostic = Diagnostic::new(NotInTest, unary_op.operand.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + generate_comparison(left, &[CmpOp::NotIn], comparators, checker.locator()), + unary_op.range(), + ))); } + checker.diagnostics.push(diagnostic); } } + [CmpOp::Is] => { + if checker.enabled(Rule::NotIsTest) { + let mut diagnostic = Diagnostic::new(NotIsTest, unary_op.operand.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + generate_comparison(left, &[CmpOp::IsNot], comparators, checker.locator()), + unary_op.range(), + ))); + } + checker.diagnostics.push(diagnostic); + } + } + _ => {} } }