Allow NoReturn-like functions for __str__, __len__, etc. (#11017)

## Summary If the method always raises, we shouldn't raise a diagnostic for "returning a value of the wrong type". Closes #11016.
astral-sh · Apr 18, 2024 · 33529c0 · 33529c0
1 parent e751b4e
commit 33529c0
Show file tree

Hide file tree

Showing 11 changed files with 103 additions and 57 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_bytes.py
@@ -21,12 +21,6 @@ def __bytes__(self):
         print("ruff")  # [invalid-bytes-return]
 
 
-class BytesWrongRaise:
-    def __bytes__(self):
-        print("raise some error")
-        raise NotImplementedError  # [invalid-bytes-return]
-
-
 # TODO: Once Ruff has better type checking
 def return_bytes():
     return "some string"
@@ -63,3 +57,9 @@ def __bytes__(self):
 class Bytes5:
     def __bytes__(self):
         raise NotImplementedError
+
+
+class Bytes6:
+    def __bytes__(self):
+        print("raise some error")
+        raise NotImplementedError
diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_length.py
@@ -26,12 +26,6 @@ def __len__(self):
         return -42  # [invalid-length-return]
 
 
-class LengthWrongRaise:
-    def __len__(self):
-        print("raise some error")
-        raise NotImplementedError  # [invalid-length-return]
-
-
 # TODO: Once Ruff has better type checking
 def return_int():
     return "3"
@@ -68,3 +62,9 @@ def __len__(self):
 class Length5:
     def __len__(self):
         raise NotImplementedError
+
+
+class Length6:
+    def __len__(self):
+        print("raise some error")
+        raise NotImplementedError
diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_str.py b/crates/ruff_linter/resources/test/fixtures/pylint/invalid_return_type_str.py
@@ -47,3 +47,14 @@ def __str__(self):
 
 class Str3:
     def __str__(self): ...
+
+
+class Str4:
+    def __str__(self):
+        raise RuntimeError("__str__ not allowed")
+
+
+class Str5:
+    def __str__(self):  # PLE0307 (returns None if x <= 0)
+        if x > 0:
+            raise RuntimeError("__str__ not allowed")
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
@@ -5,6 +5,7 @@ use ruff_python_ast::identifier::Identifier;
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_ast::{self as ast};
 use ruff_python_semantic::analyze::function_type::is_stub;
+use ruff_python_semantic::analyze::terminal::Terminal;
 use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType};
 use ruff_text_size::Ranged;
 
@@ -43,7 +44,7 @@ impl Violation for InvalidBoolReturnType {
     }
 }
 
-/// E0307
+/// PLE0304
 pub(crate) fn invalid_bool_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) {
     if function_def.name.as_str() != "__bool__" {
         return;
@@ -57,19 +58,29 @@ pub(crate) fn invalid_bool_return(checker: &mut Checker, function_def: &ast::Stm
         return;
     }
 
-    let returns = {
-        let mut visitor = ReturnStatementVisitor::default();
-        visitor.visit_body(&function_def.body);
-        visitor.returns
-    };
+    // Determine the terminal behavior (i.e., implicit return, no return, etc.).
+    let terminal = Terminal::from_function(function_def);
+
+    // If every control flow path raises an exception, ignore the function.
+    if terminal == Terminal::Raise {
+        return;
+    }
 
-    if returns.is_empty() {
+    // If there are no return statements, add a diagnostic.
+    if terminal == Terminal::Implicit {
         checker.diagnostics.push(Diagnostic::new(
             InvalidBoolReturnType,
             function_def.identifier(),
         ));
+        return;
     }
 
+    let returns = {
+        let mut visitor = ReturnStatementVisitor::default();
+        visitor.visit_body(&function_def.body);
+        visitor.returns
+    };
+
     for stmt in returns {
         if let Some(value) = stmt.value.as_deref() {
             if !matches!(

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
@@ -5,6 +5,7 @@ use ruff_python_ast::identifier::Identifier;
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_ast::{self as ast};
 use ruff_python_semantic::analyze::function_type::is_stub;
+use ruff_python_semantic::analyze::terminal::Terminal;
 use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
 use ruff_text_size::Ranged;
 
@@ -43,7 +44,7 @@ impl Violation for InvalidBytesReturnType {
     }
 }
 
-/// E0308
+/// PLE0308
 pub(crate) fn invalid_bytes_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef) {
     if function_def.name.as_str() != "__bytes__" {
         return;
@@ -57,19 +58,29 @@ pub(crate) fn invalid_bytes_return(checker: &mut Checker, function_def: &ast::St
         return;
     }
 
-    let returns = {
-        let mut visitor = ReturnStatementVisitor::default();
-        visitor.visit_body(&function_def.body);
-        visitor.returns
-    };
+    // Determine the terminal behavior (i.e., implicit return, no return, etc.).
+    let terminal = Terminal::from_function(function_def);
+
+    // If every control flow path raises an exception, ignore the function.
+    if terminal == Terminal::Raise {
+        return;
+    }
 
-    if returns.is_empty() {
+    // If there are no return statements, add a diagnostic.
+    if terminal == Terminal::Implicit {
         checker.diagnostics.push(Diagnostic::new(
             InvalidBytesReturnType,
             function_def.identifier(),
         ));
+        return;
     }
 
+    let returns = {
+        let mut visitor = ReturnStatementVisitor::default();
+        visitor.visit_body(&function_def.body);
+        visitor.returns
+    };
+
     for stmt in returns {
         if let Some(value) = stmt.value.as_deref() {
             if !matches!(

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
@@ -5,6 +5,7 @@ use ruff_python_ast::identifier::Identifier;
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_ast::{self as ast, Expr};
 use ruff_python_semantic::analyze::function_type::is_stub;
+use ruff_python_semantic::analyze::terminal::Terminal;
 use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType};
 use ruff_text_size::Ranged;
 
@@ -63,19 +64,29 @@ pub(crate) fn invalid_length_return(checker: &mut Checker, function_def: &ast::S
         return;
     }
 
-    let returns = {
-        let mut visitor = ReturnStatementVisitor::default();
-        visitor.visit_body(&function_def.body);
-        visitor.returns
-    };
+    // Determine the terminal behavior (i.e., implicit return, no return, etc.).
+    let terminal = Terminal::from_function(function_def);
+
+    // If every control flow path raises an exception, ignore the function.
+    if terminal == Terminal::Raise {
+        return;
+    }
 
-    if returns.is_empty() {
+    // If there are no return statements, add a diagnostic.
+    if terminal == Terminal::Implicit {
         checker.diagnostics.push(Diagnostic::new(
             InvalidLengthReturnType,
             function_def.identifier(),
         ));
+        return;
     }
 
+    let returns = {
+        let mut visitor = ReturnStatementVisitor::default();
+        visitor.visit_body(&function_def.body);
+        visitor.returns
+    };
+
     for stmt in returns {
         if let Some(value) = stmt.value.as_deref() {
             if is_negative_integer(value)

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
@@ -5,6 +5,7 @@ use ruff_python_ast::identifier::Identifier;
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_ast::{self as ast};
 use ruff_python_semantic::analyze::function_type::is_stub;
+use ruff_python_semantic::analyze::terminal::Terminal;
 use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
 use ruff_text_size::Ranged;
 
@@ -57,19 +58,29 @@ pub(crate) fn invalid_str_return(checker: &mut Checker, function_def: &ast::Stmt
         return;
     }
 
-    let returns = {
-        let mut visitor = ReturnStatementVisitor::default();
-        visitor.visit_body(&function_def.body);
-        visitor.returns
-    };
+    // Determine the terminal behavior (i.e., implicit return, no return, etc.).
+    let terminal = Terminal::from_function(function_def);
+
+    // If every control flow path raises an exception, ignore the function.
+    if terminal == Terminal::Raise {
+        return;
+    }
 
-    if returns.is_empty() {
+    // If there are no return statements, add a diagnostic.
+    if terminal == Terminal::Implicit {
         checker.diagnostics.push(Diagnostic::new(
             InvalidStrReturnType,
             function_def.identifier(),
         ));
+        return;
     }
 
+    let returns = {
+        let mut visitor = ReturnStatementVisitor::default();
+        visitor.visit_body(&function_def.body);
+        visitor.returns
+    };
+
     for stmt in returns {
         if let Some(value) = stmt.value.as_deref() {
             if !matches!(

diff --git a/crates/ruff_linter/src/rules/pylint/rules/non_slot_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/non_slot_assignment.rs
@@ -59,7 +59,7 @@ impl Violation for NonSlotAssignment {
     }
 }
 
-/// E0237
+/// PLE0237
 pub(crate) fn non_slot_assignment(checker: &mut Checker, class_def: &ast::StmtClassDef) {
     let semantic = checker.semantic();
 

diff --git a/...t/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap b/...t/snapshots/ruff_linter__rules__pylint__tests__PLE0303_invalid_return_type_length.py.snap
@@ -40,12 +40,3 @@ invalid_return_type_length.py:26:16: PLE0303 `__len__` does not return a non-neg
 26 |         return -42  # [invalid-length-return]
    |                ^^^ PLE0303
    |
-
-invalid_return_type_length.py:30:9: PLE0303 `__len__` does not return a non-negative integer
-   |
-29 | class LengthWrongRaise:
-30 |     def __len__(self):
-   |         ^^^^^^^ PLE0303
-31 |         print("raise some error")
-32 |         raise NotImplementedError  # [invalid-length-return]
-   |
diff --git a/...lint/snapshots/ruff_linter__rules__pylint__tests__PLE0307_invalid_return_type_str.py.snap b/...lint/snapshots/ruff_linter__rules__pylint__tests__PLE0307_invalid_return_type_str.py.snap
@@ -32,3 +32,12 @@ invalid_return_type_str.py:21:16: PLE0307 `__str__` does not return `str`
 21 |         return False
    |                ^^^^^ PLE0307
    |
+
+invalid_return_type_str.py:58:9: PLE0307 `__str__` does not return `str`
+   |
+57 | class Str5:
+58 |     def __str__(self):  # PLE0307 (returns None if x <= 0)
+   |         ^^^^^^^ PLE0307
+59 |         if x > 0:
+60 |             raise RuntimeError("__str__ not allowed")
+   |
diff --git a/...nt/snapshots/ruff_linter__rules__pylint__tests__PLE0308_invalid_return_type_bytes.py.snap b/...nt/snapshots/ruff_linter__rules__pylint__tests__PLE0308_invalid_return_type_bytes.py.snap
@@ -32,12 +32,3 @@ invalid_return_type_bytes.py:20:9: PLE0308 `__bytes__` does not return `bytes`
    |         ^^^^^^^^^ PLE0308
 21 |         print("ruff")  # [invalid-bytes-return]
    |
-
-invalid_return_type_bytes.py:25:9: PLE0308 `__bytes__` does not return `bytes`
-   |
-24 | class BytesWrongRaise:
-25 |     def __bytes__(self):
-   |         ^^^^^^^^^ PLE0308
-26 |         print("raise some error")
-27 |         raise NotImplementedError  # [invalid-bytes-return]
-   |