mirror of https://github.com/astral-sh/ruff
[`refurb`] Mark `single-item-membership-test` fix as always unsafe (`FURB171`) (#20279)
<!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary <!-- What's the purpose of the change? What does it do, and why? --> Fixes #20255 Mark single-item-membership-test fixes as always unsafe - Always set `Applicability::Unsafe` for FURB171 fixes - Update “Fix safety” docs to reflect always-unsafe behavior - Expand tests (not in, nested set/frozenset, commented args) ## Test Plan <!-- How was it tested? --> I have added new test cases to `crates/ruff_linter/resources/test/fixtures/refurb/FURB171_0.py` and `crates/ruff_linter/resources/test/fixtures/refurb/FURB171_1.py`. --------- Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
This commit is contained in:
parent
1758f26d94
commit
821b2f8b2e
|
|
@ -117,3 +117,31 @@ def _():
|
||||||
# After
|
# After
|
||||||
] and \
|
] and \
|
||||||
0 < 1: ...
|
0 < 1: ...
|
||||||
|
|
||||||
|
# https://github.com/astral-sh/ruff/issues/20255
|
||||||
|
import math
|
||||||
|
|
||||||
|
# NaN behavior differences
|
||||||
|
if math.nan in [math.nan]:
|
||||||
|
print("This is True")
|
||||||
|
|
||||||
|
if math.nan in (math.nan,):
|
||||||
|
print("This is True")
|
||||||
|
|
||||||
|
if math.nan in {math.nan}:
|
||||||
|
print("This is True")
|
||||||
|
|
||||||
|
# Potential type differences with custom __eq__ methods
|
||||||
|
class CustomEq:
|
||||||
|
def __eq__(self, other):
|
||||||
|
return "custom"
|
||||||
|
|
||||||
|
obj = CustomEq()
|
||||||
|
if obj in [CustomEq()]:
|
||||||
|
pass
|
||||||
|
|
||||||
|
if obj in (CustomEq(),):
|
||||||
|
pass
|
||||||
|
|
||||||
|
if obj in {CustomEq()}:
|
||||||
|
pass
|
||||||
|
|
|
||||||
|
|
@ -51,3 +51,13 @@ if 1 in set(1,2):
|
||||||
|
|
||||||
if 1 in set((x for x in range(2))):
|
if 1 in set((x for x in range(2))):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
# https://github.com/astral-sh/ruff/issues/20255
|
||||||
|
import math
|
||||||
|
|
||||||
|
# set() and frozenset() with NaN
|
||||||
|
if math.nan in set([math.nan]):
|
||||||
|
print("This should be marked unsafe")
|
||||||
|
|
||||||
|
if math.nan in frozenset([math.nan]):
|
||||||
|
print("This should be marked unsafe")
|
||||||
|
|
|
||||||
|
|
@ -6,7 +6,7 @@ use ruff_text_size::Ranged;
|
||||||
|
|
||||||
use crate::checkers::ast::Checker;
|
use crate::checkers::ast::Checker;
|
||||||
use crate::fix::edits::pad;
|
use crate::fix::edits::pad;
|
||||||
use crate::{Applicability, Edit, Fix, FixAvailability, Violation};
|
use crate::{Edit, Fix, FixAvailability, Violation};
|
||||||
|
|
||||||
/// ## What it does
|
/// ## What it does
|
||||||
/// Checks for membership tests against single-item containers.
|
/// Checks for membership tests against single-item containers.
|
||||||
|
|
@ -27,13 +27,16 @@ use crate::{Applicability, Edit, Fix, FixAvailability, Violation};
|
||||||
/// ```
|
/// ```
|
||||||
///
|
///
|
||||||
/// ## Fix safety
|
/// ## Fix safety
|
||||||
|
/// The fix is always marked as unsafe.
|
||||||
///
|
///
|
||||||
/// When the right-hand side is a string, the fix is marked as unsafe.
|
/// When the right-hand side is a string, this fix can change the behavior of your program.
|
||||||
/// 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.
|
|
||||||
///
|
///
|
||||||
/// Additionally, if there are comments within the fix's range,
|
/// Additionally, converting `in`/`not in` against a single-item container to `==`/`!=` can
|
||||||
/// it will also be marked as unsafe.
|
/// change runtime behavior: `in` may consider identity (e.g., `NaN`) and always
|
||||||
|
/// yields a `bool`.
|
||||||
|
///
|
||||||
|
/// Comments within the replacement range will also be removed.
|
||||||
///
|
///
|
||||||
/// ## References
|
/// ## References
|
||||||
/// - [Python documentation: Comparisons](https://docs.python.org/3/reference/expressions.html#comparisons)
|
/// - [Python documentation: Comparisons](https://docs.python.org/3/reference/expressions.html#comparisons)
|
||||||
|
|
@ -100,14 +103,8 @@ pub(crate) fn single_item_membership_test(
|
||||||
expr.range(),
|
expr.range(),
|
||||||
);
|
);
|
||||||
|
|
||||||
let applicability =
|
// All supported cases can change runtime behavior; mark as unsafe.
|
||||||
if right.is_string_literal_expr() || checker.comment_ranges().intersects(expr.range()) {
|
let fix = Fix::unsafe_edit(edit);
|
||||||
Applicability::Unsafe
|
|
||||||
} else {
|
|
||||||
Applicability::Safe
|
|
||||||
};
|
|
||||||
|
|
||||||
let fix = Fix::applicable_edit(edit, applicability);
|
|
||||||
|
|
||||||
checker
|
checker
|
||||||
.report_diagnostic(SingleItemMembershipTest { membership_test }, expr.range())
|
.report_diagnostic(SingleItemMembershipTest { membership_test }, expr.range())
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,7 @@ help: Convert to equality test
|
||||||
4 | print("Single-element tuple")
|
4 | print("Single-element tuple")
|
||||||
5 |
|
5 |
|
||||||
6 | if 1 in [1]:
|
6 | if 1 in [1]:
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_0.py:6:4
|
--> FURB171_0.py:6:4
|
||||||
|
|
@ -37,6 +38,7 @@ help: Convert to equality test
|
||||||
7 | print("Single-element list")
|
7 | print("Single-element list")
|
||||||
8 |
|
8 |
|
||||||
9 | if 1 in {1}:
|
9 | if 1 in {1}:
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_0.py:9:4
|
--> FURB171_0.py:9:4
|
||||||
|
|
@ -56,6 +58,7 @@ help: Convert to equality test
|
||||||
10 | print("Single-element set")
|
10 | print("Single-element set")
|
||||||
11 |
|
11 |
|
||||||
12 | if "a" in "a":
|
12 | if "a" in "a":
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_0.py:12:4
|
--> FURB171_0.py:12:4
|
||||||
|
|
@ -95,6 +98,7 @@ help: Convert to inequality test
|
||||||
16 | print("Check `not in` membership test")
|
16 | print("Check `not in` membership test")
|
||||||
17 |
|
17 |
|
||||||
18 | if not 1 in (1,):
|
18 | if not 1 in (1,):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_0.py:18:8
|
--> FURB171_0.py:18:8
|
||||||
|
|
@ -114,6 +118,7 @@ help: Convert to equality test
|
||||||
19 | print("Check the negated membership test")
|
19 | print("Check the negated membership test")
|
||||||
20 |
|
20 |
|
||||||
21 | # Non-errors.
|
21 | # Non-errors.
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_0.py:52:5
|
--> FURB171_0.py:52:5
|
||||||
|
|
@ -344,4 +349,122 @@ help: Convert to inequality test
|
||||||
- ] and \
|
- ] and \
|
||||||
113 + if foo != bar and \
|
113 + if foo != bar and \
|
||||||
114 | 0 < 1: ...
|
114 | 0 < 1: ...
|
||||||
|
115 |
|
||||||
|
116 | # https://github.com/astral-sh/ruff/issues/20255
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_0.py:125:4
|
||||||
|
|
|
||||||
|
124 | # NaN behavior differences
|
||||||
|
125 | if math.nan in [math.nan]:
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^^^^
|
||||||
|
126 | print("This is True")
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
122 | import math
|
||||||
|
123 |
|
||||||
|
124 | # NaN behavior differences
|
||||||
|
- if math.nan in [math.nan]:
|
||||||
|
125 + if math.nan == math.nan:
|
||||||
|
126 | print("This is True")
|
||||||
|
127 |
|
||||||
|
128 | if math.nan in (math.nan,):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_0.py:128:4
|
||||||
|
|
|
||||||
|
126 | print("This is True")
|
||||||
|
127 |
|
||||||
|
128 | if math.nan in (math.nan,):
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^^^^^
|
||||||
|
129 | print("This is True")
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
125 | if math.nan in [math.nan]:
|
||||||
|
126 | print("This is True")
|
||||||
|
127 |
|
||||||
|
- if math.nan in (math.nan,):
|
||||||
|
128 + if math.nan == math.nan:
|
||||||
|
129 | print("This is True")
|
||||||
|
130 |
|
||||||
|
131 | if math.nan in {math.nan}:
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_0.py:131:4
|
||||||
|
|
|
||||||
|
129 | print("This is True")
|
||||||
|
130 |
|
||||||
|
131 | if math.nan in {math.nan}:
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^^^^
|
||||||
|
132 | print("This is True")
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
128 | if math.nan in (math.nan,):
|
||||||
|
129 | print("This is True")
|
||||||
|
130 |
|
||||||
|
- if math.nan in {math.nan}:
|
||||||
|
131 + if math.nan == math.nan:
|
||||||
|
132 | print("This is True")
|
||||||
|
133 |
|
||||||
|
134 | # Potential type differences with custom __eq__ methods
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_0.py:140:4
|
||||||
|
|
|
||||||
|
139 | obj = CustomEq()
|
||||||
|
140 | if obj in [CustomEq()]:
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^
|
||||||
|
141 | pass
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
137 | return "custom"
|
||||||
|
138 |
|
||||||
|
139 | obj = CustomEq()
|
||||||
|
- if obj in [CustomEq()]:
|
||||||
|
140 + if obj == CustomEq():
|
||||||
|
141 | pass
|
||||||
|
142 |
|
||||||
|
143 | if obj in (CustomEq(),):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_0.py:143:4
|
||||||
|
|
|
||||||
|
141 | pass
|
||||||
|
142 |
|
||||||
|
143 | if obj in (CustomEq(),):
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^^
|
||||||
|
144 | pass
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
140 | if obj in [CustomEq()]:
|
||||||
|
141 | pass
|
||||||
|
142 |
|
||||||
|
- if obj in (CustomEq(),):
|
||||||
|
143 + if obj == CustomEq():
|
||||||
|
144 | pass
|
||||||
|
145 |
|
||||||
|
146 | if obj in {CustomEq()}:
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_0.py:146:4
|
||||||
|
|
|
||||||
|
144 | pass
|
||||||
|
145 |
|
||||||
|
146 | if obj in {CustomEq()}:
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^
|
||||||
|
147 | pass
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
143 | if obj in (CustomEq(),):
|
||||||
|
144 | pass
|
||||||
|
145 |
|
||||||
|
- if obj in {CustomEq()}:
|
||||||
|
146 + if obj == CustomEq():
|
||||||
|
147 | pass
|
||||||
note: This is an unsafe fix and may change runtime behavior
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,7 @@ help: Convert to equality test
|
||||||
4 | print("Single-element set")
|
4 | print("Single-element set")
|
||||||
5 |
|
5 |
|
||||||
6 | if 1 in set((1,)):
|
6 | if 1 in set((1,)):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_1.py:6:4
|
--> FURB171_1.py:6:4
|
||||||
|
|
@ -37,6 +38,7 @@ help: Convert to equality test
|
||||||
7 | print("Single-element set")
|
7 | print("Single-element set")
|
||||||
8 |
|
8 |
|
||||||
9 | if 1 in set({1}):
|
9 | if 1 in set({1}):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_1.py:9:4
|
--> FURB171_1.py:9:4
|
||||||
|
|
@ -56,6 +58,7 @@ help: Convert to equality test
|
||||||
10 | print("Single-element set")
|
10 | print("Single-element set")
|
||||||
11 |
|
11 |
|
||||||
12 | if 1 in frozenset([1]):
|
12 | if 1 in frozenset([1]):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_1.py:12:4
|
--> FURB171_1.py:12:4
|
||||||
|
|
@ -75,6 +78,7 @@ help: Convert to equality test
|
||||||
13 | print("Single-element set")
|
13 | print("Single-element set")
|
||||||
14 |
|
14 |
|
||||||
15 | if 1 in frozenset((1,)):
|
15 | if 1 in frozenset((1,)):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_1.py:15:4
|
--> FURB171_1.py:15:4
|
||||||
|
|
@ -94,6 +98,7 @@ help: Convert to equality test
|
||||||
16 | print("Single-element set")
|
16 | print("Single-element set")
|
||||||
17 |
|
17 |
|
||||||
18 | if 1 in frozenset({1}):
|
18 | if 1 in frozenset({1}):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_1.py:18:4
|
--> FURB171_1.py:18:4
|
||||||
|
|
@ -113,6 +118,7 @@ help: Convert to equality test
|
||||||
19 | print("Single-element set")
|
19 | print("Single-element set")
|
||||||
20 |
|
20 |
|
||||||
21 | if 1 in set(set([1])):
|
21 | if 1 in set(set([1])):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
FURB171 [*] Membership test against single-item container
|
FURB171 [*] Membership test against single-item container
|
||||||
--> FURB171_1.py:21:4
|
--> FURB171_1.py:21:4
|
||||||
|
|
@ -132,3 +138,41 @@ help: Convert to equality test
|
||||||
22 | print('Recursive solution')
|
22 | print('Recursive solution')
|
||||||
23 |
|
23 |
|
||||||
24 |
|
24 |
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_1.py:59:4
|
||||||
|
|
|
||||||
|
58 | # set() and frozenset() with NaN
|
||||||
|
59 | if math.nan in set([math.nan]):
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||||
|
60 | print("This should be marked unsafe")
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
56 | import math
|
||||||
|
57 |
|
||||||
|
58 | # set() and frozenset() with NaN
|
||||||
|
- if math.nan in set([math.nan]):
|
||||||
|
59 + if math.nan == math.nan:
|
||||||
|
60 | print("This should be marked unsafe")
|
||||||
|
61 |
|
||||||
|
62 | if math.nan in frozenset([math.nan]):
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
||||||
|
FURB171 [*] Membership test against single-item container
|
||||||
|
--> FURB171_1.py:62:4
|
||||||
|
|
|
||||||
|
60 | print("This should be marked unsafe")
|
||||||
|
61 |
|
||||||
|
62 | if math.nan in frozenset([math.nan]):
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||||
|
63 | print("This should be marked unsafe")
|
||||||
|
|
|
||||||
|
help: Convert to equality test
|
||||||
|
59 | if math.nan in set([math.nan]):
|
||||||
|
60 | print("This should be marked unsafe")
|
||||||
|
61 |
|
||||||
|
- if math.nan in frozenset([math.nan]):
|
||||||
|
62 + if math.nan == math.nan:
|
||||||
|
63 | print("This should be marked unsafe")
|
||||||
|
note: This is an unsafe fix and may change runtime behavior
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue