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 cc6ca87fc189a3..1fa9454ffd39e8 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 @@ -264,3 +264,41 @@ def func(x: int): if x > 0: return 1 raise ValueError + + +def func(x: int): + if x > 5: + raise ValueError + else: + pass + + +def func(x: int): + if x > 5: + raise ValueError + elif x > 10: + pass + + +def func(x: int): + if x > 5: + raise ValueError + elif x > 10: + return 5 + + +def func(): + try: + return 5 + except: + pass + + raise ValueError + + +def func(x: int): + match x: + case [1, 2, 3]: + return 1 + case y: + return "foo" diff --git a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs index ad1c6d615bbb64..873b1859820c53 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/helpers.rs @@ -89,7 +89,7 @@ pub(crate) fn auto_return_type(function: &ast::StmtFunctionDef) -> Option 0: // return 1 // ``` - if terminal == Terminal::ConditionalReturn || terminal == Terminal::None { + if terminal.has_implicit_return() { return_type = return_type.union(ResolvedPythonType::Atom(PythonType::None)); } diff --git a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap index e2574509aa676a..c2be32c1fa9950 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap +++ b/crates/ruff_linter/src/rules/flake8_annotations/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type.snap @@ -263,14 +263,14 @@ auto_return_type.py:82:5: ANN201 [*] Missing return type annotation for public f 83 | match x: 84 | case [1, 2, 3]: | - = help: Add return type annotation: `str | int` + = help: Add return type annotation: `str | int | None` ℹ Unsafe fix 79 79 | return 1 80 80 | 81 81 | 82 |-def func(x: int): - 82 |+def func(x: int) -> str | int: + 82 |+def func(x: int) -> str | int | None: 83 83 | match x: 84 84 | case [1, 2, 3]: 85 85 | return 1 @@ -396,14 +396,14 @@ auto_return_type.py:137:5: ANN201 [*] Missing return type annotation for public 138 | try: 139 | return 1 | - = help: Add return type annotation: `int` + = help: Add return type annotation: `int | None` ℹ Unsafe fix 134 134 | return 2 135 135 | 136 136 | 137 |-def func(x: int): - 137 |+def func(x: int) -> int: + 137 |+def func(x: int) -> int | None: 138 138 | try: 139 139 | return 1 140 140 | except: @@ -674,4 +674,99 @@ auto_return_type.py:262:5: ANN201 [*] Missing return type annotation for public 264 264 | if x > 0: 265 265 | return 1 +auto_return_type.py:269:5: ANN201 [*] Missing return type annotation for public function `func` + | +269 | def func(x: int): + | ^^^^ ANN201 +270 | if x > 5: +271 | raise ValueError + | + = help: Add return type annotation: `None` + +ℹ Unsafe fix +266 266 | raise ValueError +267 267 | +268 268 | +269 |-def func(x: int): + 269 |+def func(x: int) -> None: +270 270 | if x > 5: +271 271 | raise ValueError +272 272 | else: + +auto_return_type.py:276:5: ANN201 [*] Missing return type annotation for public function `func` + | +276 | def func(x: int): + | ^^^^ ANN201 +277 | if x > 5: +278 | raise ValueError + | + = help: Add return type annotation: `None` + +ℹ Unsafe fix +273 273 | pass +274 274 | +275 275 | +276 |-def func(x: int): + 276 |+def func(x: int) -> None: +277 277 | if x > 5: +278 278 | raise ValueError +279 279 | elif x > 10: + +auto_return_type.py:283:5: ANN201 [*] Missing return type annotation for public function `func` + | +283 | def func(x: int): + | ^^^^ ANN201 +284 | if x > 5: +285 | raise ValueError + | + = help: Add return type annotation: `int | None` + +ℹ Unsafe fix +280 280 | pass +281 281 | +282 282 | +283 |-def func(x: int): + 283 |+def func(x: int) -> int | None: +284 284 | if x > 5: +285 285 | raise ValueError +286 286 | elif x > 10: + +auto_return_type.py:290:5: ANN201 [*] Missing return type annotation for public function `func` + | +290 | def func(): + | ^^^^ ANN201 +291 | try: +292 | return 5 + | + = help: Add return type annotation: `int` + +ℹ Unsafe fix +287 287 | return 5 +288 288 | +289 289 | +290 |-def func(): + 290 |+def func() -> int: +291 291 | try: +292 292 | return 5 +293 293 | except: + +auto_return_type.py:299:5: ANN201 [*] Missing return type annotation for public function `func` + | +299 | def func(x: int): + | ^^^^ ANN201 +300 | match x: +301 | case [1, 2, 3]: + | + = help: Add return type annotation: `str | int` + +ℹ Unsafe fix +296 296 | raise ValueError +297 297 | +298 298 | +299 |-def func(x: int): + 299 |+def func(x: int) -> str | int: +300 300 | match x: +301 301 | case [1, 2, 3]: +302 302 | return 1 + 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 4dfd66c8d4b37e..4cb5bbfb615fa1 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 @@ -293,7 +293,7 @@ auto_return_type.py:82:5: ANN201 [*] Missing return type annotation for public f 83 | match x: 84 | case [1, 2, 3]: | - = help: Add return type annotation: `Union[str | int]` + = help: Add return type annotation: `Union[str | int | None]` ℹ Unsafe fix 1 |+from typing import Union @@ -305,7 +305,7 @@ auto_return_type.py:82:5: ANN201 [*] Missing return type annotation for public f 80 81 | 81 82 | 82 |-def func(x: int): - 83 |+def func(x: int) -> Union[str | int]: + 83 |+def func(x: int) -> Union[str | int | None]: 83 84 | match x: 84 85 | case [1, 2, 3]: 85 86 | return 1 @@ -446,17 +446,22 @@ auto_return_type.py:137:5: ANN201 [*] Missing return type annotation for public 138 | try: 139 | return 1 | - = help: Add return type annotation: `int` + = help: Add return type annotation: `Optional[int]` ℹ Unsafe fix -134 134 | return 2 -135 135 | -136 136 | + 1 |+from typing import Optional +1 2 | def func(): +2 3 | return 1 +3 4 | +-------------------------------------------------------------------------------- +134 135 | return 2 +135 136 | +136 137 | 137 |-def func(x: int): - 137 |+def func(x: int) -> int: -138 138 | try: -139 139 | return 1 -140 140 | except: + 138 |+def func(x: int) -> Optional[int]: +138 139 | try: +139 140 | return 1 +140 141 | except: auto_return_type.py:146:5: ANN201 [*] Missing return type annotation for public function `func` | @@ -755,4 +760,117 @@ auto_return_type.py:262:5: ANN201 [*] Missing return type annotation for public 264 264 | if x > 0: 265 265 | return 1 +auto_return_type.py:269:5: ANN201 [*] Missing return type annotation for public function `func` + | +269 | def func(x: int): + | ^^^^ ANN201 +270 | if x > 5: +271 | raise ValueError + | + = help: Add return type annotation: `None` + +ℹ Unsafe fix +266 266 | raise ValueError +267 267 | +268 268 | +269 |-def func(x: int): + 269 |+def func(x: int) -> None: +270 270 | if x > 5: +271 271 | raise ValueError +272 272 | else: + +auto_return_type.py:276:5: ANN201 [*] Missing return type annotation for public function `func` + | +276 | def func(x: int): + | ^^^^ ANN201 +277 | if x > 5: +278 | raise ValueError + | + = help: Add return type annotation: `None` + +ℹ Unsafe fix +273 273 | pass +274 274 | +275 275 | +276 |-def func(x: int): + 276 |+def func(x: int) -> None: +277 277 | if x > 5: +278 278 | raise ValueError +279 279 | elif x > 10: + +auto_return_type.py:283:5: ANN201 [*] Missing return type annotation for public function `func` + | +283 | def func(x: int): + | ^^^^ ANN201 +284 | if x > 5: +285 | raise ValueError + | + = help: Add return type annotation: `Optional[int]` + +ℹ Unsafe fix +214 214 | return 1 +215 215 | +216 216 | +217 |-from typing import overload + 217 |+from typing import overload, Optional +218 218 | +219 219 | +220 220 | @overload +-------------------------------------------------------------------------------- +280 280 | pass +281 281 | +282 282 | +283 |-def func(x: int): + 283 |+def func(x: int) -> Optional[int]: +284 284 | if x > 5: +285 285 | raise ValueError +286 286 | elif x > 10: + +auto_return_type.py:290:5: ANN201 [*] Missing return type annotation for public function `func` + | +290 | def func(): + | ^^^^ ANN201 +291 | try: +292 | return 5 + | + = help: Add return type annotation: `int` + +ℹ Unsafe fix +287 287 | return 5 +288 288 | +289 289 | +290 |-def func(): + 290 |+def func() -> int: +291 291 | try: +292 292 | return 5 +293 293 | except: + +auto_return_type.py:299:5: ANN201 [*] Missing return type annotation for public function `func` + | +299 | def func(x: int): + | ^^^^ ANN201 +300 | match x: +301 | case [1, 2, 3]: + | + = help: Add return type annotation: `Union[str | int]` + +ℹ Unsafe fix +214 214 | return 1 +215 215 | +216 216 | +217 |-from typing import overload + 217 |+from typing import overload, Union +218 218 | +219 219 | +220 220 | @overload +-------------------------------------------------------------------------------- +296 296 | raise ValueError +297 297 | +298 298 | +299 |-def func(x: int): + 299 |+def func(x: int) -> Union[str | int]: +300 300 | match x: +301 301 | case [1, 2, 3]: +302 302 | return 1 + diff --git a/crates/ruff_python_semantic/src/analyze/terminal.rs b/crates/ruff_python_semantic/src/analyze/terminal.rs index f5642876d22080..acb9483ba24abf 100644 --- a/crates/ruff_python_semantic/src/analyze/terminal.rs +++ b/crates/ruff_python_semantic/src/analyze/terminal.rs @@ -2,14 +2,16 @@ use ruff_python_ast::{self as ast, ExceptHandler, Stmt}; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Terminal { - /// There is no known terminal (e.g., an implicit return). + /// There is no known terminal. None, + /// There is an implicit return (e.g., a path that doesn't return). + Implicit, /// Every path through the function ends with a `raise` statement. Raise, /// No path through the function ends with a `return` statement. Return, /// Every path through the function ends with a `return` or `raise` statement. - Explicit, + RaiseOrReturn, /// At least one path through the function ends with a `return` statement. ConditionalReturn, } @@ -20,6 +22,19 @@ impl Terminal { Self::from_body(&function.body) } + /// Returns `true` if the [`Terminal`] behavior includes at least one `return` path. + pub fn has_any_return(self) -> bool { + matches!( + self, + Self::Return | Self::RaiseOrReturn | Self::ConditionalReturn + ) + } + + /// Returns `true` if the [`Terminal`] behavior includes at least one implicit `return` path. + pub fn has_implicit_return(self) -> bool { + matches!(self, Self::None | Self::Implicit | Self::ConditionalReturn) + } + /// Returns the [`Terminal`] behavior of the body, if it can be determined. fn from_body(stmts: &[Stmt]) -> Terminal { let mut terminal = Terminal::None; @@ -32,10 +47,10 @@ impl Terminal { continue; } - terminal = terminal.union(Self::from_body(body)); + terminal = terminal.and_then(Self::from_body(body)); if !sometimes_breaks(body) { - terminal = terminal.union(Self::from_body(orelse)); + terminal = terminal.and_then(Self::from_body(orelse)); } } Stmt::If(ast::StmtIf { @@ -43,7 +58,7 @@ impl Terminal { elif_else_clauses, .. }) => { - let branch_terminal = Terminal::combine( + let branch_terminal = Terminal::branches( std::iter::once(Self::from_body(body)).chain( elif_else_clauses .iter() @@ -55,96 +70,176 @@ impl Terminal { // `else`)... if elif_else_clauses.iter().any(|clause| clause.test.is_none()) { // And all branches return, then the `if` statement returns. - terminal = terminal.union(branch_terminal); - } else if branch_terminal.has_return() { + terminal = terminal.and_then(branch_terminal); + } else if branch_terminal.has_any_return() { // Otherwise, if any branch returns, we know this can't be a // non-returning function. - terminal = terminal.union(Terminal::ConditionalReturn); + terminal = terminal.and_then(Terminal::ConditionalReturn); } } Stmt::Match(ast::StmtMatch { cases, .. }) => { - // Note: we assume the `match` is exhaustive. - terminal = terminal.union(Terminal::combine( + let branch_terminal = terminal.and_then(Terminal::branches( cases.iter().map(|case| Self::from_body(&case.body)), )); + + // If the `match` is known to be exhaustive (by way of including a wildcard + // pattern)... + if cases.iter().any(is_wildcard) { + // And all branches return, then the `match` statement returns. + terminal = terminal.and_then(branch_terminal); + } else { + // Otherwise, if any branch returns, we know this can't be a + // non-returning function. + if branch_terminal.has_any_return() { + terminal = terminal.and_then(Terminal::ConditionalReturn); + } + } } Stmt::Try(ast::StmtTry { + body, handlers, orelse, finalbody, .. }) => { - // If the `finally` block returns, the `try` block must also return. - terminal = terminal.union(Self::from_body(finalbody)); - - // If the else block and all the handlers return, the `try` block must also - // return. - let branch_terminal = - Terminal::combine(std::iter::once(Self::from_body(orelse)).chain( - handlers.iter().map(|handler| { - let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { - body, - .. - }) = handler; - Self::from_body(body) - }), - )); + // If the body returns, then this can't be a non-returning function. We assume + // 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); + 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)); + + let branch_terminal = Terminal::branches(handlers.iter().map(|handler| { + let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + body, + .. + }) = handler; + Self::from_body(body) + })); if orelse.is_empty() { - // If there's no `else`, we may fall through. - if branch_terminal.has_return() { - terminal = terminal.union(Terminal::ConditionalReturn); + // If there's no `else`, we may fall through, so only mark that this can't + // be a non-returning function if any of the branches return. + if branch_terminal.has_any_return() { + terminal = terminal.and_then(Terminal::ConditionalReturn); } } else { - // If there's an `else`, we may not fall through. - terminal = terminal.union(branch_terminal); + // 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))); } } Stmt::With(ast::StmtWith { body, .. }) => { - terminal = terminal.union(Self::from_body(body)); + terminal = terminal.and_then(Self::from_body(body)); } Stmt::Return(_) => { - terminal = terminal.union(Terminal::Explicit); + terminal = terminal.and_then(Terminal::RaiseOrReturn); } Stmt::Raise(_) => { - terminal = terminal.union(Terminal::Raise); + terminal = terminal.and_then(Terminal::Raise); } _ => {} } } - terminal + + match terminal { + Terminal::None => Terminal::Implicit, + _ => terminal, + } } - /// Returns `true` if the [`Terminal`] behavior includes at least one `return` path. - fn has_return(self) -> bool { - matches!( - self, - Self::Return | Self::Explicit | Self::ConditionalReturn - ) + /// Combine two [`Terminal`] operators, with one appearing after the other. + fn and_then(self, other: Self) -> Self { + match (self, other) { + // If one of the operators is `None`, the result is the other operator. + (Self::None, other) => other, + (other, Self::None) => other, + + // If one of the operators is `Implicit`, the result is the other operator. + (Self::Implicit, other) => other, + (other, Self::Implicit) => other, + + // 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 `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 `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, + + // All paths through the function end with a `return` or `raise` statement. + (Self::Return, Self::Raise) => Self::RaiseOrReturn, + + // All paths through the function end with a `return` or `raise` statement. + (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, + (_, Self::RaiseOrReturn) => Self::RaiseOrReturn, + } } - /// Combine two [`Terminal`] operators. - fn union(self, other: Self) -> Self { + /// Combine two [`Terminal`] operators from different branches. + fn branch(self, other: Self) -> Self { match (self, other) { + // If one of the operators is `None`, the result is the other operator. (Self::None, other) => other, (other, Self::None) => other, - (Self::Explicit, _) => Self::Explicit, - (_, Self::Explicit) => Self::Explicit, + + // 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::Return) => Self::ConditionalReturn, + (Self::Return, Self::Implicit) => Self::ConditionalReturn, + (Self::Implicit, Self::RaiseOrReturn) => Self::ConditionalReturn, + (Self::RaiseOrReturn, Self::Implicit) => Self::ConditionalReturn, + (Self::Implicit, Self::ConditionalReturn) => Self::ConditionalReturn, + (Self::ConditionalReturn, Self::Implicit) => Self::ConditionalReturn, + + // If both operators are conditional returns, the result is a conditional return. (Self::ConditionalReturn, Self::ConditionalReturn) => Self::ConditionalReturn, - (Self::Raise, Self::ConditionalReturn) => Self::Explicit, - (Self::ConditionalReturn, Self::Raise) => Self::Explicit, + + (Self::Raise, Self::ConditionalReturn) => Self::RaiseOrReturn, + (Self::ConditionalReturn, Self::Raise) => 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 `return` statement. (Self::Return, Self::Return) => Self::Return, - (Self::Raise, Self::Return) => Self::Explicit, - (Self::Return, Self::Raise) => Self::Explicit, + // All paths through the function end with a `return` or `raise` statement. + (Self::Raise, Self::Return) => Self::RaiseOrReturn, + // All paths through the function end with a `return` or `raise` statement. + (Self::Return, Self::Raise) => Self::RaiseOrReturn, + // All paths through the function end with a `return` or `raise` statement. + (Self::RaiseOrReturn, _) => Self::RaiseOrReturn, + (_, Self::RaiseOrReturn) => Self::RaiseOrReturn, } } /// Combine a series of [`Terminal`] operators. - fn combine(iter: impl Iterator) -> Terminal { - iter.fold(Terminal::None, Self::union) + fn branches(iter: impl Iterator) -> Terminal { + iter.fold(Terminal::None, Terminal::branch) } } @@ -153,7 +248,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_return() { + if Terminal::from_body(body).has_any_return() { return false; } if sometimes_breaks(orelse) { @@ -161,7 +256,7 @@ fn sometimes_breaks(stmts: &[Stmt]) -> bool { } } Stmt::While(ast::StmtWhile { body, orelse, .. }) => { - if Terminal::from_body(body).has_return() { + if Terminal::from_body(body).has_any_return() { return false; } if sometimes_breaks(orelse) { @@ -232,3 +327,24 @@ fn always_breaks(stmts: &[Stmt]) -> bool { } false } + +/// Returns true if the [`MatchCase`] is a wildcard pattern. +fn is_wildcard(pattern: &ast::MatchCase) -> bool { + /// Returns true if the [`Pattern`] is a wildcard pattern. + fn is_wildcard_pattern(pattern: &ast::Pattern) -> bool { + match pattern { + ast::Pattern::MatchValue(_) + | ast::Pattern::MatchSingleton(_) + | ast::Pattern::MatchSequence(_) + | ast::Pattern::MatchMapping(_) + | ast::Pattern::MatchClass(_) + | ast::Pattern::MatchStar(_) => false, + ast::Pattern::MatchAs(ast::PatternMatchAs { pattern, .. }) => pattern.is_none(), + ast::Pattern::MatchOr(ast::PatternMatchOr { patterns, .. }) => { + patterns.iter().all(is_wildcard_pattern) + } + } + } + + pattern.guard.is_none() && is_wildcard_pattern(&pattern.pattern) +}