diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB171.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB171.py new file mode 100644 index 00000000000000..4596ffe4edaf0e --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB171.py @@ -0,0 +1,45 @@ +# Errors. + +if 1 in (1,): + print("Single-element tuple") + +if 1 in [1]: + print("Single-element list") + +if 1 in {1}: + print("Single-element set") + +if "a" in "a": + print("Single-element string") + +if 1 not in (1,): + print("Check `not in` membership test") + +if not 1 in (1,): + print("Check the negated membership test") + +# Non-errors. + +if 1 in (1, 2): + pass + +if 1 in [1, 2]: + pass + +if 1 in {1, 2}: + pass + +if "a" in "ab": + pass + +if 1 == (1,): + pass + +if 1 > [1]: + pass + +if 1 is {1}: + pass + +if "a" == "a": + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index cc747c0ef6f83a..948bbb6f648af9 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1218,6 +1218,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { comparators, ); } + if checker.enabled(Rule::SingleItemMembershipTest) { + refurb::rules::single_item_membership_test(checker, expr, left, ops, comparators); + } } Expr::Constant(ast::ExprConstant { value: Constant::Int(_) | Constant::Float(_) | Constant::Complex { .. }, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f985b9a2304334..968a46f221a57f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -923,6 +923,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap), (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy), (Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate), + (Refurb, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest), (Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd), // flake8-logging diff --git a/crates/ruff_linter/src/rules/pycodestyle/helpers.rs b/crates/ruff_linter/src/rules/pycodestyle/helpers.rs index ad36460faf0d6a..0c7b133787f99f 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/helpers.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/helpers.rs @@ -1,62 +1,3 @@ -use ruff_python_ast::node::AnyNodeRef; -use ruff_python_ast::parenthesize::parenthesized_range; -use ruff_python_ast::{CmpOp, Expr}; -use ruff_python_trivia::CommentRanges; -use ruff_source_file::Locator; -use ruff_text_size::Ranged; - pub(super) fn is_ambiguous_name(name: &str) -> bool { name == "l" || name == "I" || name == "O" } - -pub(super) fn generate_comparison( - left: &Expr, - ops: &[CmpOp], - comparators: &[Expr], - parent: AnyNodeRef, - comment_ranges: &CommentRanges, - locator: &Locator, -) -> String { - let start = left.start(); - let end = comparators.last().map_or_else(|| left.end(), Ranged::end); - let mut contents = String::with_capacity(usize::from(end - start)); - - // Add the left side of the comparison. - contents.push_str( - locator.slice( - parenthesized_range(left.into(), parent, comment_ranges, locator.contents()) - .unwrap_or(left.range()), - ), - ); - - for (op, comparator) in ops.iter().zip(comparators) { - // Add the operator. - contents.push_str(match op { - CmpOp::Eq => " == ", - CmpOp::NotEq => " != ", - CmpOp::Lt => " < ", - CmpOp::LtE => " <= ", - CmpOp::Gt => " > ", - CmpOp::GtE => " >= ", - CmpOp::In => " in ", - CmpOp::NotIn => " not in ", - CmpOp::Is => " is ", - CmpOp::IsNot => " is not ", - }); - - // Add the right side of the comparison. - contents.push_str( - locator.slice( - parenthesized_range( - comparator.into(), - parent, - comment_ranges, - locator.contents(), - ) - .unwrap_or(comparator.range()), - ), - ); - } - - contents -} diff --git a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs index 9f8db019974d75..eb63de1f01bbe3 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/literal_comparisons.rs @@ -3,14 +3,13 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{AlwaysFixableViolation, 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::helpers::{generate_comparison, is_const_none}; use ruff_python_ast::{self as ast, CmpOp, Constant, Expr}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::registry::AsRule; -use crate::rules::pycodestyle::helpers::generate_comparison; #[derive(Debug, PartialEq, Eq, Copy, Clone)] enum EqCmpOp { diff --git a/crates/ruff_linter/src/rules/pycodestyle/rules/not_tests.rs b/crates/ruff_linter/src/rules/pycodestyle/rules/not_tests.rs index 5e4d4b600cf93d..89080509906ffa 100644 --- a/crates/ruff_linter/src/rules/pycodestyle/rules/not_tests.rs +++ b/crates/ruff_linter/src/rules/pycodestyle/rules/not_tests.rs @@ -1,12 +1,12 @@ use crate::fix::edits::pad; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::generate_comparison; use ruff_python_ast::{self as ast, CmpOp, Expr}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::registry::{AsRule, Rule}; -use crate::rules::pycodestyle::helpers::generate_comparison; /// ## What it does /// Checks for negative comparison using `not {foo} in {bar}`. diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 5a629d6fbac810..199b4bc244ed3a 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -22,6 +22,7 @@ mod tests { #[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))] #[test_case(Rule::PrintEmptyString, Path::new("FURB105.py"))] #[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))] + #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))] 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_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 9541fa9ddd5b6b..2fcb5830628b01 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -4,6 +4,7 @@ pub(crate) use implicit_cwd::*; pub(crate) use print_empty_string::*; pub(crate) use reimplemented_starmap::*; pub(crate) use repeated_append::*; +pub(crate) use single_item_membership_test::*; pub(crate) use slice_copy::*; pub(crate) use unnecessary_enumerate::*; @@ -13,5 +14,6 @@ mod implicit_cwd; mod print_empty_string; mod reimplemented_starmap; mod repeated_append; +mod single_item_membership_test; mod slice_copy; mod unnecessary_enumerate; diff --git a/crates/ruff_linter/src/rules/refurb/rules/single_item_membership_test.rs b/crates/ruff_linter/src/rules/refurb/rules/single_item_membership_test.rs new file mode 100644 index 00000000000000..10ba36daa6a3d9 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/single_item_membership_test.rs @@ -0,0 +1,132 @@ +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::generate_comparison; +use ruff_python_ast::ExprConstant; +use ruff_python_ast::{CmpOp, Constant, Expr}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::fix::edits::pad; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for membership tests against single-item containers. +/// +/// ## Why is this bad? +/// Performing a membership test against a container (like a `list` or `set`) +/// with a single item is less readable and less efficient than comparing +/// against the item directly. +/// +/// ## Example +/// ```python +/// 1 in [1] +/// ``` +/// +/// Use instead: +/// ```python +/// 1 == 1 +/// ``` +/// +/// ## References +/// - [Python documentation: Comparisons](https://docs.python.org/3/reference/expressions.html#comparisons) +/// - [Python documentation: Membership test operations](https://docs.python.org/3/reference/expressions.html#membership-test-operations) +#[violation] +pub struct SingleItemMembershipTest { + membership_test: MembershipTest, +} + +impl Violation for SingleItemMembershipTest { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Membership test against single-item container") + } + + fn fix_title(&self) -> Option { + let SingleItemMembershipTest { membership_test } = self; + match membership_test { + MembershipTest::In => Some("Convert to equality test".to_string()), + MembershipTest::NotIn => Some("Convert to inequality test".to_string()), + } + } +} + +/// FURB171 +pub(crate) fn single_item_membership_test( + checker: &mut Checker, + expr: &Expr, + left: &Expr, + ops: &[CmpOp], + comparators: &[Expr], +) { + let ([op], [right]) = (ops, comparators) else { + return; + }; + + // Ensure that the comparison is a membership test. + let membership_test = match op { + CmpOp::In => MembershipTest::In, + CmpOp::NotIn => MembershipTest::NotIn, + _ => return, + }; + + // Check if the right-hand side is a single-item object. + let Some(item) = single_item(right) else { + return; + }; + + let mut diagnostic = + Diagnostic::new(SingleItemMembershipTest { membership_test }, expr.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + pad( + generate_comparison( + left, + &[membership_test.replacement_op()], + &[item.clone()], + expr.into(), + checker.indexer().comment_ranges(), + checker.locator(), + ), + expr.range(), + checker.locator(), + ), + expr.range(), + ))); + } + checker.diagnostics.push(diagnostic); +} + +/// Return the single item wrapped in Some if the expression contains a single +/// item, otherwise return None. +fn single_item(expr: &Expr) -> Option<&Expr> { + match expr { + Expr::List(list) if list.elts.len() == 1 => Some(&list.elts[0]), + Expr::Tuple(tuple) if tuple.elts.len() == 1 => Some(&tuple.elts[0]), + Expr::Set(set) if set.elts.len() == 1 => Some(&set.elts[0]), + string_expr @ Expr::Constant(ExprConstant { + value: Constant::Str(string), + .. + }) if string.chars().count() == 1 => Some(string_expr), + _ => None, + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum MembershipTest { + /// Ex) `1 in [1]` + In, + /// Ex) `1 not in [1]` + NotIn, +} + +impl MembershipTest { + /// Returns the replacement comparison operator for this membership test. + fn replacement_op(self) -> CmpOp { + match self { + MembershipTest::In => CmpOp::Eq, + MembershipTest::NotIn => CmpOp::NotEq, + } + } +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171.py.snap new file mode 100644 index 00000000000000..e1e847ad8d53be --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171.py.snap @@ -0,0 +1,123 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB171.py:3:4: FURB171 [*] Membership test against single-item container + | +1 | # Errors. +2 | +3 | if 1 in (1,): + | ^^^^^^^^^ FURB171 +4 | print("Single-element tuple") + | + = help: Convert to equality test + +ℹ Fix +1 1 | # Errors. +2 2 | +3 |-if 1 in (1,): + 3 |+if 1 == 1: +4 4 | print("Single-element tuple") +5 5 | +6 6 | if 1 in [1]: + +FURB171.py:6:4: FURB171 [*] Membership test against single-item container + | +4 | print("Single-element tuple") +5 | +6 | if 1 in [1]: + | ^^^^^^^^ FURB171 +7 | print("Single-element list") + | + = help: Convert to equality test + +ℹ Fix +3 3 | if 1 in (1,): +4 4 | print("Single-element tuple") +5 5 | +6 |-if 1 in [1]: + 6 |+if 1 == 1: +7 7 | print("Single-element list") +8 8 | +9 9 | if 1 in {1}: + +FURB171.py:9:4: FURB171 [*] Membership test against single-item container + | + 7 | print("Single-element list") + 8 | + 9 | if 1 in {1}: + | ^^^^^^^^ FURB171 +10 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Fix +6 6 | if 1 in [1]: +7 7 | print("Single-element list") +8 8 | +9 |-if 1 in {1}: + 9 |+if 1 == 1: +10 10 | print("Single-element set") +11 11 | +12 12 | if "a" in "a": + +FURB171.py:12:4: FURB171 [*] Membership test against single-item container + | +10 | print("Single-element set") +11 | +12 | if "a" in "a": + | ^^^^^^^^^^ FURB171 +13 | print("Single-element string") + | + = help: Convert to equality test + +ℹ Fix +9 9 | if 1 in {1}: +10 10 | print("Single-element set") +11 11 | +12 |-if "a" in "a": + 12 |+if "a" == "a": +13 13 | print("Single-element string") +14 14 | +15 15 | if 1 not in (1,): + +FURB171.py:15:4: FURB171 [*] Membership test against single-item container + | +13 | print("Single-element string") +14 | +15 | if 1 not in (1,): + | ^^^^^^^^^^^^^ FURB171 +16 | print("Check `not in` membership test") + | + = help: Convert to inequality test + +ℹ Fix +12 12 | if "a" in "a": +13 13 | print("Single-element string") +14 14 | +15 |-if 1 not in (1,): + 15 |+if 1 != 1: +16 16 | print("Check `not in` membership test") +17 17 | +18 18 | if not 1 in (1,): + +FURB171.py:18:8: FURB171 [*] Membership test against single-item container + | +16 | print("Check `not in` membership test") +17 | +18 | if not 1 in (1,): + | ^^^^^^^^^ FURB171 +19 | print("Check the negated membership test") + | + = help: Convert to equality test + +ℹ Fix +15 15 | if 1 not in (1,): +16 16 | print("Check `not in` membership test") +17 17 | +18 |-if not 1 in (1,): + 18 |+if not 1 == 1: +19 19 | print("Check the negated membership test") +20 20 | +21 21 | # Non-errors. + + diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index bd2a907e117c3e..598fb4b5ac3eea 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1,14 +1,19 @@ use std::borrow::Cow; use std::path::Path; +use ruff_python_trivia::CommentRanges; +use ruff_source_file::Locator; use smallvec::SmallVec; use ruff_text_size::{Ranged, TextRange}; use crate::call_path::CallPath; +use crate::node::AnyNodeRef; +use crate::parenthesize::parenthesized_range; use crate::statement_visitor::{walk_body, walk_stmt, StatementVisitor}; use crate::{ - self as ast, Arguments, Constant, ExceptHandler, Expr, MatchCase, Pattern, Stmt, TypeParam, + self as ast, Arguments, CmpOp, Constant, ExceptHandler, Expr, MatchCase, Pattern, Stmt, + TypeParam, }; /// Return `true` if the `Stmt` is a compound statement (as opposed to a simple statement). @@ -1129,6 +1134,58 @@ impl Truthiness { } } +pub fn generate_comparison( + left: &Expr, + ops: &[CmpOp], + comparators: &[Expr], + parent: AnyNodeRef, + comment_ranges: &CommentRanges, + locator: &Locator, +) -> String { + let start = left.start(); + let end = comparators.last().map_or_else(|| left.end(), Ranged::end); + let mut contents = String::with_capacity(usize::from(end - start)); + + // Add the left side of the comparison. + contents.push_str( + locator.slice( + parenthesized_range(left.into(), parent, comment_ranges, locator.contents()) + .unwrap_or(left.range()), + ), + ); + + for (op, comparator) in ops.iter().zip(comparators) { + // Add the operator. + contents.push_str(match op { + CmpOp::Eq => " == ", + CmpOp::NotEq => " != ", + CmpOp::Lt => " < ", + CmpOp::LtE => " <= ", + CmpOp::Gt => " > ", + CmpOp::GtE => " >= ", + CmpOp::In => " in ", + CmpOp::NotIn => " not in ", + CmpOp::Is => " is ", + CmpOp::IsNot => " is not ", + }); + + // Add the right side of the comparison. + contents.push_str( + locator.slice( + parenthesized_range( + comparator.into(), + parent, + comment_ranges, + locator.contents(), + ) + .unwrap_or(comparator.range()), + ), + ); + } + + contents +} + #[cfg(test)] mod tests { use std::borrow::Cow; diff --git a/ruff.schema.json b/ruff.schema.json index de0bf5807ed9d4..0149b046ee7753 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2684,6 +2684,7 @@ "FURB145", "FURB148", "FURB17", + "FURB171", "FURB177", "G", "G0",