From cda2ff0b180d311a24cf545c0339ba8977437db3 Mon Sep 17 00:00:00 2001 From: Peter Pentchev Date: Fri, 10 Feb 2023 16:28:34 +0200 Subject: [PATCH] Handle functions that never return in RET503 (#2701) --- .../test/fixtures/flake8_return/RET503.py | 71 +++++++++++++++++++ crates/ruff/src/rules/flake8_return/rules.rs | 30 ++++++++ ...lake8_return__tests__RET503_RET503.py.snap | 70 +++++++++++++----- 3 files changed, 155 insertions(+), 16 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_return/RET503.py b/crates/ruff/resources/test/fixtures/flake8_return/RET503.py index 0cf189612f..e708d7d3e5 100644 --- a/crates/ruff/resources/test/fixtures/flake8_return/RET503.py +++ b/crates/ruff/resources/test/fixtures/flake8_return/RET503.py @@ -1,3 +1,11 @@ +import os +import posix +from posix import abort +import sys as std_sys + +import pytest +from pytest import xfail as py_xfail + ### # Errors ### @@ -39,6 +47,20 @@ def x(y): print() # error +# A nonexistent function +def func_unknown(x): + if x > 0: + return False + no_such_function() # error + + +# A function that does return the control +def func_no_noreturn(x): + if x > 0: + return False + print("", end="") # error + + ### # Non-errors ### @@ -123,3 +145,52 @@ def prompts(self, foo): for x in foo: yield x yield x + 1 + + +# Functions that never return +def noreturn__exit(x): + if x > 0: + return 1 + os._exit(0) + + +def noreturn_abort(x): + if x > 0: + return 1 + os.abort() + + +def noreturn_abort_2(): + if x > 0: + return 1 + abort() + + +def noreturn_exit(): + if x > 0: + return 1 + std_sys.exit(0) + + +def noreturn_pytest_exit(): + if x > 0: + return 1 + pytest.exit("oof") + + +def noreturn_pytest_fail(): + if x > 0: + return 1 + pytest.fail("oof") + + +def noreturn_pytest_skip(): + if x > 0: + return 1 + pytest.skip("oof") + + +def noreturn_pytest_xfail(): + if x > 0: + return 1 + py_xfail("oof") diff --git a/crates/ruff/src/rules/flake8_return/rules.rs b/crates/ruff/src/rules/flake8_return/rules.rs index b406eb74b9..f2703beaa8 100644 --- a/crates/ruff/src/rules/flake8_return/rules.rs +++ b/crates/ruff/src/rules/flake8_return/rules.rs @@ -165,6 +165,30 @@ fn implicit_return_value(checker: &mut Checker, stack: &Stack) { } } +const NORETURN_FUNCS: &[&[&str]] = &[ + // builtins + &["", "exit"], + // stdlib + &["os", "_exit"], + &["os", "abort"], + &["posix", "abort"], + &["sys", "exit"], + // third-party modules + &["pytest", "exit"], + &["pytest", "fail"], + &["pytest", "skip"], + &["pytest", "xfail"], +]; + +/// Return `true` if the `func` is a known function that never returns. +fn is_noreturn_func(checker: &Checker, func: &Expr) -> bool { + checker.resolve_call_path(func).map_or(false, |call_path| { + NORETURN_FUNCS + .iter() + .any(|target| call_path.as_slice() == *target) + }) +} + /// RET503 fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) { match &last_stmt.node { @@ -208,6 +232,12 @@ fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) { | StmtKind::While { .. } | StmtKind::Raise { .. } | StmtKind::Try { .. } => {} + StmtKind::Expr { value, .. } + if matches!( + &value.node, + ExprKind::Call { func, .. } + if is_noreturn_func(checker, func) + ) => {} _ => { let mut diagnostic = Diagnostic::new(ImplicitReturn, Range::from_located(last_stmt)); if checker.patch(diagnostic.kind.rule()) { diff --git a/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET503_RET503.py.snap b/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET503_RET503.py.snap index 7b2ce1e456..1398784d1c 100644 --- a/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET503_RET503.py.snap +++ b/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET503_RET503.py.snap @@ -5,78 +5,116 @@ expression: diagnostics - kind: ImplicitReturn: ~ location: - row: 7 + row: 15 column: 4 end_location: - row: 8 + row: 16 column: 16 fix: ~ parent: ~ - kind: ImplicitReturn: ~ location: - row: 14 + row: 22 column: 8 end_location: - row: 14 + row: 22 column: 15 fix: content: - " return None" - "" location: - row: 15 + row: 23 column: 0 end_location: - row: 15 + row: 23 column: 0 parent: ~ - kind: ImplicitReturn: ~ location: - row: 23 + row: 31 column: 4 end_location: - row: 23 + row: 31 column: 11 fix: content: - " return None" - "" location: - row: 24 + row: 32 column: 0 end_location: - row: 24 + row: 32 column: 0 parent: ~ - kind: ImplicitReturn: ~ location: - row: 29 + row: 37 column: 8 end_location: - row: 30 + row: 38 column: 20 fix: ~ parent: ~ - kind: ImplicitReturn: ~ location: - row: 39 + row: 47 column: 8 end_location: - row: 39 + row: 47 column: 15 fix: content: - " return None" - "" location: - row: 40 + row: 48 column: 0 end_location: - row: 40 + row: 48 + column: 0 + parent: ~ +- kind: + ImplicitReturn: ~ + location: + row: 54 + column: 4 + end_location: + row: 54 + column: 22 + fix: + content: + - " return None" + - "" + location: + row: 55 + column: 0 + end_location: + row: 55 + column: 0 + parent: ~ +- kind: + ImplicitReturn: ~ + location: + row: 61 + column: 4 + end_location: + row: 61 + column: 21 + fix: + content: + - " return None" + - "" + location: + row: 62 + column: 0 + end_location: + row: 62 column: 0 parent: ~