From 0c87832e8acc25a09a709718136efe7e6cbd8634 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 7 Nov 2025 17:59:19 -0500 Subject: [PATCH] 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 2a0992ac60..6467699c23 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 ab7970ddce..f48b3e083e 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 ca2126ef2a..9e89ba2ba6 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 4ed2f1a470..8f76d8c72f 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 c863e1011d..a45df7099d 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 cf659ffa2b..4b9986a124 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 ecc67de402..86206e9c39 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 4702ac439a..7efc8c0730 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) {