From 28aed61a22dcd13068530486c9af74f9727b2a43 Mon Sep 17 00:00:00 2001 From: wangxiaolei Date: Fri, 24 Oct 2025 06:02:41 +0800 Subject: [PATCH] [`pylint`] Implement `stop-iteration-return` (`PLR1708`) (#20733) ## Summary implement pylint rule stop-iteration-return / R1708 ## Test Plan --------- Co-authored-by: Brent Westbrook --- .../fixtures/pylint/stop_iteration_return.py | 131 ++++++++++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../pylint/rules/stop_iteration_return.rs | 114 +++++++++++++++ ...sts__PLR1708_stop_iteration_return.py.snap | 109 +++++++++++++++ ruff.schema.json | 1 + 8 files changed, 362 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/stop_iteration_return.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1708_stop_iteration_return.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/stop_iteration_return.py b/crates/ruff_linter/resources/test/fixtures/pylint/stop_iteration_return.py new file mode 100644 index 0000000000..a35e31a21e --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/stop_iteration_return.py @@ -0,0 +1,131 @@ +"""Test cases for PLR1708 stop-iteration-return.""" + + +# Valid cases - should not trigger the rule +def normal_function(): + raise StopIteration # Not a generator, should not trigger + + +def normal_function_with_value(): + raise StopIteration("value") # Not a generator, should not trigger + + +def generator_with_return(): + yield 1 + yield 2 + return "finished" # This is the correct way + + +def generator_with_yield_from(): + yield from [1, 2, 3] + + +def generator_without_stop_iteration(): + yield 1 + yield 2 + # No explicit termination + + +def generator_with_other_exception(): + yield 1 + raise ValueError("something else") # Different exception + + +# Invalid cases - should trigger the rule +def generator_with_stop_iteration(): + yield 1 + yield 2 + raise StopIteration # Should trigger + + +def generator_with_stop_iteration_value(): + yield 1 + yield 2 + raise StopIteration("finished") # Should trigger + + +def generator_with_stop_iteration_expr(): + yield 1 + yield 2 + raise StopIteration(1 + 2) # Should trigger + + +def async_generator_with_stop_iteration(): + yield 1 + yield 2 + raise StopIteration("async") # Should trigger + + +def nested_generator(): + def inner_gen(): + yield 1 + raise StopIteration("inner") # Should trigger + + yield from inner_gen() + + +def generator_in_class(): + class MyClass: + def generator_method(self): + yield 1 + raise StopIteration("method") # Should trigger + + return MyClass + + +# Complex cases +def complex_generator(): + try: + yield 1 + yield 2 + raise StopIteration("complex") # Should trigger + except ValueError: + yield 3 + finally: + pass + + +def generator_with_conditional_stop_iteration(condition): + yield 1 + if condition: + raise StopIteration("conditional") # Should trigger + yield 2 + + +# Edge cases +def generator_with_bare_stop_iteration(): + yield 1 + raise StopIteration # Should trigger (no arguments) + + +def generator_with_stop_iteration_in_loop(): + for i in range(5): + yield i + if i == 3: + raise StopIteration("loop") # Should trigger + + +# Should not trigger - different exceptions +def generator_with_runtime_error(): + yield 1 + raise RuntimeError("not StopIteration") # Should not trigger + + +def generator_with_custom_exception(): + yield 1 + raise CustomException("custom") # Should not trigger + + +class CustomException(Exception): + pass + + +# Generator comprehensions should not be affected +list_comp = [x for x in range(10)] # Should not trigger + + +# Lambda in generator context +def generator_with_lambda(): + yield 1 + func = lambda x: x # Just a regular lambda + yield 2 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index cded9e44e6..7c0037e10d 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -951,6 +951,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.is_rule_enabled(Rule::MisplacedBareRaise) { pylint::rules::misplaced_bare_raise(checker, raise); } + if checker.is_rule_enabled(Rule::StopIterationReturn) { + pylint::rules::stop_iteration_return(checker, raise); + } } Stmt::AugAssign(aug_assign @ ast::StmtAugAssign { target, .. }) => { if checker.is_rule_enabled(Rule::GlobalStatement) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index eab00b66d8..172841dc7c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -286,6 +286,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1702") => rules::pylint::rules::TooManyNestedBlocks, (Pylint, "R1704") => rules::pylint::rules::RedefinedArgumentFromLocal, (Pylint, "R1706") => rules::pylint::rules::AndOrTernary, + (Pylint, "R1708") => rules::pylint::rules::StopIterationReturn, (Pylint, "R1711") => rules::pylint::rules::UselessReturn, (Pylint, "R1714") => rules::pylint::rules::RepeatedEqualityComparison, (Pylint, "R1722") => rules::pylint::rules::SysExitAlias, diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 7813ffd1c6..a0ab9a908e 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -52,6 +52,7 @@ mod tests { #[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))] #[test_case(Rule::IfStmtMinMax, Path::new("if_stmt_min_max.py"))] #[test_case(Rule::SingleStringSlots, Path::new("single_string_slots.py"))] + #[test_case(Rule::StopIterationReturn, Path::new("stop_iteration_return.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_0.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_1.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_2.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 891691f21b..2853bb84c9 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -75,6 +75,7 @@ pub(crate) use shallow_copy_environ::*; pub(crate) use single_string_slots::*; pub(crate) use singledispatch_method::*; pub(crate) use singledispatchmethod_function::*; +pub(crate) use stop_iteration_return::*; pub(crate) use subprocess_popen_preexec_fn::*; pub(crate) use subprocess_run_without_check::*; pub(crate) use super_without_brackets::*; @@ -185,6 +186,7 @@ mod shallow_copy_environ; mod single_string_slots; mod singledispatch_method; mod singledispatchmethod_function; +mod stop_iteration_return; mod subprocess_popen_preexec_fn; mod subprocess_run_without_check; mod super_without_brackets; diff --git a/crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs b/crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs new file mode 100644 index 0000000000..2abc339592 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/stop_iteration_return.rs @@ -0,0 +1,114 @@ +use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast as ast; +use ruff_python_ast::visitor::{Visitor, walk_expr, walk_stmt}; +use ruff_text_size::Ranged; + +use crate::Violation; +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for explicit `raise StopIteration` in generator functions. +/// +/// ## Why is this bad? +/// Raising `StopIteration` in a generator function causes a `RuntimeError` +/// when the generator is iterated over. +/// +/// Instead of `raise StopIteration`, use `return` in generator functions. +/// +/// ## Example +/// ```python +/// def my_generator(): +/// yield 1 +/// yield 2 +/// raise StopIteration # This causes RuntimeError at runtime +/// ``` +/// +/// Use instead: +/// ```python +/// def my_generator(): +/// yield 1 +/// yield 2 +/// return # Use return instead +/// ``` +/// +/// ## References +/// - [PEP 479](https://peps.python.org/pep-0479/) +/// - [Python documentation](https://docs.python.org/3/library/exceptions.html#StopIteration) +#[derive(ViolationMetadata)] +#[violation_metadata(preview_since = "0.14.3")] +pub(crate) struct StopIterationReturn; + +impl Violation for StopIterationReturn { + #[derive_message_formats] + fn message(&self) -> String { + "Explicit `raise StopIteration` in generator".to_string() + } + + fn fix_title(&self) -> Option { + Some("Use `return` instead".to_string()) + } +} + +/// PLR1708 +pub(crate) fn stop_iteration_return(checker: &Checker, raise_stmt: &ast::StmtRaise) { + // Fast-path: only continue if this is `raise StopIteration` (with or without args) + let Some(exc) = &raise_stmt.exc else { + return; + }; + + let is_stop_iteration = match exc.as_ref() { + ast::Expr::Call(ast::ExprCall { func, .. }) => { + checker.semantic().match_builtin_expr(func, "StopIteration") + } + expr => checker.semantic().match_builtin_expr(expr, "StopIteration"), + }; + + if !is_stop_iteration { + return; + } + + // Now check the (more expensive) generator context + if !in_generator_context(checker) { + return; + } + + checker.report_diagnostic(StopIterationReturn, raise_stmt.range()); +} + +/// Returns true if we're inside a function that contains any `yield`/`yield from`. +fn in_generator_context(checker: &Checker) -> bool { + for scope in checker.semantic().current_scopes() { + if let ruff_python_semantic::ScopeKind::Function(function_def) = scope.kind { + if contains_yield_statement(&function_def.body) { + return true; + } + } + } + false +} + +/// Check if a statement list contains any yield statements +fn contains_yield_statement(body: &[ast::Stmt]) -> bool { + struct YieldFinder { + found: bool, + } + + impl Visitor<'_> for YieldFinder { + fn visit_expr(&mut self, expr: &ast::Expr) { + if matches!(expr, ast::Expr::Yield(_) | ast::Expr::YieldFrom(_)) { + self.found = true; + } else { + walk_expr(self, expr); + } + } + } + + let mut finder = YieldFinder { found: false }; + for stmt in body { + walk_stmt(&mut finder, stmt); + if finder.found { + return true; + } + } + false +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1708_stop_iteration_return.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1708_stop_iteration_return.py.snap new file mode 100644 index 0000000000..5773ff81ea --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1708_stop_iteration_return.py.snap @@ -0,0 +1,109 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:38:5 + | +36 | yield 1 +37 | yield 2 +38 | raise StopIteration # Should trigger + | ^^^^^^^^^^^^^^^^^^^ + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:44:5 + | +42 | yield 1 +43 | yield 2 +44 | raise StopIteration("finished") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:50:5 + | +48 | yield 1 +49 | yield 2 +50 | raise StopIteration(1 + 2) # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:56:5 + | +54 | yield 1 +55 | yield 2 +56 | raise StopIteration("async") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:62:9 + | +60 | def inner_gen(): +61 | yield 1 +62 | raise StopIteration("inner") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +63 | +64 | yield from inner_gen() + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:71:13 + | +69 | def generator_method(self): +70 | yield 1 +71 | raise StopIteration("method") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +72 | +73 | return MyClass + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:81:9 + | +79 | yield 1 +80 | yield 2 +81 | raise StopIteration("complex") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +82 | except ValueError: +83 | yield 3 + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:91:9 + | +89 | yield 1 +90 | if condition: +91 | raise StopIteration("conditional") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +92 | yield 2 + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:98:5 + | +96 | def generator_with_bare_stop_iteration(): +97 | yield 1 +98 | raise StopIteration # Should trigger (no arguments) + | ^^^^^^^^^^^^^^^^^^^ + | +help: Use `return` instead + +PLR1708 Explicit `raise StopIteration` in generator + --> stop_iteration_return.py:105:13 + | +103 | yield i +104 | if i == 3: +105 | raise StopIteration("loop") # Should trigger + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `return` instead diff --git a/ruff.schema.json b/ruff.schema.json index 1917af7f6d..04ef3fcc3d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3715,6 +3715,7 @@ "PLR170", "PLR1702", "PLR1704", + "PLR1708", "PLR171", "PLR1711", "PLR1714",