Skip to content

Commit 95e517f

Browse files
danparizherntBre
andauthored
[flake8-annotations] Don't suggest NoReturn for functions raising NotImplementedError (ANN201, ANN202, ANN205, ANN206) (#21311)
## Summary Fix ANN201, ANN202, ANN205, and ANN206 to not automatically suggest `NoReturn`/`Never` return type annotations for functions that raise `NotImplementedError`. These rules will still flag missing return type annotations but won't provide an auto-fix, allowing developers to manually specify the actual return type that implementations should return. Fixes #18886 ## Problem Analysis The bug occurred because the `auto_return_type` function in `flake8_annotations/helpers.rs` detects when all control flow paths in a function raise an exception (`Terminal::Raise`) and automatically suggests `NoReturn`/`Never` as the return type. However, `NotImplementedError` is semantically different from other exceptions - it's used to indicate abstract methods that should be implemented by subclasses, not methods that truly never return. When a function raises `NotImplementedError`, it's typically an abstract method pattern where: - The base class defines the method signature but doesn't implement it - Subclasses are expected to override the method with a proper implementation - The return type should reflect what implementations will actually return, not `NoReturn` For example, in Django's serializer pattern: ```py class BaseSerializer: def to_internal_value(self, data): raise NotImplementedError('`to_internal_value()` must be implemented.') ``` This method should have a return type annotation like `dict` or `Any`, not `NoReturn`, because implementations will return actual values. ## Approach The fix modifies the `auto_return_type` function to: 1. **Added semantic analysis capability**: Modified `auto_return_type` to accept a `SemanticModel` parameter, enabling it to analyze exception types in raise statements. 2. **Created detection helper**: Implemented `only_raises_not_implemented_error()` function that: - Traverses the function body using a custom visitor to collect all `raise` statements - Checks if all raises are `NotImplementedError` (or `NotImplemented`) using semantic analysis - Returns `true` only if all raises are `NotImplementedError` 3. **Modified return type inference**: When `Terminal::Raise` is detected, the function now: - First checks if all raises are `NotImplementedError` - If yes, returns `None` (no auto-fix suggested, but diagnostic still emitted) - If no, returns `AutoPythonType::Never` as before (suggesting `NoReturn`/`Never`) 4. **Updated all call sites**: Updated all 4 call sites in `definition.rs` to pass `checker.semantic()`: - ANN201 (public functions) - ANN202 (private functions) - ANN205 (static methods) - ANN206 (class methods) The fix ensures that: - Functions raising `NotImplementedError` still get flagged for missing return type annotations - But no auto-fix is suggested, allowing manual specification of the correct return type - Functions raising other exceptions (like `ValueError`) still correctly get `NoReturn`/`Never` suggestions --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
1 parent a8d1d86 commit 95e517f

12 files changed

Lines changed: 275 additions & 60 deletions

File tree

crates/ruff_linter/resources/test/fixtures/flake8_annotations/auto_return_type.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,3 +302,26 @@ def func(x: int):
302302
return 1
303303
case y:
304304
return "foo"
305+
306+
307+
# Test cases for NotImplementedError - should not get NoReturn auto-fix
308+
def func():
309+
raise NotImplementedError
310+
311+
312+
class Class:
313+
@staticmethod
314+
def func():
315+
raise NotImplementedError
316+
317+
@classmethod
318+
def method(cls):
319+
raise NotImplementedError
320+
321+
def instance_method(self):
322+
raise NotImplementedError
323+
324+
325+
# Test case: function that raises other exceptions should still get NoReturn
326+
def func():
327+
raise ValueError

crates/ruff_linter/src/rules/flake8_annotations/helpers.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,10 @@ pub(crate) fn is_overload_impl(
4848
}
4949

5050
/// Given a function, guess its return type.
51-
pub(crate) fn auto_return_type(function: &ast::StmtFunctionDef) -> Option<AutoPythonType> {
51+
pub(crate) fn auto_return_type(
52+
function: &ast::StmtFunctionDef,
53+
semantic: &SemanticModel,
54+
) -> Option<AutoPythonType> {
5255
// Collect all the `return` statements.
5356
let returns = {
5457
let mut visitor = ReturnStatementVisitor::default();
@@ -63,9 +66,16 @@ pub(crate) fn auto_return_type(function: &ast::StmtFunctionDef) -> Option<AutoPy
6366
};
6467

6568
// Determine the terminal behavior (i.e., implicit return, no return, etc.).
66-
let terminal = Terminal::from_function(function);
69+
let terminal = Terminal::from_function(function, semantic);
70+
71+
// If every control flow path raises NotImplementedError, don't suggest NoReturn
72+
// since these are abstract methods that should have the actual return type.
73+
if terminal == Terminal::RaiseNotImplemented {
74+
return None;
75+
}
6776

68-
// If every control flow path raises an exception, return `NoReturn`.
77+
// If every control flow path raises an exception (other than NotImplementedError),
78+
// suggest NoReturn.
6979
if terminal == Terminal::Raise {
7080
return Some(AutoPythonType::Never);
7181
}

crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -774,7 +774,7 @@ pub(crate) fn definition(
774774
let return_type = if is_stub_function(function, checker) {
775775
None
776776
} else {
777-
auto_return_type(function)
777+
auto_return_type(function, checker.semantic())
778778
.and_then(|return_type| {
779779
return_type.into_expression(checker, function.parameters.start())
780780
})
@@ -801,7 +801,7 @@ pub(crate) fn definition(
801801
let return_type = if is_stub_function(function, checker) {
802802
None
803803
} else {
804-
auto_return_type(function)
804+
auto_return_type(function, checker.semantic())
805805
.and_then(|return_type| {
806806
return_type.into_expression(checker, function.parameters.start())
807807
})
@@ -887,7 +887,7 @@ pub(crate) fn definition(
887887
let return_type = if is_stub_function(function, checker) {
888888
None
889889
} else {
890-
auto_return_type(function)
890+
auto_return_type(function, checker.semantic())
891891
.and_then(|return_type| {
892892
return_type
893893
.into_expression(checker, function.parameters.start())
@@ -922,7 +922,7 @@ pub(crate) fn definition(
922922
let return_type = if is_stub_function(function, checker) {
923923
None
924924
} else {
925-
auto_return_type(function)
925+
auto_return_type(function, checker.semantic())
926926
.and_then(|return_type| {
927927
return_type
928928
.into_expression(checker, function.parameters.start())

crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -775,3 +775,71 @@ help: Add return type annotation: `str | int`
775775
301 | case [1, 2, 3]:
776776
302 | return 1
777777
note: This is an unsafe fix and may change runtime behavior
778+
779+
ANN201 Missing return type annotation for public function `func`
780+
--> auto_return_type.py:308:5
781+
|
782+
307 | # Test cases for NotImplementedError - should not get NoReturn auto-fix
783+
308 | def func():
784+
| ^^^^
785+
309 | raise NotImplementedError
786+
|
787+
help: Add return type annotation
788+
789+
ANN205 Missing return type annotation for staticmethod `func`
790+
--> auto_return_type.py:314:9
791+
|
792+
312 | class Class:
793+
313 | @staticmethod
794+
314 | def func():
795+
| ^^^^
796+
315 | raise NotImplementedError
797+
|
798+
help: Add return type annotation
799+
800+
ANN206 Missing return type annotation for classmethod `method`
801+
--> auto_return_type.py:318:9
802+
|
803+
317 | @classmethod
804+
318 | def method(cls):
805+
| ^^^^^^
806+
319 | raise NotImplementedError
807+
|
808+
help: Add return type annotation
809+
810+
ANN201 Missing return type annotation for public function `instance_method`
811+
--> auto_return_type.py:321:9
812+
|
813+
319 | raise NotImplementedError
814+
320 |
815+
321 | def instance_method(self):
816+
| ^^^^^^^^^^^^^^^
817+
322 | raise NotImplementedError
818+
|
819+
help: Add return type annotation
820+
821+
ANN201 [*] Missing return type annotation for public function `func`
822+
--> auto_return_type.py:326:5
823+
|
824+
325 | # Test case: function that raises other exceptions should still get NoReturn
825+
326 | def func():
826+
| ^^^^
827+
327 | raise ValueError
828+
|
829+
help: Add return type annotation: `Never`
830+
214 | return 1
831+
215 |
832+
216 |
833+
- from typing import overload
834+
217 + from typing import overload, Never
835+
218 |
836+
219 |
837+
220 | @overload
838+
--------------------------------------------------------------------------------
839+
323 |
840+
324 |
841+
325 | # Test case: function that raises other exceptions should still get NoReturn
842+
- def func():
843+
326 + def func() -> Never:
844+
327 | raise ValueError
845+
note: This is an unsafe fix and may change runtime behavior

crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -879,3 +879,71 @@ help: Add return type annotation: `Union[str, int]`
879879
301 | case [1, 2, 3]:
880880
302 | return 1
881881
note: This is an unsafe fix and may change runtime behavior
882+
883+
ANN201 Missing return type annotation for public function `func`
884+
--> auto_return_type.py:308:5
885+
|
886+
307 | # Test cases for NotImplementedError - should not get NoReturn auto-fix
887+
308 | def func():
888+
| ^^^^
889+
309 | raise NotImplementedError
890+
|
891+
help: Add return type annotation
892+
893+
ANN205 Missing return type annotation for staticmethod `func`
894+
--> auto_return_type.py:314:9
895+
|
896+
312 | class Class:
897+
313 | @staticmethod
898+
314 | def func():
899+
| ^^^^
900+
315 | raise NotImplementedError
901+
|
902+
help: Add return type annotation
903+
904+
ANN206 Missing return type annotation for classmethod `method`
905+
--> auto_return_type.py:318:9
906+
|
907+
317 | @classmethod
908+
318 | def method(cls):
909+
| ^^^^^^
910+
319 | raise NotImplementedError
911+
|
912+
help: Add return type annotation
913+
914+
ANN201 Missing return type annotation for public function `instance_method`
915+
--> auto_return_type.py:321:9
916+
|
917+
319 | raise NotImplementedError
918+
320 |
919+
321 | def instance_method(self):
920+
| ^^^^^^^^^^^^^^^
921+
322 | raise NotImplementedError
922+
|
923+
help: Add return type annotation
924+
925+
ANN201 [*] Missing return type annotation for public function `func`
926+
--> auto_return_type.py:326:5
927+
|
928+
325 | # Test case: function that raises other exceptions should still get NoReturn
929+
326 | def func():
930+
| ^^^^
931+
327 | raise ValueError
932+
|
933+
help: Add return type annotation: `NoReturn`
934+
214 | return 1
935+
215 |
936+
216 |
937+
- from typing import overload
938+
217 + from typing import overload, NoReturn
939+
218 |
940+
219 |
941+
220 | @overload
942+
--------------------------------------------------------------------------------
943+
323 |
944+
324 |
945+
325 | # Test case: function that raises other exceptions should still get NoReturn
946+
- def func():
947+
326 + def func() -> NoReturn:
948+
327 | raise ValueError
949+
note: This is an unsafe fix and may change runtime behavior

crates/ruff_linter/src/rules/pylint/rules/invalid_bool_return.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,10 @@ pub(crate) fn invalid_bool_return(checker: &Checker, function_def: &ast::StmtFun
6060
}
6161

6262
// Determine the terminal behavior (i.e., implicit return, no return, etc.).
63-
let terminal = Terminal::from_function(function_def);
63+
let terminal = Terminal::from_function(function_def, checker.semantic());
6464

6565
// If every control flow path raises an exception, ignore the function.
66-
if terminal == Terminal::Raise {
66+
if terminal.is_always_raise() {
6767
return;
6868
}
6969

crates/ruff_linter/src/rules/pylint/rules/invalid_bytes_return.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,10 @@ pub(crate) fn invalid_bytes_return(checker: &Checker, function_def: &ast::StmtFu
6060
}
6161

6262
// Determine the terminal behavior (i.e., implicit return, no return, etc.).
63-
let terminal = Terminal::from_function(function_def);
63+
let terminal = Terminal::from_function(function_def, checker.semantic());
6464

6565
// If every control flow path raises an exception, ignore the function.
66-
if terminal == Terminal::Raise {
66+
if terminal.is_always_raise() {
6767
return;
6868
}
6969

crates/ruff_linter/src/rules/pylint/rules/invalid_hash_return.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,10 @@ pub(crate) fn invalid_hash_return(checker: &Checker, function_def: &ast::StmtFun
6464
}
6565

6666
// Determine the terminal behavior (i.e., implicit return, no return, etc.).
67-
let terminal = Terminal::from_function(function_def);
67+
let terminal = Terminal::from_function(function_def, checker.semantic());
6868

6969
// If every control flow path raises an exception, ignore the function.
70-
if terminal == Terminal::Raise {
70+
if terminal.is_always_raise() {
7171
return;
7272
}
7373

crates/ruff_linter/src/rules/pylint/rules/invalid_index_return.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,10 @@ pub(crate) fn invalid_index_return(checker: &Checker, function_def: &ast::StmtFu
6666
}
6767

6868
// Determine the terminal behavior (i.e., implicit return, no return, etc.).
69-
let terminal = Terminal::from_function(function_def);
69+
let terminal = Terminal::from_function(function_def, checker.semantic());
7070

7171
// If every control flow path raises an exception, ignore the function.
72-
if terminal == Terminal::Raise {
72+
if terminal.is_always_raise() {
7373
return;
7474
}
7575

crates/ruff_linter/src/rules/pylint/rules/invalid_length_return.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ pub(crate) fn invalid_length_return(checker: &Checker, function_def: &ast::StmtF
6565
}
6666

6767
// Determine the terminal behavior (i.e., implicit return, no return, etc.).
68-
let terminal = Terminal::from_function(function_def);
68+
let terminal = Terminal::from_function(function_def, checker.semantic());
6969

7070
// If every control flow path raises an exception, ignore the function.
71-
if terminal == Terminal::Raise {
71+
if terminal.is_always_raise() {
7272
return;
7373
}
7474

0 commit comments

Comments
 (0)