Skip to content

Commit 498c05f

Browse files
alex-700alex-700
committed
[refurb] Implement manual_set_update (FURB142)
1 parent bd07c13 commit 498c05f

File tree

8 files changed

+341
-0
lines changed

8 files changed

+341
-0
lines changed

‎crates/ruff_linter/resources/test/fixtures/refurb/FURB142.py

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
# Errors
2+
3+
s = set()
4+
5+
for x in [1, 2, 3]:
6+
s.add(x)
7+
8+
for x in {1, 2, 3}:
9+
s.add(x)
10+
11+
for x in (1, 2, 3):
12+
s.add(x)
13+
14+
for x in (1, 2, 3):
15+
s.discard(x)
16+
17+
for x in (1, 2, 3):
18+
s.add(x + 1)
19+
20+
for x, y in ((1, 2), (3, 4)):
21+
s.add((x, y))
22+
23+
num = 123
24+
25+
for x in (1, 2, 3):
26+
s.add(num)
27+
28+
29+
# False negative
30+
31+
class C:
32+
s: set[int]
33+
34+
35+
c = C()
36+
for x in (1, 2, 3):
37+
c.s.add(x)
38+
39+
# Ok
40+
41+
s.update(x for x in (1, 2, 3))
42+
43+
for x in (1, 2, 3):
44+
s.add(x)
45+
else:
46+
pass
47+
48+
49+
async def f(y):
50+
async for x in y:
51+
s.add(x)
52+
53+
54+
def g():
55+
for x in (set(),):
56+
x.add(x)

‎crates/ruff_linter/src/checkers/ast/analyze/statement.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1315,6 +1315,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
13151315
if checker.enabled(Rule::TryExceptInLoop) {
13161316
perflint::rules::try_except_in_loop(checker, body);
13171317
}
1318+
if checker.enabled(Rule::ManualSetUpdate) {
1319+
refurb::rules::manual_set_update(checker, for_stmt);
1320+
}
13181321
}
13191322
}
13201323
Stmt::Try(ast::StmtTry {

‎crates/ruff_linter/src/codes.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1044,6 +1044,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
10441044
(Refurb, "132") => (RuleGroup::Nursery, rules::refurb::rules::CheckAndRemoveFromSet),
10451045
(Refurb, "136") => (RuleGroup::Preview, rules::refurb::rules::IfExprMinMax),
10461046
(Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap),
1047+
(Refurb, "142") => (RuleGroup::Preview, rules::refurb::rules::ManualSetUpdate),
10471048
(Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
10481049
(Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),
10491050
(Refurb, "152") => (RuleGroup::Preview, rules::refurb::rules::MathConstant),

‎crates/ruff_linter/src/rules/refurb/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ mod tests {
2222
#[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))]
2323
#[test_case(Rule::IfExprMinMax, Path::new("FURB136.py"))]
2424
#[test_case(Rule::ReimplementedStarmap, Path::new("FURB140.py"))]
25+
#[test_case(Rule::ManualSetUpdate, Path::new("FURB142.py"))]
2526
#[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
2627
#[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))]
2728
#[test_case(Rule::MathConstant, Path::new("FURB152.py"))]

‎crates/ruff_linter/src/rules/refurb/rules/manual_set_update.rs

+116
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
2+
use ruff_macros::{derive_message_formats, violation};
3+
use ruff_python_ast::{Expr, Stmt, StmtFor};
4+
use ruff_python_semantic::analyze::typing;
5+
6+
use crate::checkers::ast::Checker;
7+
8+
/// ## What it does
9+
/// Checks for updating list with iterable object by calling `add`/`discard` on each element
10+
/// separately in the `for` loop.
11+
///
12+
/// ## Why is this bad?
13+
/// When adding/removing a batch of elements to/from a set, it's more readable to call
14+
/// an appropriate method, instead of add/remove elements one-by-one.
15+
///
16+
/// ## Example
17+
/// ```python
18+
/// s = set()
19+
///
20+
/// for x in (1, 2, 3):
21+
/// s.add(x)
22+
///
23+
/// for x in (1, 2, 3):
24+
/// s.discard(x)
25+
/// ```
26+
///
27+
/// Use instead:
28+
/// ```python
29+
/// s = set()
30+
///
31+
/// s.update((1, 2, 3))
32+
/// s.difference_update((1, 2, 3))
33+
/// ```
34+
///
35+
/// ## References
36+
/// - [Python documentation: `set`](https://docs.python.org/3/library/stdtypes.html#set)
37+
#[violation]
38+
pub struct ManualSetUpdate {
39+
batch_method_name: &'static str,
40+
}
41+
42+
impl AlwaysFixableViolation for ManualSetUpdate {
43+
#[derive_message_formats]
44+
fn message(&self) -> String {
45+
format!("Use of manual set update")
46+
}
47+
48+
fn fix_title(&self) -> String {
49+
format!("Replace with `{}`", self.batch_method_name)
50+
}
51+
}
52+
53+
// FURB142
54+
pub(crate) fn manual_set_update(checker: &mut Checker, for_stmt: &StmtFor) {
55+
if !for_stmt.orelse.is_empty() {
56+
return;
57+
}
58+
let [Stmt::Expr(stmt_expr)] = for_stmt.body.as_slice() else {
59+
return;
60+
};
61+
let Expr::Call(expr_call) = stmt_expr.value.as_ref() else {
62+
return;
63+
};
64+
let Expr::Attribute(expr_attr) = expr_call.func.as_ref() else {
65+
return;
66+
};
67+
if !expr_call.arguments.keywords.is_empty() {
68+
return;
69+
}
70+
71+
let batch_method_name = match expr_attr.attr.as_str() {
72+
"add" => "update",
73+
"discard" => "difference_update",
74+
_ => {
75+
return;
76+
}
77+
};
78+
79+
let Expr::Name(set) = expr_attr.value.as_ref() else {
80+
return;
81+
};
82+
83+
if !checker
84+
.semantic()
85+
.resolve_name(set)
86+
.is_some_and(|s| typing::is_set(checker.semantic().binding(s), checker.semantic()))
87+
{
88+
return;
89+
}
90+
let [arg] = expr_call.arguments.args.as_ref() else {
91+
return;
92+
};
93+
94+
let content = match (for_stmt.target.as_ref(), arg) {
95+
(Expr::Name(for_target), Expr::Name(arg)) if for_target.id == arg.id => {
96+
format!(
97+
"{}.{batch_method_name}({})",
98+
set.id,
99+
checker.locator().slice(for_stmt.iter.as_ref())
100+
)
101+
}
102+
(for_target, arg) => format!(
103+
"{}.{batch_method_name}({} for {} in {})",
104+
set.id,
105+
checker.locator().slice(arg),
106+
checker.locator().slice(for_target),
107+
checker.locator().slice(for_stmt.iter.as_ref())
108+
),
109+
};
110+
111+
checker.diagnostics.push(
112+
Diagnostic::new(ManualSetUpdate { batch_method_name }, for_stmt.range).with_fix(
113+
Fix::safe_edit(Edit::range_replacement(content, for_stmt.range)),
114+
),
115+
);
116+
}

‎crates/ruff_linter/src/rules/refurb/rules/mod.rs

+2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ pub(crate) use if_expr_min_max::*;
66
pub(crate) use implicit_cwd::*;
77
pub(crate) use isinstance_type_none::*;
88
pub(crate) use list_reverse_copy::*;
9+
pub(crate) use manual_set_update::*;
910
pub(crate) use math_constant::*;
1011
pub(crate) use metaclass_abcmeta::*;
1112
pub(crate) use print_empty_string::*;
@@ -30,6 +31,7 @@ mod if_expr_min_max;
3031
mod implicit_cwd;
3132
mod isinstance_type_none;
3233
mod list_reverse_copy;
34+
mod manual_set_update;
3335
mod math_constant;
3436
mod metaclass_abcmeta;
3537
mod print_empty_string;

‎crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB142_FURB142.py.snap

+161
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
---
2+
source: crates/ruff_linter/src/rules/refurb/mod.rs
3+
---
4+
FURB142.py:5:1: FURB142 [*] Use of manual set update
5+
|
6+
3 | s = set()
7+
4 |
8+
5 | / for x in [1, 2, 3]:
9+
6 | | s.add(x)
10+
| |____________^ FURB142
11+
7 |
12+
8 | for x in {1, 2, 3}:
13+
|
14+
= help: Replace with `update`
15+
16+
ℹ Safe fix
17+
2 2 |
18+
3 3 | s = set()
19+
4 4 |
20+
5 |-for x in [1, 2, 3]:
21+
6 |- s.add(x)
22+
5 |+s.update([1, 2, 3])
23+
7 6 |
24+
8 7 | for x in {1, 2, 3}:
25+
9 8 | s.add(x)
26+
27+
FURB142.py:8:1: FURB142 [*] Use of manual set update
28+
|
29+
6 | s.add(x)
30+
7 |
31+
8 | / for x in {1, 2, 3}:
32+
9 | | s.add(x)
33+
| |____________^ FURB142
34+
10 |
35+
11 | for x in (1, 2, 3):
36+
|
37+
= help: Replace with `update`
38+
39+
ℹ Safe fix
40+
5 5 | for x in [1, 2, 3]:
41+
6 6 | s.add(x)
42+
7 7 |
43+
8 |-for x in {1, 2, 3}:
44+
9 |- s.add(x)
45+
8 |+s.update({1, 2, 3})
46+
10 9 |
47+
11 10 | for x in (1, 2, 3):
48+
12 11 | s.add(x)
49+
50+
FURB142.py:11:1: FURB142 [*] Use of manual set update
51+
|
52+
9 | s.add(x)
53+
10 |
54+
11 | / for x in (1, 2, 3):
55+
12 | | s.add(x)
56+
| |____________^ FURB142
57+
13 |
58+
14 | for x in (1, 2, 3):
59+
|
60+
= help: Replace with `update`
61+
62+
ℹ Safe fix
63+
8 8 | for x in {1, 2, 3}:
64+
9 9 | s.add(x)
65+
10 10 |
66+
11 |-for x in (1, 2, 3):
67+
12 |- s.add(x)
68+
11 |+s.update((1, 2, 3))
69+
13 12 |
70+
14 13 | for x in (1, 2, 3):
71+
15 14 | s.discard(x)
72+
73+
FURB142.py:14:1: FURB142 [*] Use of manual set update
74+
|
75+
12 | s.add(x)
76+
13 |
77+
14 | / for x in (1, 2, 3):
78+
15 | | s.discard(x)
79+
| |________________^ FURB142
80+
16 |
81+
17 | for x in (1, 2, 3):
82+
|
83+
= help: Replace with `difference_update`
84+
85+
ℹ Safe fix
86+
11 11 | for x in (1, 2, 3):
87+
12 12 | s.add(x)
88+
13 13 |
89+
14 |-for x in (1, 2, 3):
90+
15 |- s.discard(x)
91+
14 |+s.difference_update((1, 2, 3))
92+
16 15 |
93+
17 16 | for x in (1, 2, 3):
94+
18 17 | s.add(x + 1)
95+
96+
FURB142.py:17:1: FURB142 [*] Use of manual set update
97+
|
98+
15 | s.discard(x)
99+
16 |
100+
17 | / for x in (1, 2, 3):
101+
18 | | s.add(x + 1)
102+
| |________________^ FURB142
103+
19 |
104+
20 | for x, y in ((1, 2), (3, 4)):
105+
|
106+
= help: Replace with `update`
107+
108+
ℹ Safe fix
109+
14 14 | for x in (1, 2, 3):
110+
15 15 | s.discard(x)
111+
16 16 |
112+
17 |-for x in (1, 2, 3):
113+
18 |- s.add(x + 1)
114+
17 |+s.update(x + 1 for x in (1, 2, 3))
115+
19 18 |
116+
20 19 | for x, y in ((1, 2), (3, 4)):
117+
21 20 | s.add((x, y))
118+
119+
FURB142.py:20:1: FURB142 [*] Use of manual set update
120+
|
121+
18 | s.add(x + 1)
122+
19 |
123+
20 | / for x, y in ((1, 2), (3, 4)):
124+
21 | | s.add((x, y))
125+
| |_________________^ FURB142
126+
22 |
127+
23 | num = 123
128+
|
129+
= help: Replace with `update`
130+
131+
ℹ Safe fix
132+
17 17 | for x in (1, 2, 3):
133+
18 18 | s.add(x + 1)
134+
19 19 |
135+
20 |-for x, y in ((1, 2), (3, 4)):
136+
21 |- s.add((x, y))
137+
20 |+s.update((x, y) for x, y in ((1, 2), (3, 4)))
138+
22 21 |
139+
23 22 | num = 123
140+
24 23 |
141+
142+
FURB142.py:25:1: FURB142 [*] Use of manual set update
143+
|
144+
23 | num = 123
145+
24 |
146+
25 | / for x in (1, 2, 3):
147+
26 | | s.add(num)
148+
| |______________^ FURB142
149+
|
150+
= help: Replace with `update`
151+
152+
ℹ Safe fix
153+
22 22 |
154+
23 23 | num = 123
155+
24 24 |
156+
25 |-for x in (1, 2, 3):
157+
26 |- s.add(num)
158+
25 |+s.update(num for x in (1, 2, 3))
159+
27 26 |
160+
28 27 |
161+
29 28 | # False negative

‎ruff.schema.json

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)