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..6467699c23562 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs @@ -48,7 +48,10 @@ pub(crate) fn is_overload_impl( } /// 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(); @@ -63,9 +66,16 @@ 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 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..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 @@ -60,10 +60,10 @@ 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 { + 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 ca2126ef2ac2e..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 @@ -60,10 +60,10 @@ 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 { + 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 4ed2f1a470a1e..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 @@ -64,10 +64,10 @@ 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 { + 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 c863e1011d089..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 @@ -66,10 +66,10 @@ 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 { + 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 cf659ffa2b4b9..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 @@ -65,10 +65,10 @@ 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 { + 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 ecc67de40289f..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 @@ -60,10 +60,10 @@ 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 { + 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 4702ac439a8eb..1d810b067e566 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, semantic) } /// Returns `true` if the [`Terminal`] behavior includes at least one `return` path. @@ -35,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]) -> Terminal { + fn from_body(stmts: &[Stmt], semantic: &SemanticModel) -> Terminal { let mut terminal = Terminal::None; for stmt in stmts { @@ -47,10 +57,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)); + if !sometimes_breaks(body, semantic) { + terminal = terminal.and_then(Self::from_body(orelse, semantic)); } } Stmt::If(ast::StmtIf { @@ -59,10 +69,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 +89,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 +118,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 +144,29 @@ 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 + let is_not_implemented = raise.exc.as_ref().is_some_and(|exc| { + let exc = map_callable(exc); + semantic.match_builtin_expr(exc, "NotImplementedError") + }); + + if is_not_implemented { + terminal = terminal.and_then(Terminal::RaiseNotImplemented); + } else { + terminal = terminal.and_then(Terminal::Raise); + } } _ => {} } @@ -169,10 +192,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 `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, @@ -180,15 +207,21 @@ impl Terminal { // 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::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::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::Raise | Self::RaiseNotImplemented) => Self::RaiseOrReturn, // All paths through the function end with a `return` or `raise` statement. (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, @@ -205,8 +238,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::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, @@ -217,20 +250,30 @@ 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::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, // 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::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::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::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, @@ -244,22 +287,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).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).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; } } @@ -270,13 +313,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; } } @@ -287,22 +333,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; } }