Skip to content

Commit a3ae76e

Browse files
[pyupgrade] Do not offer fix when at least one target is global/nonlocal (UP028) (#16451)
## Summary Resolves #16445. `UP028` is now no longer always fixable: it will not offer a fix when at least one `ExprName` target is bound to either a `global` or a `nonlocal` declaration. ## Test Plan `cargo nextest run` and `cargo insta test`.
1 parent d93ed29 commit a3ae76e

File tree

5 files changed

+115
-27
lines changed

5 files changed

+115
-27
lines changed

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP028_0.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,3 +163,26 @@ def f():
163163
pass
164164
except Exception as x:
165165
pass
166+
167+
168+
# https://github.com/astral-sh/ruff/issues/15540
169+
def f():
170+
for a in 1,:
171+
yield a
172+
173+
174+
SOME_GLOBAL = None
175+
176+
def f(iterable):
177+
global SOME_GLOBAL
178+
179+
for SOME_GLOBAL in iterable:
180+
yield SOME_GLOBAL
181+
182+
some_non_local = None
183+
184+
def g():
185+
nonlocal some_non_local
186+
187+
for some_non_local in iterable:
188+
yield some_non_local

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP028_1.py

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,20 @@ def f():
123123
yield x, y, x + y
124124

125125

126-
# https://github.com/astral-sh/ruff/issues/15540
127126
def f():
128-
for a in 1,:
129-
yield a
127+
global some_global
128+
129+
for element in iterable:
130+
some_global = element
131+
yield some_global
132+
133+
134+
def f():
135+
some_nonlocal = 1
136+
137+
def g():
138+
nonlocal some_nonlocal
139+
140+
for element in iterable:
141+
some_nonlocal = element
142+
yield some_nonlocal

crates/ruff_linter/src/rules/pyupgrade/rules/yield_in_for_loop.rs

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
1+
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
22
use ruff_macros::{derive_message_formats, ViolationMetadata};
33
use ruff_python_ast::parenthesize::parenthesized_range;
44
use ruff_python_ast::{self as ast, Expr, Stmt};
@@ -17,11 +17,19 @@ use crate::checkers::ast::Checker;
1717
/// ```python
1818
/// for x in foo:
1919
/// yield x
20+
///
21+
/// global y
22+
/// for y in foo:
23+
/// yield y
2024
/// ```
2125
///
2226
/// Use instead:
2327
/// ```python
2428
/// yield from foo
29+
///
30+
/// for _element in foo:
31+
/// y = _element
32+
/// yield y
2533
/// ```
2634
///
2735
/// ## Fix safety
@@ -31,6 +39,9 @@ use crate::checkers::ast::Checker;
3139
/// to a `yield from` could lead to an attribute error if the underlying
3240
/// generator does not implement the `send` method.
3341
///
42+
/// Additionally, if at least one target is `global` or `nonlocal`,
43+
/// no fix will be offered.
44+
///
3445
/// In most cases, however, the fix is safe, and such a modification should have
3546
/// no effect on the behavior of the program.
3647
///
@@ -40,14 +51,16 @@ use crate::checkers::ast::Checker;
4051
#[derive(ViolationMetadata)]
4152
pub(crate) struct YieldInForLoop;
4253

43-
impl AlwaysFixableViolation for YieldInForLoop {
54+
impl Violation for YieldInForLoop {
55+
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
56+
4457
#[derive_message_formats]
4558
fn message(&self) -> String {
4659
"Replace `yield` over `for` loop with `yield from`".to_string()
4760
}
4861

49-
fn fix_title(&self) -> String {
50-
"Replace with `yield from`".to_string()
62+
fn fix_title(&self) -> Option<String> {
63+
Some("Replace with `yield from`".to_string())
5164
}
5265
}
5366

@@ -130,10 +143,22 @@ pub(crate) fn yield_in_for_loop(checker: &Checker, stmt_for: &ast::StmtFor) {
130143
format!("yield from {contents}")
131144
};
132145

133-
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
134-
contents,
135-
stmt_for.range(),
136-
)));
146+
if !collect_names(value).any(|name| {
147+
let semantic = checker.semantic();
148+
let mut bindings = semantic.current_scope().get_all(name.id.as_str());
149+
150+
bindings.any(|id| {
151+
let binding = semantic.binding(id);
152+
153+
binding.is_global() || binding.is_nonlocal()
154+
})
155+
}) {
156+
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
157+
contents,
158+
stmt_for.range(),
159+
)));
160+
}
161+
137162
checker.report_diagnostic(diagnostic);
138163
}
139164

crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP028_0.py.snap

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,3 +380,46 @@ UP028_0.py:134:5: UP028 [*] Replace `yield` over `for` loop with `yield from`
380380
136 135 | # Shadowing with multiple `except` blocks
381381
137 136 | try:
382382
138 137 | pass
383+
384+
UP028_0.py:170:5: UP028 [*] Replace `yield` over `for` loop with `yield from`
385+
|
386+
168 | # https://github.com/astral-sh/ruff/issues/15540
387+
169 | def f():
388+
170 | / for a in 1,:
389+
171 | | yield a
390+
| |_______________^ UP028
391+
|
392+
= help: Replace with `yield from`
393+
394+
Unsafe fix
395+
167 167 |
396+
168 168 | # https://github.com/astral-sh/ruff/issues/15540
397+
169 169 | def f():
398+
170 |- for a in 1,:
399+
171 |- yield a
400+
170 |+ yield from (1,)
401+
172 171 |
402+
173 172 |
403+
174 173 | SOME_GLOBAL = None
404+
405+
UP028_0.py:179:5: UP028 Replace `yield` over `for` loop with `yield from`
406+
|
407+
177 | global SOME_GLOBAL
408+
178 |
409+
179 | / for SOME_GLOBAL in iterable:
410+
180 | | yield SOME_GLOBAL
411+
| |_________________________^ UP028
412+
181 |
413+
182 | some_non_local = None
414+
|
415+
= help: Replace with `yield from`
416+
417+
UP028_0.py:187:9: UP028 Replace `yield` over `for` loop with `yield from`
418+
|
419+
185 | nonlocal some_non_local
420+
186 |
421+
187 | / for some_non_local in iterable:
422+
188 | | yield some_non_local
423+
| |________________________________^ UP028
424+
|
425+
= help: Replace with `yield from`
Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,4 @@
11
---
22
source: crates/ruff_linter/src/rules/pyupgrade/mod.rs
33
---
4-
UP028_1.py:128:5: UP028 [*] Replace `yield` over `for` loop with `yield from`
5-
|
6-
126 | # https://github.com/astral-sh/ruff/issues/15540
7-
127 | def f():
8-
128 | / for a in 1,:
9-
129 | | yield a
10-
| |_______________^ UP028
11-
|
12-
= help: Replace with `yield from`
134

14-
Unsafe fix
15-
125 125 |
16-
126 126 | # https://github.com/astral-sh/ruff/issues/15540
17-
127 127 | def f():
18-
128 |- for a in 1,:
19-
129 |- yield a
20-
128 |+ yield from (1,)

0 commit comments

Comments
 (0)