From 1d1e7b7aea5297230b3103750ba713045b09780c Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 5 Jan 2025 16:47:04 +0000 Subject: [PATCH 1/2] [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) --- .../rules/single_item_membership_test.rs | 21 ++++++++++++++++--- ...es__refurb__tests__FURB171_FURB171.py.snap | 2 +- 2 files changed, 19 insertions(+), 4 deletions(-) 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 2d5cefacc4639..7a113a42af1e1 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,4 +1,4 @@ -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::generate_comparison; use ruff_python_ast::{self as ast, CmpOp, Expr, ExprStringLiteral}; @@ -25,6 +25,11 @@ use crate::fix::edits::pad; /// 1 == 1 /// ``` /// +/// ## Fix safety +/// +/// When the right-hand side is a string, the fix is marked as unsafe. +/// This is because `c in "a"` is true both when `c` is `"a"` and when `c` is the empty string. +/// /// ## 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) @@ -76,7 +81,8 @@ pub(crate) fn single_item_membership_test( let mut diagnostic = Diagnostic::new(SingleItemMembershipTest { membership_test }, expr.range()); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + + let edit = Edit::range_replacement( pad( generate_comparison( left, @@ -90,7 +96,16 @@ pub(crate) fn single_item_membership_test( checker.locator(), ), expr.range(), - ))); + ); + + let applicability = if right.is_string_literal_expr() { + Applicability::Unsafe + } else { + Applicability::Safe + }; + + diagnostic.set_fix(Fix::applicable_edit(edit, applicability)); + checker.diagnostics.push(diagnostic); } 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 index 7547b029589ab..7124e3e5c7f8d 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.py.snap @@ -71,7 +71,7 @@ FURB171.py:12:4: FURB171 [*] Membership test against single-item container | = help: Convert to equality test -ℹ Safe fix +ℹ Unsafe fix 9 9 | if 1 in {1}: 10 10 | print("Single-element set") 11 11 | From 3567e201e68795503bc0f4d36f9bb9f558b8f358 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 5 Jan 2025 17:46:38 +0000 Subject: [PATCH 2/2] Per review --- .../rules/refurb/rules/single_item_membership_test.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 7a113a42af1e1..774fc4632146f 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 @@ -28,7 +28,8 @@ use crate::fix::edits::pad; /// ## Fix safety /// /// When the right-hand side is a string, the fix is marked as unsafe. -/// This is because `c in "a"` is true both when `c` is `"a"` and when `c` is the empty string. +/// This is because `c in "a"` is true both when `c` is `"a"` and when `c` is the empty string, +/// so the fix can change the behavior of your program in these cases. /// /// ## References /// - [Python documentation: Comparisons](https://docs.python.org/3/reference/expressions.html#comparisons) @@ -79,8 +80,7 @@ pub(crate) fn single_item_membership_test( return; }; - let mut diagnostic = - Diagnostic::new(SingleItemMembershipTest { membership_test }, expr.range()); + let diagnostic = Diagnostic::new(SingleItemMembershipTest { membership_test }, expr.range()); let edit = Edit::range_replacement( pad( @@ -104,9 +104,9 @@ pub(crate) fn single_item_membership_test( Applicability::Safe }; - diagnostic.set_fix(Fix::applicable_edit(edit, applicability)); + let fix = Fix::applicable_edit(edit, applicability); - checker.diagnostics.push(diagnostic); + checker.diagnostics.push(diagnostic.with_fix(fix)); } /// Return the single item wrapped in `Some` if the expression contains a single