From 9d3cad95bca58788e58c03f28dde35b29525a4fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20N=C3=A4slund?= Date: Thu, 29 May 2025 20:59:49 +0200 Subject: [PATCH] [`refurb`] Add coverage of `set` and `frozenset` calls (`FURB171`) (#18035) ## Summary Adds coverage of using set(...) in addition to `{...} in SingleItemMembershipTest. Fixes #15792 (and replaces the old PR #15793) ## Test Plan Updated unit test and snapshot. Steps to reproduce are in the issue linked above. --- .../refurb/{FURB171.py => FURB171_0.py} | 0 .../test/fixtures/refurb/FURB171_1.py | 53 +++++++ crates/ruff_linter/src/rules/refurb/mod.rs | 3 +- .../rules/single_item_membership_test.rs | 26 +++- ..._refurb__tests__FURB171_FURB171_0.py.snap} | 26 ++-- ...__refurb__tests__FURB171_FURB171_1.py.snap | 141 ++++++++++++++++++ 6 files changed, 232 insertions(+), 17 deletions(-) rename crates/ruff_linter/resources/test/fixtures/refurb/{FURB171.py => FURB171_0.py} (100%) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB171_1.py rename crates/ruff_linter/src/rules/refurb/snapshots/{ruff_linter__rules__refurb__tests__FURB171_FURB171.py.snap => ruff_linter__rules__refurb__tests__FURB171_FURB171_0.py.snap} (87%) create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171_1.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB171.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB171_0.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/refurb/FURB171.py rename to crates/ruff_linter/resources/test/fixtures/refurb/FURB171_0.py diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB171_1.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB171_1.py new file mode 100644 index 0000000000..41109f6cfa --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB171_1.py @@ -0,0 +1,53 @@ +# Errors. + +if 1 in set([1]): + print("Single-element set") + +if 1 in set((1,)): + print("Single-element set") + +if 1 in set({1}): + print("Single-element set") + +if 1 in frozenset([1]): + print("Single-element set") + +if 1 in frozenset((1,)): + print("Single-element set") + +if 1 in frozenset({1}): + print("Single-element set") + +if 1 in set(set([1])): + print('Recursive solution') + + + +# Non-errors. + +if 1 in set((1, 2)): + pass + +if 1 in set([1, 2]): + pass + +if 1 in set({1, 2}): + pass + +if 1 in frozenset((1, 2)): + pass + +if 1 in frozenset([1, 2]): + pass + +if 1 in frozenset({1, 2}): + pass + +if 1 in set(1,): + pass + +if 1 in set(1,2): + pass + +if 1 in set((x for x in range(2))): + pass diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index d841bcafe1..a14531e43a 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -35,7 +35,8 @@ mod tests { #[test_case(Rule::UnnecessaryFromFloat, Path::new("FURB164.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"))] + #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171_0.py"))] + #[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171_1.py"))] #[test_case(Rule::BitCount, Path::new("FURB161.py"))] #[test_case(Rule::IntOnSlicedStr, Path::new("FURB166.py"))] #[test_case(Rule::RegexFlagAlias, Path::new("FURB167.py"))] 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 index 536a5008f8..f0b9930bf2 100644 --- 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 @@ -1,6 +1,7 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::helpers::generate_comparison; use ruff_python_ast::{self as ast, CmpOp, Expr, ExprStringLiteral}; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -78,8 +79,8 @@ pub(crate) fn single_item_membership_test( _ => return, }; - // Check if the right-hand side is a single-item object. - let Some(item) = single_item(right) else { + // Check if the right-hand side is a single-item object + let Some(item) = single_item(right, checker.semantic()) else { return; }; @@ -115,7 +116,7 @@ pub(crate) fn single_item_membership_test( /// Return the single item wrapped in `Some` if the expression contains a single /// item, otherwise return `None`. -fn single_item(expr: &Expr) -> Option<&Expr> { +fn single_item<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Expr> { match expr { Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) @@ -124,6 +125,19 @@ fn single_item(expr: &Expr) -> Option<&Expr> { [item] => Some(item), _ => None, }, + Expr::Call(ast::ExprCall { + func, + arguments, + range: _, + }) => { + if arguments.len() != 1 || !is_set_method(func, semantic) { + return None; + } + + arguments + .find_positional(0) + .and_then(|arg| single_item(arg, semantic)) + } string_expr @ Expr::StringLiteral(ExprStringLiteral { value: string, .. }) if string.chars().count() == 1 => { @@ -133,6 +147,12 @@ fn single_item(expr: &Expr) -> Option<&Expr> { } } +fn is_set_method(func: &Expr, semantic: &SemanticModel) -> bool { + ["set", "frozenset"] + .iter() + .any(|s| semantic.match_builtin_expr(func, s)) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum MembershipTest { /// Ex) `1 in [1]` 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_0.py.snap similarity index 87% rename from crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171.py.snap rename to crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171_0.py.snap index e4af1c0427..8bc2c8a6af 100644 --- 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_0.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/refurb/mod.rs --- -FURB171.py:3:4: FURB171 [*] Membership test against single-item container +FURB171_0.py:3:4: FURB171 [*] Membership test against single-item container | 1 | # Errors. 2 | @@ -20,7 +20,7 @@ FURB171.py:3:4: FURB171 [*] Membership test against single-item container 5 5 | 6 6 | if 1 in [1]: -FURB171.py:6:4: FURB171 [*] Membership test against single-item container +FURB171_0.py:6:4: FURB171 [*] Membership test against single-item container | 4 | print("Single-element tuple") 5 | @@ -40,7 +40,7 @@ FURB171.py:6:4: FURB171 [*] Membership test against single-item container 8 8 | 9 9 | if 1 in {1}: -FURB171.py:9:4: FURB171 [*] Membership test against single-item container +FURB171_0.py:9:4: FURB171 [*] Membership test against single-item container | 7 | print("Single-element list") 8 | @@ -60,7 +60,7 @@ FURB171.py:9:4: FURB171 [*] Membership test against single-item container 11 11 | 12 12 | if "a" in "a": -FURB171.py:12:4: FURB171 [*] Membership test against single-item container +FURB171_0.py:12:4: FURB171 [*] Membership test against single-item container | 10 | print("Single-element set") 11 | @@ -80,7 +80,7 @@ FURB171.py:12:4: FURB171 [*] Membership test against single-item container 14 14 | 15 15 | if 1 not in (1,): -FURB171.py:15:4: FURB171 [*] Membership test against single-item container +FURB171_0.py:15:4: FURB171 [*] Membership test against single-item container | 13 | print("Single-element string") 14 | @@ -100,7 +100,7 @@ FURB171.py:15:4: FURB171 [*] Membership test against single-item container 17 17 | 18 18 | if not 1 in (1,): -FURB171.py:18:8: FURB171 [*] Membership test against single-item container +FURB171_0.py:18:8: FURB171 [*] Membership test against single-item container | 16 | print("Check `not in` membership test") 17 | @@ -120,7 +120,7 @@ FURB171.py:18:8: FURB171 [*] Membership test against single-item container 20 20 | 21 21 | # Non-errors. -FURB171.py:52:5: FURB171 [*] Membership test against single-item container +FURB171_0.py:52:5: FURB171 [*] Membership test against single-item container | 51 | # https://github.com/astral-sh/ruff/issues/10063 52 | _ = a in ( @@ -147,7 +147,7 @@ FURB171.py:52:5: FURB171 [*] Membership test against single-item container 57 54 | _ = a in ( # Foo1 58 55 | ( # Foo2 -FURB171.py:57:5: FURB171 [*] Membership test against single-item container +FURB171_0.py:57:5: FURB171 [*] Membership test against single-item container | 55 | ) 56 | @@ -199,7 +199,7 @@ FURB171.py:57:5: FURB171 [*] Membership test against single-item container 74 63 | foo = ( 75 64 | lorem() -FURB171.py:77:28: FURB171 [*] Membership test against single-item container +FURB171_0.py:77:28: FURB171 [*] Membership test against single-item container | 75 | lorem() 76 | .ipsum() @@ -228,7 +228,7 @@ FURB171.py:77:28: FURB171 [*] Membership test against single-item container 83 79 | 84 80 | foo = ( -FURB171.py:87:28: FURB171 [*] Membership test against single-item container +FURB171_0.py:87:28: FURB171 [*] Membership test against single-item container | 85 | lorem() 86 | .ipsum() @@ -262,7 +262,7 @@ FURB171.py:87:28: FURB171 [*] Membership test against single-item container 95 93 | 96 94 | foo = lorem() \ -FURB171.py:98:24: FURB171 [*] Membership test against single-item container +FURB171_0.py:98:24: FURB171 [*] Membership test against single-item container | 96 | foo = lorem() \ 97 | .ipsum() \ @@ -292,7 +292,7 @@ FURB171.py:98:24: FURB171 [*] Membership test against single-item container 104 100 | def _(): 105 101 | if foo not \ -FURB171.py:105:8: FURB171 [*] Membership test against single-item container +FURB171_0.py:105:8: FURB171 [*] Membership test against single-item container | 104 | def _(): 105 | if foo not \ @@ -323,7 +323,7 @@ FURB171.py:105:8: FURB171 [*] Membership test against single-item container 112 107 | def _(): 113 108 | if foo not \ -FURB171.py:113:8: FURB171 [*] Membership test against single-item container +FURB171_0.py:113:8: FURB171 [*] Membership test against single-item container | 112 | def _(): 113 | if foo not \ diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171_1.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171_1.py.snap new file mode 100644 index 0000000000..01e249adf5 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB171_FURB171_1.py.snap @@ -0,0 +1,141 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB171_1.py:3:4: FURB171 [*] Membership test against single-item container + | +1 | # Errors. +2 | +3 | if 1 in set([1]): + | ^^^^^^^^^^^^^ FURB171 +4 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Safe fix +1 1 | # Errors. +2 2 | +3 |-if 1 in set([1]): + 3 |+if 1 == 1: +4 4 | print("Single-element set") +5 5 | +6 6 | if 1 in set((1,)): + +FURB171_1.py:6:4: FURB171 [*] Membership test against single-item container + | +4 | print("Single-element set") +5 | +6 | if 1 in set((1,)): + | ^^^^^^^^^^^^^^ FURB171 +7 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Safe fix +3 3 | if 1 in set([1]): +4 4 | print("Single-element set") +5 5 | +6 |-if 1 in set((1,)): + 6 |+if 1 == 1: +7 7 | print("Single-element set") +8 8 | +9 9 | if 1 in set({1}): + +FURB171_1.py:9:4: FURB171 [*] Membership test against single-item container + | + 7 | print("Single-element set") + 8 | + 9 | if 1 in set({1}): + | ^^^^^^^^^^^^^ FURB171 +10 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Safe fix +6 6 | if 1 in set((1,)): +7 7 | print("Single-element set") +8 8 | +9 |-if 1 in set({1}): + 9 |+if 1 == 1: +10 10 | print("Single-element set") +11 11 | +12 12 | if 1 in frozenset([1]): + +FURB171_1.py:12:4: FURB171 [*] Membership test against single-item container + | +10 | print("Single-element set") +11 | +12 | if 1 in frozenset([1]): + | ^^^^^^^^^^^^^^^^^^^ FURB171 +13 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Safe fix +9 9 | if 1 in set({1}): +10 10 | print("Single-element set") +11 11 | +12 |-if 1 in frozenset([1]): + 12 |+if 1 == 1: +13 13 | print("Single-element set") +14 14 | +15 15 | if 1 in frozenset((1,)): + +FURB171_1.py:15:4: FURB171 [*] Membership test against single-item container + | +13 | print("Single-element set") +14 | +15 | if 1 in frozenset((1,)): + | ^^^^^^^^^^^^^^^^^^^^ FURB171 +16 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Safe fix +12 12 | if 1 in frozenset([1]): +13 13 | print("Single-element set") +14 14 | +15 |-if 1 in frozenset((1,)): + 15 |+if 1 == 1: +16 16 | print("Single-element set") +17 17 | +18 18 | if 1 in frozenset({1}): + +FURB171_1.py:18:4: FURB171 [*] Membership test against single-item container + | +16 | print("Single-element set") +17 | +18 | if 1 in frozenset({1}): + | ^^^^^^^^^^^^^^^^^^^ FURB171 +19 | print("Single-element set") + | + = help: Convert to equality test + +ℹ Safe fix +15 15 | if 1 in frozenset((1,)): +16 16 | print("Single-element set") +17 17 | +18 |-if 1 in frozenset({1}): + 18 |+if 1 == 1: +19 19 | print("Single-element set") +20 20 | +21 21 | if 1 in set(set([1])): + +FURB171_1.py:21:4: FURB171 [*] Membership test against single-item container + | +19 | print("Single-element set") +20 | +21 | if 1 in set(set([1])): + | ^^^^^^^^^^^^^^^^^^ FURB171 +22 | print('Recursive solution') + | + = help: Convert to equality test + +ℹ Safe fix +18 18 | if 1 in frozenset({1}): +19 19 | print("Single-element set") +20 20 | +21 |-if 1 in set(set([1])): + 21 |+if 1 == 1: +22 22 | print('Recursive solution') +23 23 | +24 24 |