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 1fa9454ffd..b6cd1ea9a6 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 1d7047da77..6467699c23 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 65f71dd345..41d76ce242 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 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) {