From 32620b80d3ab57a6811aa6f3e8f5d1456bb28fe7 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 7 Nov 2025 00:05:56 -0500 Subject: [PATCH 1/4] fix-18886 --- .../flake8_annotations/auto_return_type.py | 23 +++++ .../src/rules/flake8_annotations/helpers.rs | 98 ++++++++++++++++++- .../flake8_annotations/rules/definition.rs | 8 +- ..._annotations__tests__auto_return_type.snap | 68 +++++++++++++ ...tations__tests__auto_return_type_py38.snap | 68 +++++++++++++ 5 files changed, 258 insertions(+), 7 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py b/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py index 1fa9454ffd39e..b6cd1ea9a6010 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py @@ -302,3 +302,26 @@ def func(x: int): return 1 case y: return "foo" + + +# Test cases for NotImplementedError - should not get NoReturn auto-fix +def func(): + raise NotImplementedError + + +class Class: + @staticmethod + def func(): + raise NotImplementedError + + @classmethod + def method(cls): + raise NotImplementedError + + def instance_method(self): + raise NotImplementedError + + +# Test case: function that raises other exceptions should still get NoReturn +def func(): + raise ValueError diff --git a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs index 1d7047da77bff..2a0992ac600ab 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs @@ -2,7 +2,7 @@ use itertools::Itertools; use rustc_hash::FxHashSet; use ruff_python_ast::helpers::{ - ReturnStatementVisitor, pep_604_union, typing_optional, typing_union, + ReturnStatementVisitor, map_callable, pep_604_union, typing_optional, typing_union, }; use ruff_python_ast::name::Name; use ruff_python_ast::visitor::Visitor; @@ -47,8 +47,95 @@ pub(crate) fn is_overload_impl( } } +/// Returns `true` if all raise statements in the function body are `NotImplementedError`. +fn only_raises_not_implemented_error( + function: &ast::StmtFunctionDef, + semantic: &SemanticModel, +) -> bool { + use ruff_python_ast::statement_visitor::{StatementVisitor, walk_body}; + + struct NotImplementedErrorVisitor<'a> { + raises: Vec<&'a ast::StmtRaise>, + } + + impl<'a> StatementVisitor<'a> for NotImplementedErrorVisitor<'a> { + fn visit_stmt(&mut self, stmt: &'a ast::Stmt) { + match stmt { + ast::Stmt::Raise(raise) => { + self.raises.push(raise); + } + ast::Stmt::ClassDef(_) | ast::Stmt::FunctionDef(_) => { + // Don't recurse into nested classes/functions + } + ast::Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + walk_body(self, body); + for clause in elif_else_clauses { + walk_body(self, &clause.body); + } + } + ast::Stmt::For(ast::StmtFor { body, orelse, .. }) + | ast::Stmt::While(ast::StmtWhile { body, orelse, .. }) => { + walk_body(self, body); + walk_body(self, orelse); + } + ast::Stmt::With(ast::StmtWith { body, .. }) => { + walk_body(self, body); + } + ast::Stmt::Match(ast::StmtMatch { cases, .. }) => { + for case in cases { + walk_body(self, &case.body); + } + } + ast::Stmt::Try(ast::StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }) => { + walk_body(self, body); + for handler in handlers { + let ast::ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + body, + .. + }) = handler; + walk_body(self, body); + } + walk_body(self, orelse); + walk_body(self, finalbody); + } + _ => {} + } + } + } + + let mut visitor = NotImplementedErrorVisitor { raises: Vec::new() }; + walk_body(&mut visitor, &function.body); + + // If there are no raises, return false + if visitor.raises.is_empty() { + return false; + } + + // Check if all raises are NotImplementedError + visitor.raises.iter().all(|raise| { + raise.exc.as_ref().is_some_and(|exc| { + semantic + .resolve_builtin_symbol(map_callable(exc)) + .is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented")) + }) + }) +} + /// Given a function, guess its return type. -pub(crate) fn auto_return_type(function: &ast::StmtFunctionDef) -> Option { +pub(crate) fn auto_return_type( + function: &ast::StmtFunctionDef, + semantic: &SemanticModel, +) -> Option { // Collect all the `return` statements. let returns = { let mut visitor = ReturnStatementVisitor::default(); @@ -65,8 +152,13 @@ pub(crate) fn auto_return_type(function: &ast::StmtFunctionDef) -> Option auto_return_type.py:308:5 + | +307 | # Test cases for NotImplementedError - should not get NoReturn auto-fix +308 | def func(): + | ^^^^ +309 | raise NotImplementedError + | +help: Add return type annotation + +ANN205 Missing return type annotation for staticmethod `func` + --> auto_return_type.py:314:9 + | +312 | class Class: +313 | @staticmethod +314 | def func(): + | ^^^^ +315 | raise NotImplementedError + | +help: Add return type annotation + +ANN206 Missing return type annotation for classmethod `method` + --> auto_return_type.py:318:9 + | +317 | @classmethod +318 | def method(cls): + | ^^^^^^ +319 | raise NotImplementedError + | +help: Add return type annotation + +ANN201 Missing return type annotation for public function `instance_method` + --> auto_return_type.py:321:9 + | +319 | raise NotImplementedError +320 | +321 | def instance_method(self): + | ^^^^^^^^^^^^^^^ +322 | raise NotImplementedError + | +help: Add return type annotation + +ANN201 [*] Missing return type annotation for public function `func` + --> auto_return_type.py:326:5 + | +325 | # Test case: function that raises other exceptions should still get NoReturn +326 | def func(): + | ^^^^ +327 | raise ValueError + | +help: Add return type annotation: `Never` +214 | return 1 +215 | +216 | + - from typing import overload +217 + from typing import overload, Never +218 | +219 | +220 | @overload +-------------------------------------------------------------------------------- +323 | +324 | +325 | # Test case: function that raises other exceptions should still get NoReturn + - def func(): +326 + def func() -> Never: +327 | raise ValueError +note: This is an unsafe fix and may change runtime behavior diff --git a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap index 65f71dd345cfa..41d76ce242c5f 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap +++ b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap @@ -879,3 +879,71 @@ help: Add return type annotation: `Union[str, int]` 301 | case [1, 2, 3]: 302 | return 1 note: This is an unsafe fix and may change runtime behavior + +ANN201 Missing return type annotation for public function `func` + --> auto_return_type.py:308:5 + | +307 | # Test cases for NotImplementedError - should not get NoReturn auto-fix +308 | def func(): + | ^^^^ +309 | raise NotImplementedError + | +help: Add return type annotation + +ANN205 Missing return type annotation for staticmethod `func` + --> auto_return_type.py:314:9 + | +312 | class Class: +313 | @staticmethod +314 | def func(): + | ^^^^ +315 | raise NotImplementedError + | +help: Add return type annotation + +ANN206 Missing return type annotation for classmethod `method` + --> auto_return_type.py:318:9 + | +317 | @classmethod +318 | def method(cls): + | ^^^^^^ +319 | raise NotImplementedError + | +help: Add return type annotation + +ANN201 Missing return type annotation for public function `instance_method` + --> auto_return_type.py:321:9 + | +319 | raise NotImplementedError +320 | +321 | def instance_method(self): + | ^^^^^^^^^^^^^^^ +322 | raise NotImplementedError + | +help: Add return type annotation + +ANN201 [*] Missing return type annotation for public function `func` + --> auto_return_type.py:326:5 + | +325 | # Test case: function that raises other exceptions should still get NoReturn +326 | def func(): + | ^^^^ +327 | raise ValueError + | +help: Add return type annotation: `NoReturn` +214 | return 1 +215 | +216 | + - from typing import overload +217 + from typing import overload, NoReturn +218 | +219 | +220 | @overload +-------------------------------------------------------------------------------- +323 | +324 | +325 | # Test case: function that raises other exceptions should still get NoReturn + - def func(): +326 + def func() -> NoReturn: +327 | raise ValueError +note: This is an unsafe fix and may change runtime behavior From 0c87832e8acc25a09a709718136efe7e6cbd8634 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 7 Nov 2025 17:59:19 -0500 Subject: [PATCH 2/4] Add `Terminal::RaiseNotImplemented` variant to avoid double traversal Changes: - Add `Terminal::RaiseNotImplemented` enum variant - Modify `Terminal::from_function()` to accept `SemanticModel` and detect `NotImplementedError` during traversal - Update `and_then()` and `branch()` combination methods to handle the new variant - Remove `only_raises_not_implemented_error()` helper function - Update `auto_return_type()` to check for `Terminal::RaiseNotImplemented` directly - Update all call sites to pass `SemanticModel` to `Terminal::from_function()` --- .../src/rules/flake8_annotations/helpers.rs | 102 ++---------------- .../rules/pylint/rules/invalid_bool_return.rs | 2 +- .../pylint/rules/invalid_bytes_return.rs | 2 +- .../rules/pylint/rules/invalid_hash_return.rs | 2 +- .../pylint/rules/invalid_index_return.rs | 2 +- .../pylint/rules/invalid_length_return.rs | 2 +- .../rules/pylint/rules/invalid_str_return.rs | 2 +- .../src/analyze/terminal.rs | 82 ++++++++++---- 8 files changed, 80 insertions(+), 116 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs index 2a0992ac600ab..6467699c23562 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs @@ -2,7 +2,7 @@ use itertools::Itertools; use rustc_hash::FxHashSet; use ruff_python_ast::helpers::{ - ReturnStatementVisitor, map_callable, pep_604_union, typing_optional, typing_union, + ReturnStatementVisitor, pep_604_union, typing_optional, typing_union, }; use ruff_python_ast::name::Name; use ruff_python_ast::visitor::Visitor; @@ -47,90 +47,6 @@ pub(crate) fn is_overload_impl( } } -/// Returns `true` if all raise statements in the function body are `NotImplementedError`. -fn only_raises_not_implemented_error( - function: &ast::StmtFunctionDef, - semantic: &SemanticModel, -) -> bool { - use ruff_python_ast::statement_visitor::{StatementVisitor, walk_body}; - - struct NotImplementedErrorVisitor<'a> { - raises: Vec<&'a ast::StmtRaise>, - } - - impl<'a> StatementVisitor<'a> for NotImplementedErrorVisitor<'a> { - fn visit_stmt(&mut self, stmt: &'a ast::Stmt) { - match stmt { - ast::Stmt::Raise(raise) => { - self.raises.push(raise); - } - ast::Stmt::ClassDef(_) | ast::Stmt::FunctionDef(_) => { - // Don't recurse into nested classes/functions - } - ast::Stmt::If(ast::StmtIf { - body, - elif_else_clauses, - .. - }) => { - walk_body(self, body); - for clause in elif_else_clauses { - walk_body(self, &clause.body); - } - } - ast::Stmt::For(ast::StmtFor { body, orelse, .. }) - | ast::Stmt::While(ast::StmtWhile { body, orelse, .. }) => { - walk_body(self, body); - walk_body(self, orelse); - } - ast::Stmt::With(ast::StmtWith { body, .. }) => { - walk_body(self, body); - } - ast::Stmt::Match(ast::StmtMatch { cases, .. }) => { - for case in cases { - walk_body(self, &case.body); - } - } - ast::Stmt::Try(ast::StmtTry { - body, - handlers, - orelse, - finalbody, - .. - }) => { - walk_body(self, body); - for handler in handlers { - let ast::ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { - body, - .. - }) = handler; - walk_body(self, body); - } - walk_body(self, orelse); - walk_body(self, finalbody); - } - _ => {} - } - } - } - - let mut visitor = NotImplementedErrorVisitor { raises: Vec::new() }; - walk_body(&mut visitor, &function.body); - - // If there are no raises, return false - if visitor.raises.is_empty() { - return false; - } - - // Check if all raises are NotImplementedError - visitor.raises.iter().all(|raise| { - raise.exc.as_ref().is_some_and(|exc| { - semantic - .resolve_builtin_symbol(map_callable(exc)) - .is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented")) - }) - }) -} - /// Given a function, guess its return type. pub(crate) fn auto_return_type( function: &ast::StmtFunctionDef, @@ -150,15 +66,17 @@ pub(crate) fn auto_return_type( }; // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function); + let terminal = Terminal::from_function(function, semantic); - // If every control flow path raises an exception, check if it's NotImplementedError. - // If all raises are NotImplementedError, don't suggest NoReturn since these are - // abstract methods that should have the actual return type. + // If every control flow path raises NotImplementedError, don't suggest NoReturn + // since these are abstract methods that should have the actual return type. + if terminal == Terminal::RaiseNotImplemented { + return None; + } + + // If every control flow path raises an exception (other than NotImplementedError), + // suggest NoReturn. if terminal == Terminal::Raise { - if only_raises_not_implemented_error(function, semantic) { - return None; - } return Some(AutoPythonType::Never); } diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs index ab7970ddce9ea..f48b3e083ea3c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs @@ -60,7 +60,7 @@ pub(crate) fn invalid_bool_return(checker: &Checker, function_def: &ast::StmtFun } // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function_def); + let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. if terminal == Terminal::Raise { diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs index ca2126ef2ac2e..9e89ba2ba6a12 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -60,7 +60,7 @@ pub(crate) fn invalid_bytes_return(checker: &Checker, function_def: &ast::StmtFu } // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function_def); + let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. if terminal == Terminal::Raise { diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index 4ed2f1a470a1e..8f76d8c72f842 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -64,7 +64,7 @@ pub(crate) fn invalid_hash_return(checker: &Checker, function_def: &ast::StmtFun } // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function_def); + let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. if terminal == Terminal::Raise { diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs index c863e1011d089..a45df7099dfc0 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs @@ -66,7 +66,7 @@ pub(crate) fn invalid_index_return(checker: &Checker, function_def: &ast::StmtFu } // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function_def); + let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. if terminal == Terminal::Raise { diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs index cf659ffa2b4b9..4b9986a1245cb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs @@ -65,7 +65,7 @@ pub(crate) fn invalid_length_return(checker: &Checker, function_def: &ast::StmtF } // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function_def); + let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. if terminal == Terminal::Raise { diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs index ecc67de40289f..86206e9c39705 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs @@ -60,7 +60,7 @@ pub(crate) fn invalid_str_return(checker: &Checker, function_def: &ast::StmtFunc } // Determine the terminal behavior (i.e., implicit return, no return, etc.). - let terminal = Terminal::from_function(function_def); + let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. if terminal == Terminal::Raise { diff --git a/crates/ruff_python_semantic/src/analyze/terminal.rs b/crates/ruff_python_semantic/src/analyze/terminal.rs index 4702ac439a8eb..7efc8c0730908 100644 --- a/crates/ruff_python_semantic/src/analyze/terminal.rs +++ b/crates/ruff_python_semantic/src/analyze/terminal.rs @@ -1,5 +1,8 @@ +use ruff_python_ast::helpers::map_callable; use ruff_python_ast::{self as ast, ExceptHandler, Stmt}; +use crate::model::SemanticModel; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Terminal { /// There is no known terminal. @@ -8,6 +11,8 @@ pub enum Terminal { Implicit, /// Every path through the function ends with a `raise` statement. Raise, + /// Every path through the function ends with a `raise NotImplementedError` statement. + RaiseNotImplemented, /// No path through the function ends with a `return` statement. Return, /// Every path through the function ends with a `return` or `raise` statement. @@ -18,8 +23,8 @@ pub enum Terminal { impl Terminal { /// Returns the [`Terminal`] behavior of the function, if it can be determined. - pub fn from_function(function: &ast::StmtFunctionDef) -> Terminal { - Self::from_body(&function.body) + pub fn from_function(function: &ast::StmtFunctionDef, semantic: &SemanticModel) -> Terminal { + Self::from_body(&function.body, Some(semantic)) } /// Returns `true` if the [`Terminal`] behavior includes at least one `return` path. @@ -36,7 +41,7 @@ impl Terminal { } /// Returns the [`Terminal`] behavior of the body, if it can be determined. - fn from_body(stmts: &[Stmt]) -> Terminal { + fn from_body(stmts: &[Stmt], semantic: Option<&SemanticModel>) -> Terminal { let mut terminal = Terminal::None; for stmt in stmts { @@ -47,10 +52,10 @@ impl Terminal { continue; } - terminal = terminal.and_then(Self::from_body(body)); + terminal = terminal.and_then(Self::from_body(body, semantic)); if !sometimes_breaks(body) { - terminal = terminal.and_then(Self::from_body(orelse)); + terminal = terminal.and_then(Self::from_body(orelse, semantic)); } } Stmt::If(ast::StmtIf { @@ -59,10 +64,10 @@ impl Terminal { .. }) => { let branch_terminal = Terminal::branches( - std::iter::once(Self::from_body(body)).chain( + std::iter::once(Self::from_body(body, semantic)).chain( elif_else_clauses .iter() - .map(|clause| Self::from_body(&clause.body)), + .map(|clause| Self::from_body(&clause.body, semantic)), ), ); @@ -79,7 +84,9 @@ impl Terminal { } Stmt::Match(ast::StmtMatch { cases, .. }) => { let branch_terminal = terminal.and_then(Terminal::branches( - cases.iter().map(|case| Self::from_body(&case.body)), + cases + .iter() + .map(|case| Self::from_body(&case.body, semantic)), )); // If the `match` is known to be exhaustive (by way of including a wildcard @@ -106,21 +113,21 @@ impl Terminal { // that _any_ statement in the body could raise an exception, so we don't // consider the body to be exhaustive. In other words, we assume the exception // handlers exist for a reason. - let body_terminal = Self::from_body(body); + let body_terminal = Self::from_body(body, semantic); if body_terminal.has_any_return() { terminal = terminal.and_then(Terminal::ConditionalReturn); } // If the `finally` block returns, the `try` block must also return. (Similarly, // if the `finally` block raises, the `try` block must also raise.) - terminal = terminal.and_then(Self::from_body(finalbody)); + terminal = terminal.and_then(Self::from_body(finalbody, semantic)); let branch_terminal = Terminal::branches(handlers.iter().map(|handler| { let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler; - Self::from_body(body) + Self::from_body(body, semantic) })); if orelse.is_empty() { @@ -132,18 +139,35 @@ impl Terminal { } else { // If there's an `else`, we won't fall through. If all the handlers and // the `else` block return,, the `try` block also returns. - terminal = - terminal.and_then(branch_terminal.branch(Terminal::from_body(orelse))); + terminal = terminal.and_then( + branch_terminal.branch(Terminal::from_body(orelse, semantic)), + ); } } Stmt::With(ast::StmtWith { body, .. }) => { - terminal = terminal.and_then(Self::from_body(body)); + terminal = terminal.and_then(Self::from_body(body, semantic)); } Stmt::Return(_) => { terminal = terminal.and_then(Terminal::RaiseOrReturn); } - Stmt::Raise(_) => { - terminal = terminal.and_then(Terminal::Raise); + Stmt::Raise(raise) => { + // Check if this raise is NotImplementedError (only if semantic model is available) + let is_not_implemented = semantic + .and_then(|sem| { + raise.exc.as_ref().and_then(|exc| { + sem.resolve_builtin_symbol(map_callable(exc)) + .filter(|name| { + matches!(*name, "NotImplementedError" | "NotImplemented") + }) + }) + }) + .is_some(); + + if is_not_implemented { + terminal = terminal.and_then(Terminal::RaiseNotImplemented); + } else { + terminal = terminal.and_then(Terminal::Raise); + } } _ => {} } @@ -174,21 +198,32 @@ impl Terminal { (Self::Raise, Self::ConditionalReturn) => Self::RaiseOrReturn, (Self::ConditionalReturn, Self::Raise) => Self::RaiseOrReturn, + // If one of the operators is `RaiseNotImplemented`, treat it like `Raise` for combinations + (Self::RaiseNotImplemented, Self::ConditionalReturn) => Self::RaiseOrReturn, + (Self::ConditionalReturn, Self::RaiseNotImplemented) => Self::RaiseOrReturn, + // If one of the operators is `Return`, then the function returns. (Self::Return, Self::ConditionalReturn) => Self::Return, (Self::ConditionalReturn, Self::Return) => Self::Return, // All paths through the function end with a `raise` statement. (Self::Raise, Self::Raise) => Self::Raise, + // All paths through the function end with a `raise NotImplementedError` statement. + (Self::RaiseNotImplemented, Self::RaiseNotImplemented) => Self::RaiseNotImplemented, + // Mixed raises: if one is NotImplementedError and one is regular Raise, result is Raise + (Self::RaiseNotImplemented, Self::Raise) => Self::Raise, + (Self::Raise, Self::RaiseNotImplemented) => Self::Raise, // All paths through the function end with a `return` statement. (Self::Return, Self::Return) => Self::Return, // All paths through the function end with a `return` or `raise` statement. (Self::Raise, Self::Return) => Self::RaiseOrReturn, + (Self::RaiseNotImplemented, Self::Return) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::Return, Self::Raise) => Self::RaiseOrReturn, + (Self::Return, Self::RaiseNotImplemented) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, @@ -207,6 +242,8 @@ impl Terminal { (Self::Implicit, Self::Implicit) => Self::Implicit, (Self::Implicit, Self::Raise) => Self::Implicit, (Self::Raise, Self::Implicit) => Self::Implicit, + (Self::Implicit, Self::RaiseNotImplemented) => Self::Implicit, + (Self::RaiseNotImplemented, Self::Implicit) => Self::Implicit, (Self::Implicit, Self::Return) => Self::ConditionalReturn, (Self::Return, Self::Implicit) => Self::ConditionalReturn, (Self::Implicit, Self::RaiseOrReturn) => Self::ConditionalReturn, @@ -219,18 +256,27 @@ impl Terminal { (Self::Raise, Self::ConditionalReturn) => Self::RaiseOrReturn, (Self::ConditionalReturn, Self::Raise) => Self::RaiseOrReturn, + (Self::RaiseNotImplemented, Self::ConditionalReturn) => Self::RaiseOrReturn, + (Self::ConditionalReturn, Self::RaiseNotImplemented) => Self::RaiseOrReturn, (Self::Return, Self::ConditionalReturn) => Self::Return, (Self::ConditionalReturn, Self::Return) => Self::Return, // All paths through the function end with a `raise` statement. (Self::Raise, Self::Raise) => Self::Raise, + // All paths through the function end with a `raise NotImplementedError` statement. + (Self::RaiseNotImplemented, Self::RaiseNotImplemented) => Self::RaiseNotImplemented, + // Mixed raises: if one is NotImplementedError and one is regular Raise, result is Raise + (Self::RaiseNotImplemented, Self::Raise) => Self::Raise, + (Self::Raise, Self::RaiseNotImplemented) => Self::Raise, // All paths through the function end with a `return` statement. (Self::Return, Self::Return) => Self::Return, // All paths through the function end with a `return` or `raise` statement. (Self::Raise, Self::Return) => Self::RaiseOrReturn, + (Self::RaiseNotImplemented, Self::Return) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::Return, Self::Raise) => Self::RaiseOrReturn, + (Self::Return, Self::RaiseNotImplemented) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, (_, Self::RaiseOrReturn) => Self::RaiseOrReturn, @@ -248,7 +294,7 @@ fn sometimes_breaks(stmts: &[Stmt]) -> bool { for stmt in stmts { match stmt { Stmt::For(ast::StmtFor { body, orelse, .. }) => { - if Terminal::from_body(body).has_any_return() { + if Terminal::from_body(body, None).has_any_return() { return false; } if sometimes_breaks(orelse) { @@ -256,7 +302,7 @@ fn sometimes_breaks(stmts: &[Stmt]) -> bool { } } Stmt::While(ast::StmtWhile { body, orelse, .. }) => { - if Terminal::from_body(body).has_any_return() { + if Terminal::from_body(body, None).has_any_return() { return false; } if sometimes_breaks(orelse) { From 510242877f3475bdea674200007f888fc76ffc57 Mon Sep 17 00:00:00 2001 From: Dan Date: Sun, 25 Jan 2026 22:14:59 -0500 Subject: [PATCH 3/4] Implement feedback --- .../rules/pylint/rules/invalid_bool_return.rs | 2 +- .../pylint/rules/invalid_bytes_return.rs | 2 +- .../rules/pylint/rules/invalid_hash_return.rs | 2 +- .../pylint/rules/invalid_index_return.rs | 2 +- .../pylint/rules/invalid_length_return.rs | 2 +- .../rules/pylint/rules/invalid_str_return.rs | 2 +- .../src/analyze/terminal.rs | 109 +++++++++--------- 7 files changed, 61 insertions(+), 60 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs index f48b3e083ea3c..cd49f9ad69815 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs @@ -63,7 +63,7 @@ pub(crate) fn invalid_bool_return(checker: &Checker, function_def: &ast::StmtFun let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. - if terminal == Terminal::Raise { + if terminal.is_always_raise() { return; } diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs index 9e89ba2ba6a12..8040620a76e02 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs @@ -63,7 +63,7 @@ pub(crate) fn invalid_bytes_return(checker: &Checker, function_def: &ast::StmtFu let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. - if terminal == Terminal::Raise { + if terminal.is_always_raise() { return; } diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs index 8f76d8c72f842..59e451142d2ec 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs @@ -67,7 +67,7 @@ pub(crate) fn invalid_hash_return(checker: &Checker, function_def: &ast::StmtFun let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. - if terminal == Terminal::Raise { + if terminal.is_always_raise() { return; } diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs index a45df7099dfc0..881823b5511fb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs @@ -69,7 +69,7 @@ pub(crate) fn invalid_index_return(checker: &Checker, function_def: &ast::StmtFu let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. - if terminal == Terminal::Raise { + if terminal.is_always_raise() { return; } diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs index 4b9986a1245cb..16db437ec27f1 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs @@ -68,7 +68,7 @@ pub(crate) fn invalid_length_return(checker: &Checker, function_def: &ast::StmtF let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. - if terminal == Terminal::Raise { + if terminal.is_always_raise() { return; } diff --git a/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs b/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs index 86206e9c39705..5ad2821f1d9f0 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/invalid_str_return.rs @@ -63,7 +63,7 @@ pub(crate) fn invalid_str_return(checker: &Checker, function_def: &ast::StmtFunc let terminal = Terminal::from_function(function_def, checker.semantic()); // If every control flow path raises an exception, ignore the function. - if terminal == Terminal::Raise { + if terminal.is_always_raise() { return; } diff --git a/crates/ruff_python_semantic/src/analyze/terminal.rs b/crates/ruff_python_semantic/src/analyze/terminal.rs index 7efc8c0730908..fbbe143df9f7a 100644 --- a/crates/ruff_python_semantic/src/analyze/terminal.rs +++ b/crates/ruff_python_semantic/src/analyze/terminal.rs @@ -24,7 +24,7 @@ pub enum Terminal { impl Terminal { /// Returns the [`Terminal`] behavior of the function, if it can be determined. pub fn from_function(function: &ast::StmtFunctionDef, semantic: &SemanticModel) -> Terminal { - Self::from_body(&function.body, Some(semantic)) + Self::from_body(&function.body, semantic) } /// Returns `true` if the [`Terminal`] behavior includes at least one `return` path. @@ -40,8 +40,13 @@ impl Terminal { matches!(self, Self::None | Self::Implicit | Self::ConditionalReturn) } + /// Returns `true` if the [`Terminal`] behavior is `Raise` or `RaiseNotImplemented`. + pub fn is_always_raise(self) -> bool { + matches!(self, Self::Raise | Self::RaiseNotImplemented) + } + /// Returns the [`Terminal`] behavior of the body, if it can be determined. - fn from_body(stmts: &[Stmt], semantic: Option<&SemanticModel>) -> Terminal { + fn from_body(stmts: &[Stmt], semantic: &SemanticModel) -> Terminal { let mut terminal = Terminal::None; for stmt in stmts { @@ -54,7 +59,7 @@ impl Terminal { terminal = terminal.and_then(Self::from_body(body, semantic)); - if !sometimes_breaks(body) { + if !sometimes_breaks(body, semantic) { terminal = terminal.and_then(Self::from_body(orelse, semantic)); } } @@ -151,17 +156,12 @@ impl Terminal { terminal = terminal.and_then(Terminal::RaiseOrReturn); } Stmt::Raise(raise) => { - // Check if this raise is NotImplementedError (only if semantic model is available) - let is_not_implemented = semantic - .and_then(|sem| { - raise.exc.as_ref().and_then(|exc| { - sem.resolve_builtin_symbol(map_callable(exc)) - .filter(|name| { - matches!(*name, "NotImplementedError" | "NotImplemented") - }) - }) - }) - .is_some(); + // Check if this raise is NotImplementedError + let is_not_implemented = raise.exc.as_ref().is_some_and(|exc| { + let exc = map_callable(exc); + semantic.match_builtin_expr(exc, "NotImplementedError") + || semantic.match_builtin_expr(exc, "NotImplemented") + }); if is_not_implemented { terminal = terminal.and_then(Terminal::RaiseNotImplemented); @@ -193,14 +193,14 @@ impl Terminal { // If both operators are conditional returns, the result is a conditional return. (Self::ConditionalReturn, Self::ConditionalReturn) => Self::ConditionalReturn, - // If one of the operators is `Raise`, then the function ends with an explicit `raise` - // or `return` statement. - (Self::Raise, Self::ConditionalReturn) => Self::RaiseOrReturn, - (Self::ConditionalReturn, Self::Raise) => Self::RaiseOrReturn, - - // If one of the operators is `RaiseNotImplemented`, treat it like `Raise` for combinations - (Self::RaiseNotImplemented, Self::ConditionalReturn) => Self::RaiseOrReturn, - (Self::ConditionalReturn, Self::RaiseNotImplemented) => Self::RaiseOrReturn, + // If one of the operators is `Raise` or `RaiseNotImplemented`, then the function ends + // with an explicit `raise` or `return` statement. + (Self::Raise | Self::RaiseNotImplemented, Self::ConditionalReturn) => { + Self::RaiseOrReturn + } + (Self::ConditionalReturn, Self::Raise | Self::RaiseNotImplemented) => { + Self::RaiseOrReturn + } // If one of the operators is `Return`, then the function returns. (Self::Return, Self::ConditionalReturn) => Self::Return, @@ -211,19 +211,18 @@ impl Terminal { // All paths through the function end with a `raise NotImplementedError` statement. (Self::RaiseNotImplemented, Self::RaiseNotImplemented) => Self::RaiseNotImplemented, // Mixed raises: if one is NotImplementedError and one is regular Raise, result is Raise - (Self::RaiseNotImplemented, Self::Raise) => Self::Raise, - (Self::Raise, Self::RaiseNotImplemented) => Self::Raise, + (Self::RaiseNotImplemented, Self::Raise) | (Self::Raise, Self::RaiseNotImplemented) => { + Self::Raise + } // All paths through the function end with a `return` statement. (Self::Return, Self::Return) => Self::Return, // All paths through the function end with a `return` or `raise` statement. - (Self::Raise, Self::Return) => Self::RaiseOrReturn, - (Self::RaiseNotImplemented, Self::Return) => Self::RaiseOrReturn, + (Self::Raise | Self::RaiseNotImplemented, Self::Return) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. - (Self::Return, Self::Raise) => Self::RaiseOrReturn, - (Self::Return, Self::RaiseNotImplemented) => Self::RaiseOrReturn, + (Self::Return, Self::Raise | Self::RaiseNotImplemented) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, @@ -240,10 +239,8 @@ impl Terminal { // If one of the operators is `Implicit`, the other operator should be downgraded. (Self::Implicit, Self::Implicit) => Self::Implicit, - (Self::Implicit, Self::Raise) => Self::Implicit, - (Self::Raise, Self::Implicit) => Self::Implicit, - (Self::Implicit, Self::RaiseNotImplemented) => Self::Implicit, - (Self::RaiseNotImplemented, Self::Implicit) => Self::Implicit, + (Self::Implicit, Self::Raise | Self::RaiseNotImplemented) => Self::Implicit, + (Self::Raise | Self::RaiseNotImplemented, Self::Implicit) => Self::Implicit, (Self::Implicit, Self::Return) => Self::ConditionalReturn, (Self::Return, Self::Implicit) => Self::ConditionalReturn, (Self::Implicit, Self::RaiseOrReturn) => Self::ConditionalReturn, @@ -254,10 +251,12 @@ impl Terminal { // If both operators are conditional returns, the result is a conditional return. (Self::ConditionalReturn, Self::ConditionalReturn) => Self::ConditionalReturn, - (Self::Raise, Self::ConditionalReturn) => Self::RaiseOrReturn, - (Self::ConditionalReturn, Self::Raise) => Self::RaiseOrReturn, - (Self::RaiseNotImplemented, Self::ConditionalReturn) => Self::RaiseOrReturn, - (Self::ConditionalReturn, Self::RaiseNotImplemented) => Self::RaiseOrReturn, + (Self::Raise | Self::RaiseNotImplemented, Self::ConditionalReturn) => { + Self::RaiseOrReturn + } + (Self::ConditionalReturn, Self::Raise | Self::RaiseNotImplemented) => { + Self::RaiseOrReturn + } (Self::Return, Self::ConditionalReturn) => Self::Return, (Self::ConditionalReturn, Self::Return) => Self::Return, @@ -267,16 +266,15 @@ impl Terminal { // All paths through the function end with a `raise NotImplementedError` statement. (Self::RaiseNotImplemented, Self::RaiseNotImplemented) => Self::RaiseNotImplemented, // Mixed raises: if one is NotImplementedError and one is regular Raise, result is Raise - (Self::RaiseNotImplemented, Self::Raise) => Self::Raise, - (Self::Raise, Self::RaiseNotImplemented) => Self::Raise, + (Self::RaiseNotImplemented, Self::Raise) | (Self::Raise, Self::RaiseNotImplemented) => { + Self::Raise + } // All paths through the function end with a `return` statement. (Self::Return, Self::Return) => Self::Return, // All paths through the function end with a `return` or `raise` statement. - (Self::Raise, Self::Return) => Self::RaiseOrReturn, - (Self::RaiseNotImplemented, Self::Return) => Self::RaiseOrReturn, + (Self::Raise | Self::RaiseNotImplemented, Self::Return) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. - (Self::Return, Self::Raise) => Self::RaiseOrReturn, - (Self::Return, Self::RaiseNotImplemented) => Self::RaiseOrReturn, + (Self::Return, Self::Raise | Self::RaiseNotImplemented) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, (_, Self::RaiseOrReturn) => Self::RaiseOrReturn, @@ -290,22 +288,22 @@ impl Terminal { } /// Returns `true` if the body may break via a `break` statement. -fn sometimes_breaks(stmts: &[Stmt]) -> bool { +fn sometimes_breaks(stmts: &[Stmt], semantic: &SemanticModel) -> bool { for stmt in stmts { match stmt { Stmt::For(ast::StmtFor { body, orelse, .. }) => { - if Terminal::from_body(body, None).has_any_return() { + if Terminal::from_body(body, semantic).has_any_return() { return false; } - if sometimes_breaks(orelse) { + if sometimes_breaks(orelse, semantic) { return true; } } Stmt::While(ast::StmtWhile { body, orelse, .. }) => { - if Terminal::from_body(body, None).has_any_return() { + if Terminal::from_body(body, semantic).has_any_return() { return false; } - if sometimes_breaks(orelse) { + if sometimes_breaks(orelse, semantic) { return true; } } @@ -316,13 +314,16 @@ fn sometimes_breaks(stmts: &[Stmt]) -> bool { }) => { if std::iter::once(body) .chain(elif_else_clauses.iter().map(|clause| &clause.body)) - .any(|body| sometimes_breaks(body)) + .any(|body| sometimes_breaks(body, semantic)) { return true; } } Stmt::Match(ast::StmtMatch { cases, .. }) => { - if cases.iter().any(|case| sometimes_breaks(&case.body)) { + if cases + .iter() + .any(|case| sometimes_breaks(&case.body, semantic)) + { return true; } } @@ -333,22 +334,22 @@ fn sometimes_breaks(stmts: &[Stmt]) -> bool { finalbody, .. }) => { - if sometimes_breaks(body) + if sometimes_breaks(body, semantic) || handlers.iter().any(|handler| { let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler; - sometimes_breaks(body) + sometimes_breaks(body, semantic) }) - || sometimes_breaks(orelse) - || sometimes_breaks(finalbody) + || sometimes_breaks(orelse, semantic) + || sometimes_breaks(finalbody, semantic) { return true; } } Stmt::With(ast::StmtWith { body, .. }) => { - if sometimes_breaks(body) { + if sometimes_breaks(body, semantic) { return true; } } From 8a9e5350389a131b456df853ceedad169a3c45cf Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Tue, 27 Jan 2026 17:27:46 -0500 Subject: [PATCH 4/4] drop NotImplemented check this is a common error (see F901) but not a valid exception to raise, so I don't think it's something we should detect here --- crates/ruff_python_semantic/src/analyze/terminal.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ruff_python_semantic/src/analyze/terminal.rs b/crates/ruff_python_semantic/src/analyze/terminal.rs index fbbe143df9f7a..1d810b067e566 100644 --- a/crates/ruff_python_semantic/src/analyze/terminal.rs +++ b/crates/ruff_python_semantic/src/analyze/terminal.rs @@ -160,7 +160,6 @@ impl Terminal { let is_not_implemented = raise.exc.as_ref().is_some_and(|exc| { let exc = map_callable(exc); semantic.match_builtin_expr(exc, "NotImplementedError") - || semantic.match_builtin_expr(exc, "NotImplemented") }); if is_not_implemented {