diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF070.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF070.py new file mode 100644 index 0000000000000..cfd134283aa77 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF070.py @@ -0,0 +1,195 @@ +### +# Errors +### + +def foo(): + x = 1 + yield x # RUF070 + +def foo(): + x = [1, 2, 3] + yield from x # RUF070 + +def foo(): + for i in range(10): + x = i * 2 + yield x # RUF070 + +def foo(): + if True: + x = 1 + yield x # RUF070 + +def foo(): + with open("foo.txt") as f: + x = f.read() + yield x # RUF070 + +def foo(): + try: + x = something() + yield x # RUF070 + except Exception: + pass + +def foo(): + x = some_function() + yield x # RUF070 + +def foo(): + x = some_generator() + yield from x # RUF070 + +def foo(): + x = lambda: 1 + yield x # RUF070 + +def foo(): + x = (y := 1) + yield x # RUF070 + +def foo(): + x =1 + yield x # RUF070 (no space after `=`) + +def foo(): + x = yield 1 + yield x # RUF070 (yield as assigned value, fix adds parentheses) + +# Assignment inside `with`, yield outside +def foo(): + with open("foo.txt") as f: + x = f.read() + yield x # RUF070 + + +### +# Non-errors +### + +# Variable used after yield +def foo(): + x = 1 + yield x + print(x) + +# Variable used before yield (two references) +def foo(): + x = 1 + print(x) + yield x + +# Multiple yields of same var +def foo(): + x = 1 + yield x + yield x + +# Annotated variable +def foo(): + x: int + x = 1 + yield x + +# Nonlocal variable +def foo(): + x = 0 + def bar(): + nonlocal x + x = 1 + yield x + +# Global variable +def foo(): + global x + x = 1 + yield x + +# Intervening statement between assign and yield +def foo(): + x = 1 + print("hello") + yield x + +# Augmented assignment +def foo(): + x = 1 + x += 1 + yield x + +# Unpacking assignment +def foo(): + x, y = 1, 2 + yield x + +# Non-name target (attribute) +def foo(): + self.x = 1 + yield self.x + +# Yield with no value +def foo(): + x = 1 + yield + +# Multiple assignment targets (e.g. `x = y = 1`) +def foo(): + x = y = 1 + yield x + +# Different variable names +def foo(): + x = 1 + yield y + +# Cross-scope reference (closure) +def foo(): + x = 1 + def inner(): + print(x) + yield x + +# Cross-scope reference (comprehension) +def foo(): + x = 1 + _ = [i for i in x] + yield x + +# Yield from with non-name value +def foo(): + yield from [1, 2, 3] + +# Yield non-name value +def foo(): + yield 1 + +# Variable used in nested function after yield +def foo(): + x = compute() + yield x + def bar(): + return x + +# Yield non-name value preceded by assignment +def foo(): + x = 1 + yield x + 1 + +# Yield from non-name value preceded by assignment +def foo(): + x = [1, 2, 3] + yield from [1, 2, 3] + +# Annotated assignment with value (not a plain assignment) +def foo(): + x: int = 1 + yield x + +# Assignment inside `with` using `contextlib.suppress` (body may not execute) +import contextlib + +def foo(): + x = default() + with contextlib.suppress(Exception): + x = something() + yield x diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 8023ed32e4386..398e0f2692187 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -26,6 +26,7 @@ pub(crate) fn bindings(checker: &Checker) { Rule::CustomTypeVarForSelf, Rule::PrivateTypeParameter, Rule::UnnecessaryAssign, + Rule::UnnecessaryAssignBeforeYield, ]) { return; } @@ -39,6 +40,14 @@ pub(crate) fn bindings(checker: &Checker) { ); } } + if checker.is_rule_enabled(Rule::UnnecessaryAssignBeforeYield) { + if binding.kind.is_function_definition() { + ruff::rules::unnecessary_assign_before_yield( + checker, + binding.statement(checker.semantic()).unwrap(), + ); + } + } if checker.is_rule_enabled(Rule::UnusedVariable) { if binding.kind.is_bound_exception() && binding.is_unused() diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 25096b73aaf51..edc7ea16c8ef5 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1063,6 +1063,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "067") => rules::ruff::rules::NonEmptyInitModule, (Ruff, "068") => rules::ruff::rules::DuplicateEntryInDunderAll, (Ruff, "069") => rules::ruff::rules::FloatEqualityComparison, + (Ruff, "070") => rules::ruff::rules::UnnecessaryAssignBeforeYield, (Ruff, "100") => rules::ruff::rules::UnusedNOQA, (Ruff, "101") => rules::ruff::rules::RedirectedNOQA, diff --git a/crates/ruff_linter/src/rules/flake8_return/mod.rs b/crates/ruff_linter/src/rules/flake8_return/mod.rs index 871e3af3c2690..f8370dd8ccbec 100644 --- a/crates/ruff_linter/src/rules/flake8_return/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_return/mod.rs @@ -4,6 +4,8 @@ mod helpers; pub(crate) mod rules; mod visitor; +pub(crate) use visitor::has_conditional_body; + #[cfg(test)] mod tests { use std::path::Path; diff --git a/crates/ruff_linter/src/rules/flake8_return/visitor.rs b/crates/ruff_linter/src/rules/flake8_return/visitor.rs index e939209f53665..d06894c6dfe0a 100644 --- a/crates/ruff_linter/src/rules/flake8_return/visitor.rs +++ b/crates/ruff_linter/src/rules/flake8_return/visitor.rs @@ -207,7 +207,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'_, 'a> { /// data = data.decode() /// return data /// ``` -fn has_conditional_body(with: &ast::StmtWith, semantic: &SemanticModel) -> bool { +pub(crate) fn has_conditional_body(with: &ast::StmtWith, semantic: &SemanticModel) -> bool { with.items.iter().any(|item| { let ast::WithItem { context_expr: Expr::Call(ast::ExprCall { func, .. }), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index bd346213d9093..f5afe8dbbbbfb 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -613,6 +613,7 @@ mod tests { #[test_case(Rule::IndentedFormFeed, Path::new("RUF054.py"))] #[test_case(Rule::ImplicitClassVarInDataclass, Path::new("RUF045.py"))] #[test_case(Rule::FloatEqualityComparison, Path::new("RUF069.py"))] + #[test_case(Rule::UnnecessaryAssignBeforeYield, Path::new("RUF070.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index e0640d1b225e5..06cde1ce691ef 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -51,6 +51,7 @@ pub(crate) use static_key_dict_comprehension::*; #[cfg(any(feature = "test-rules", test))] pub(crate) use test_rules::*; pub(crate) use unmatched_suppression_comment::*; +pub(crate) use unnecessary_assign_before_yield::*; pub(crate) use unnecessary_cast_to_int::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; @@ -123,6 +124,7 @@ mod suppression_comment_visitor; #[cfg(any(feature = "test-rules", test))] pub(crate) mod test_rules; mod unmatched_suppression_comment; +mod unnecessary_assign_before_yield; mod unnecessary_cast_to_int; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_assign_before_yield.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_assign_before_yield.rs new file mode 100644 index 0000000000000..b968d6fafcad4 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_assign_before_yield.rs @@ -0,0 +1,237 @@ +use anyhow::Context; +use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast::token::TokenKind; +use ruff_python_ast::visitor; +use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{self as ast, Expr, Identifier, Stmt}; +use ruff_python_semantic::SemanticModel; +use ruff_text_size::{Ranged, TextRange}; +use rustc_hash::FxHashSet; + +use crate::checkers::ast::Checker; +use crate::fix::edits; +use crate::rules::flake8_return::has_conditional_body; +use crate::{AlwaysFixableViolation, Edit, Fix}; + +/// ## What it does +/// Checks for variable assignments that immediately precede a `yield` (or +/// `yield from`) of the assigned variable, where the variable is not +/// referenced anywhere else. +/// +/// ## Why is this bad? +/// The variable assignment is not necessary, as the value can be yielded +/// directly. +/// +/// ## Example +/// ```python +/// def gen(): +/// x = 1 +/// yield x +/// ``` +/// +/// Use instead: +/// ```python +/// def gen(): +/// yield 1 +/// ``` +/// +/// ## Fix safety +/// This fix is always marked as unsafe because removing the intermediate +/// variable assignment changes the local variable bindings visible to +/// `locals()` and debuggers when the generator is suspended at the `yield`. +#[derive(ViolationMetadata)] +#[violation_metadata(preview_since = "NEXT_RUFF_VERSION")] +pub(crate) struct UnnecessaryAssignBeforeYield { + name: String, + is_yield_from: bool, +} + +impl AlwaysFixableViolation for UnnecessaryAssignBeforeYield { + #[derive_message_formats] + fn message(&self) -> String { + let UnnecessaryAssignBeforeYield { + name, + is_yield_from, + } = self; + if *is_yield_from { + format!("Unnecessary assignment to `{name}` before `yield from` statement") + } else { + format!("Unnecessary assignment to `{name}` before `yield` statement") + } + } + + fn fix_title(&self) -> String { + "Remove unnecessary assignment".to_string() + } +} + +/// RUF070 +pub(crate) fn unnecessary_assign_before_yield(checker: &Checker, function_stmt: &Stmt) { + let Stmt::FunctionDef(function_def) = function_stmt else { + return; + }; + + let Some(function_scope) = checker.semantic().function_scope(function_def) else { + return; + }; + + let visitor = { + let mut visitor = YieldVisitor::new(checker.semantic()); + visitor.visit_body(&function_def.body); + visitor + }; + + for (assign, yield_expr, stmt) in &visitor.assignment_yield { + let (value, is_yield_from) = match yield_expr { + Expr::Yield(ast::ExprYield { + value: Some(value), .. + }) => (value.as_ref(), false), + Expr::YieldFrom(ast::ExprYieldFrom { value, .. }) => (value.as_ref(), true), + _ => continue, + }; + + let Expr::Name(ast::ExprName { id: yielded_id, .. }) = value else { + continue; + }; + + if let [Expr::Name(ast::ExprName { + id: assigned_id, .. + })] = assign.targets.as_slice() + && yielded_id == assigned_id + && !visitor.annotations.contains(assigned_id.as_str()) + && !visitor.non_locals.contains(assigned_id.as_str()) + && let Some(assigned_binding) = function_scope + .get(assigned_id) + .map(|binding_id| checker.semantic().binding(binding_id)) + // Unlike `return`, `yield` doesn't exit the function, so the variable could be + // referenced elsewhere. Only flag if the binding has exactly one reference (the + // yield itself). + && assigned_binding.references().count() == 1 + && assigned_binding + .references() + .map(|reference_id| checker.semantic().reference(reference_id)) + .all(|reference| reference.scope_id() == assigned_binding.scope) + { + checker + .report_diagnostic( + UnnecessaryAssignBeforeYield { + name: assigned_id.to_string(), + is_yield_from, + }, + value.range(), + ) + .try_set_fix(|| { + let delete_yield = + edits::delete_stmt(stmt, None, checker.locator(), checker.indexer()); + + let eq_token = checker + .tokens() + .before(assign.value.start()) + .iter() + .rfind(|token| token.kind() == TokenKind::Equal) + .context("Expected an equals token")?; + + let keyword = if is_yield_from { "yield from" } else { "yield" }; + let needs_parens = + matches!(assign.value.as_ref(), Expr::Yield(_) | Expr::YieldFrom(_)); + + let replace_assign = Edit::range_replacement( + if eq_token.end() < assign.value.start() { + keyword.to_string() + } else { + format!("{keyword} ") + }, + TextRange::new(assign.start(), eq_token.range().end()), + ); + + let mut edits = vec![replace_assign, delete_yield]; + if needs_parens { + edits.push(Edit::insertion("(".to_string(), assign.value.start())); + edits.push(Edit::insertion(")".to_string(), assign.value.end())); + } + + Ok(Fix::unsafe_edits(edits.remove(0), edits)) + }); + } + } +} + +struct YieldVisitor<'semantic, 'a> { + /// The semantic model of the current file. + semantic: &'semantic SemanticModel<'a>, + /// The non-local variables in the current function. + non_locals: FxHashSet<&'a str>, + /// The annotated variables in the current function. + annotations: FxHashSet<&'a str>, + /// The `assignment`-to-`yield` statement pairs in the current function. + assignment_yield: Vec<(&'a ast::StmtAssign, &'a Expr, &'a Stmt)>, + /// The preceding sibling of the current node. + sibling: Option<&'a Stmt>, +} + +impl<'semantic, 'a> YieldVisitor<'semantic, 'a> { + fn new(semantic: &'semantic SemanticModel<'a>) -> Self { + Self { + semantic, + non_locals: FxHashSet::default(), + annotations: FxHashSet::default(), + assignment_yield: Vec::new(), + sibling: None, + } + } +} + +impl<'a> Visitor<'a> for YieldVisitor<'_, 'a> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + match stmt { + Stmt::ClassDef(_) | Stmt::FunctionDef(_) => { + // Do not recurse into nested class/function bodies. + self.sibling = Some(stmt); + return; + } + Stmt::Global(ast::StmtGlobal { names, .. }) + | Stmt::Nonlocal(ast::StmtNonlocal { names, .. }) => { + self.non_locals.extend(names.iter().map(Identifier::as_str)); + } + Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { + // Ex) `x: int` + if value.is_none() + && let Expr::Name(name) = target.as_ref() + { + self.annotations.insert(name.id.as_str()); + } + } + Stmt::Expr(ast::StmtExpr { value, .. }) => { + if matches!(value.as_ref(), Expr::Yield(_) | Expr::YieldFrom(_)) { + match self.sibling { + Some(Stmt::Assign(stmt_assign)) => { + self.assignment_yield + .push((stmt_assign, value.as_ref(), stmt)); + } + Some(Stmt::With(with)) => { + if let Some(stmt_assign) = + with.body.last().and_then(Stmt::as_assign_stmt) + && !has_conditional_body(with, self.semantic) + { + self.assignment_yield + .push((stmt_assign, value.as_ref(), stmt)); + } + } + _ => {} + } + } + } + _ => {} + } + + self.sibling = Some(stmt); + visitor::walk_stmt(self, stmt); + } + + fn visit_body(&mut self, body: &'a [Stmt]) { + let sibling = self.sibling; + self.sibling = None; + visitor::walk_body(self, body); + self.sibling = sibling; + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF070_RUF070.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF070_RUF070.py.snap new file mode 100644 index 0000000000000..0ff182c8e69bd --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF070_RUF070.py.snap @@ -0,0 +1,286 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:7:11 + | +5 | def foo(): +6 | x = 1 +7 | yield x # RUF070 + | ^ +8 | +9 | def foo(): + | +help: Remove unnecessary assignment +3 | ### +4 | +5 | def foo(): + - x = 1 + - yield x # RUF070 +6 + yield 1 +7 | +8 | def foo(): +9 | x = [1, 2, 3] +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield from` statement + --> RUF070.py:11:16 + | + 9 | def foo(): +10 | x = [1, 2, 3] +11 | yield from x # RUF070 + | ^ +12 | +13 | def foo(): + | +help: Remove unnecessary assignment +7 | yield x # RUF070 +8 | +9 | def foo(): + - x = [1, 2, 3] + - yield from x # RUF070 +10 + yield from [1, 2, 3] +11 | +12 | def foo(): +13 | for i in range(10): +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:16:15 + | +14 | for i in range(10): +15 | x = i * 2 +16 | yield x # RUF070 + | ^ +17 | +18 | def foo(): + | +help: Remove unnecessary assignment +12 | +13 | def foo(): +14 | for i in range(10): + - x = i * 2 + - yield x # RUF070 +15 + yield i * 2 +16 | +17 | def foo(): +18 | if True: +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:21:15 + | +19 | if True: +20 | x = 1 +21 | yield x # RUF070 + | ^ +22 | +23 | def foo(): + | +help: Remove unnecessary assignment +17 | +18 | def foo(): +19 | if True: + - x = 1 + - yield x # RUF070 +20 + yield 1 +21 | +22 | def foo(): +23 | with open("foo.txt") as f: +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:26:15 + | +24 | with open("foo.txt") as f: +25 | x = f.read() +26 | yield x # RUF070 + | ^ +27 | +28 | def foo(): + | +help: Remove unnecessary assignment +22 | +23 | def foo(): +24 | with open("foo.txt") as f: + - x = f.read() + - yield x # RUF070 +25 + yield f.read() +26 | +27 | def foo(): +28 | try: +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:31:15 + | +29 | try: +30 | x = something() +31 | yield x # RUF070 + | ^ +32 | except Exception: +33 | pass + | +help: Remove unnecessary assignment +27 | +28 | def foo(): +29 | try: + - x = something() + - yield x # RUF070 +30 + yield something() +31 | except Exception: +32 | pass +33 | +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:37:11 + | +35 | def foo(): +36 | x = some_function() +37 | yield x # RUF070 + | ^ +38 | +39 | def foo(): + | +help: Remove unnecessary assignment +33 | pass +34 | +35 | def foo(): + - x = some_function() + - yield x # RUF070 +36 + yield some_function() +37 | +38 | def foo(): +39 | x = some_generator() +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield from` statement + --> RUF070.py:41:16 + | +39 | def foo(): +40 | x = some_generator() +41 | yield from x # RUF070 + | ^ +42 | +43 | def foo(): + | +help: Remove unnecessary assignment +37 | yield x # RUF070 +38 | +39 | def foo(): + - x = some_generator() + - yield from x # RUF070 +40 + yield from some_generator() +41 | +42 | def foo(): +43 | x = lambda: 1 +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:45:11 + | +43 | def foo(): +44 | x = lambda: 1 +45 | yield x # RUF070 + | ^ +46 | +47 | def foo(): + | +help: Remove unnecessary assignment +41 | yield from x # RUF070 +42 | +43 | def foo(): + - x = lambda: 1 + - yield x # RUF070 +44 + yield lambda: 1 +45 | +46 | def foo(): +47 | x = (y := 1) +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:49:11 + | +47 | def foo(): +48 | x = (y := 1) +49 | yield x # RUF070 + | ^ +50 | +51 | def foo(): + | +help: Remove unnecessary assignment +45 | yield x # RUF070 +46 | +47 | def foo(): + - x = (y := 1) + - yield x # RUF070 +48 + yield (y := 1) +49 | +50 | def foo(): +51 | x =1 +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:53:11 + | +51 | def foo(): +52 | x =1 +53 | yield x # RUF070 (no space after `=`) + | ^ +54 | +55 | def foo(): + | +help: Remove unnecessary assignment +49 | yield x # RUF070 +50 | +51 | def foo(): + - x =1 + - yield x # RUF070 (no space after `=`) +52 + yield 1 +53 | +54 | def foo(): +55 | x = yield 1 +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:57:11 + | +55 | def foo(): +56 | x = yield 1 +57 | yield x # RUF070 (yield as assigned value, fix adds parentheses) + | ^ +58 | +59 | # Assignment inside `with`, yield outside + | +help: Remove unnecessary assignment +53 | yield x # RUF070 (no space after `=`) +54 | +55 | def foo(): + - x = yield 1 + - yield x # RUF070 (yield as assigned value, fix adds parentheses) +56 + yield (yield 1) +57 | +58 | # Assignment inside `with`, yield outside +59 | def foo(): +note: This is an unsafe fix and may change runtime behavior + +RUF070 [*] Unnecessary assignment to `x` before `yield` statement + --> RUF070.py:63:11 + | +61 | with open("foo.txt") as f: +62 | x = f.read() +63 | yield x # RUF070 + | ^ + | +help: Remove unnecessary assignment +59 | # Assignment inside `with`, yield outside +60 | def foo(): +61 | with open("foo.txt") as f: + - x = f.read() + - yield x # RUF070 +62 + yield f.read() +63 | +64 | +65 | ### +note: This is an unsafe fix and may change runtime behavior diff --git a/ruff.schema.json b/ruff.schema.json index 1de42c71a9ba7..7c085f04e76da 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4119,6 +4119,8 @@ "RUF067", "RUF068", "RUF069", + "RUF07", + "RUF070", "RUF1", "RUF10", "RUF100",