Skip to content

Commit f3bf008

Browse files
charliermarshcharliermarsh
authored
Avoid removing statements that contain side-effects (#1920)
Closes #1917.
1 parent 3b4aaa5 commit f3bf008

File tree

6 files changed

+114
-76
lines changed

6 files changed

+114
-76
lines changed

‎resources/test/fixtures/pyflakes/F841_3.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,17 @@ def f():
4646
pass
4747

4848

49+
def f(a, b):
50+
x = (
51+
a()
52+
if a is not None
53+
else b
54+
)
55+
56+
y = \
57+
a() if a is not None else b
58+
59+
4960
def f(a, b):
5061
x = (
5162
a

‎src/ast/helpers.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,45 @@ pub fn contains_call_path(checker: &Checker, expr: &Expr, target: &[&str]) -> bo
9494
})
9595
}
9696

97+
/// Return `true` if the `Expr` contains an expression that appears to include a
98+
/// side-effect (like a function call).
99+
pub fn contains_effect(checker: &Checker, expr: &Expr) -> bool {
100+
any_over_expr(expr, &|expr| {
101+
// Accept empty initializers.
102+
if let ExprKind::Call {
103+
func,
104+
args,
105+
keywords,
106+
} = &expr.node
107+
{
108+
if args.is_empty() && keywords.is_empty() {
109+
if let ExprKind::Name { id, .. } = &func.node {
110+
let is_empty_initializer = (id == "set"
111+
|| id == "list"
112+
|| id == "tuple"
113+
|| id == "dict"
114+
|| id == "frozenset")
115+
&& checker.is_builtin(id);
116+
return !is_empty_initializer;
117+
}
118+
}
119+
}
120+
121+
// Otherwise, avoid all complex expressions.
122+
matches!(
123+
expr.node,
124+
ExprKind::Call { .. }
125+
| ExprKind::Await { .. }
126+
| ExprKind::GeneratorExp { .. }
127+
| ExprKind::ListComp { .. }
128+
| ExprKind::SetComp { .. }
129+
| ExprKind::DictComp { .. }
130+
| ExprKind::Yield { .. }
131+
| ExprKind::YieldFrom { .. }
132+
)
133+
})
134+
}
135+
97136
/// Call `func` over every `Expr` in `expr`, returning `true` if any expression
98137
/// returns `true`..
99138
pub fn any_over_expr<F>(expr: &Expr, func: &F) -> bool

‎src/rules/flake8_simplify/rules/ast_if.rs‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use rustpython_ast::{Cmpop, Constant, Expr, ExprContext, ExprKind, Stmt, StmtKin
22

33
use crate::ast::comparable::ComparableExpr;
44
use crate::ast::helpers::{
5-
any_over_expr, contains_call_path, create_expr, create_stmt, has_comments, unparse_expr,
5+
contains_call_path, contains_effect, create_expr, create_stmt, has_comments, unparse_expr,
66
unparse_stmt,
77
};
88
use crate::ast::types::Range;
@@ -272,19 +272,7 @@ pub fn use_dict_get_with_default(
272272
}
273273

274274
// Check that the default value is not "complex".
275-
if any_over_expr(default_val, &|expr| {
276-
matches!(
277-
expr.node,
278-
ExprKind::Call { .. }
279-
| ExprKind::Await { .. }
280-
| ExprKind::GeneratorExp { .. }
281-
| ExprKind::ListComp { .. }
282-
| ExprKind::SetComp { .. }
283-
| ExprKind::DictComp { .. }
284-
| ExprKind::Yield { .. }
285-
| ExprKind::YieldFrom { .. }
286-
)
287-
}) {
275+
if contains_effect(checker, default_val) {
288276
return;
289277
}
290278

‎src/rules/pyflakes/rules/unused_variable.rs‎

Lines changed: 25 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use itertools::Itertools;
22
use log::error;
3-
use rustpython_ast::{Expr, ExprKind, Location, Stmt, StmtKind};
3+
use rustpython_ast::{ExprKind, Location, Stmt, StmtKind};
44
use rustpython_parser::lexer;
55
use rustpython_parser::lexer::Tok;
66

7+
use crate::ast::helpers::contains_effect;
78
use crate::ast::types::{BindingKind, Range, RefEquality, ScopeKind};
89
use crate::autofix::helpers::delete_stmt;
910
use crate::checkers::ast::Checker;
@@ -12,41 +13,6 @@ use crate::registry::{Diagnostic, RuleCode};
1213
use crate::source_code::Locator;
1314
use crate::violations;
1415

15-
fn is_literal_or_name(expr: &Expr, checker: &Checker) -> bool {
16-
// Accept any obvious literals or names.
17-
if matches!(
18-
expr.node,
19-
ExprKind::Constant { .. }
20-
| ExprKind::Name { .. }
21-
| ExprKind::List { .. }
22-
| ExprKind::Tuple { .. }
23-
| ExprKind::Set { .. }
24-
) {
25-
return true;
26-
}
27-
28-
// Accept empty initializers.
29-
if let ExprKind::Call {
30-
func,
31-
args,
32-
keywords,
33-
} = &expr.node
34-
{
35-
if args.is_empty() && keywords.is_empty() {
36-
if let ExprKind::Name { id, .. } = &func.node {
37-
return (id == "set"
38-
|| id == "list"
39-
|| id == "tuple"
40-
|| id == "dict"
41-
|| id == "frozenset")
42-
&& checker.is_builtin(id);
43-
}
44-
}
45-
}
46-
47-
false
48-
}
49-
5016
fn match_token_after<F>(stmt: &Stmt, locator: &Locator, f: F) -> Location
5117
where
5218
F: Fn(Tok) -> bool,
@@ -78,8 +44,18 @@ fn remove_unused_variable(
7844
// First case: simple assignment (`x = 1`)
7945
if let StmtKind::Assign { targets, value, .. } = &stmt.node {
8046
if targets.len() == 1 && matches!(targets[0].node, ExprKind::Name { .. }) {
81-
return if is_literal_or_name(value, checker) {
82-
// If assigning to a constant (`x = 1`), delete the entire statement.
47+
return if contains_effect(checker, value) {
48+
// If the expression is complex (`x = foo()`), remove the assignment,
49+
// but preserve the right-hand side.
50+
Some((
51+
DeletionKind::Partial,
52+
Fix::deletion(
53+
stmt.location,
54+
match_token_after(stmt, checker.locator, |tok| tok == Tok::Equal),
55+
),
56+
))
57+
} else {
58+
// If (e.g.) assigning to a constant (`x = 1`), delete the entire statement.
8359
let parent = checker
8460
.child_to_parent
8561
.get(&RefEquality(stmt))
@@ -98,16 +74,6 @@ fn remove_unused_variable(
9874
None
9975
}
10076
}
101-
} else {
102-
// If the expression is more complex (`x = foo()`), remove the assignment,
103-
// but preserve the right-hand side.
104-
Some((
105-
DeletionKind::Partial,
106-
Fix::deletion(
107-
stmt.location,
108-
match_token_after(stmt, checker.locator, |tok| tok == Tok::Equal),
109-
),
110-
))
11177
};
11278
}
11379
}
@@ -120,7 +86,17 @@ fn remove_unused_variable(
12086
} = &stmt.node
12187
{
12288
if matches!(target.node, ExprKind::Name { .. }) {
123-
return if is_literal_or_name(value, checker) {
89+
return if contains_effect(checker, value) {
90+
// If the expression is complex (`x = foo()`), remove the assignment,
91+
// but preserve the right-hand side.
92+
Some((
93+
DeletionKind::Partial,
94+
Fix::deletion(
95+
stmt.location,
96+
match_token_after(stmt, checker.locator, |tok| tok == Tok::Equal),
97+
),
98+
))
99+
} else {
124100
// If assigning to a constant (`x = 1`), delete the entire statement.
125101
let parent = checker
126102
.child_to_parent
@@ -140,16 +116,6 @@ fn remove_unused_variable(
140116
None
141117
}
142118
}
143-
} else {
144-
// If the expression is more complex (`x = foo()`), remove the assignment,
145-
// but preserve the right-hand side.
146-
Some((
147-
DeletionKind::Partial,
148-
Fix::deletion(
149-
stmt.location,
150-
match_token_after(stmt, checker.locator, |tok| tok == Tok::Equal),
151-
),
152-
))
153119
};
154120
}
155121
}

‎src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_0.py.snap‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ expression: diagnostics
3131
content: ""
3232
location:
3333
row: 16
34-
column: 4
34+
column: 0
3535
end_location:
36-
row: 16
37-
column: 8
36+
row: 17
37+
column: 0
3838
parent: ~
3939
- kind:
4040
UnusedVariable: foo

‎src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_3.py.snap‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,4 +246,38 @@ expression: diagnostics
246246
row: 57
247247
column: 8
248248
parent: ~
249+
- kind:
250+
UnusedVariable: x
251+
location:
252+
row: 61
253+
column: 4
254+
end_location:
255+
row: 61
256+
column: 5
257+
fix:
258+
content: pass
259+
location:
260+
row: 61
261+
column: 4
262+
end_location:
263+
row: 65
264+
column: 5
265+
parent: ~
266+
- kind:
267+
UnusedVariable: y
268+
location:
269+
row: 67
270+
column: 4
271+
end_location:
272+
row: 67
273+
column: 5
274+
fix:
275+
content: ""
276+
location:
277+
row: 67
278+
column: 0
279+
end_location:
280+
row: 69
281+
column: 0
282+
parent: ~
249283

0 commit comments

Comments
 (0)