diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index 894363e776..47ad622a09 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -464,6 +464,8 @@ pub enum ErrorKind { UnboundName, /// An error caused by a keyword argument used in the wrong place. UnexpectedKeyword, + /// A function's inferred return type is `Never`, contrary to its annotation. + UnexpectedNever, /// An error caused by passing a positional argument for a keyword-only parameter. UnexpectedPositionalArgument, /// Attempting to use a type checker directive without importing it from `typing`. @@ -661,6 +663,7 @@ impl ErrorKind { ErrorKind::ImplicitlyDefinedAttribute => Severity::Ignore, ErrorKind::IncompatibleComparison => Severity::Ignore, ErrorKind::InstanceVarAssign => Severity::Ignore, + ErrorKind::UnexpectedNever => Severity::Warn, ErrorKind::InvalidAbstractMethod => Severity::Ignore, ErrorKind::InvalidCast => Severity::Ignore, ErrorKind::InvalidDecorator => Severity::Warn, diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index db6ecdabd7..ac62f50155 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -13,6 +13,7 @@ use pyrefly_graph::index::Idx; use pyrefly_python::ast::Ast; use pyrefly_python::dunder; use pyrefly_python::module_name::ModuleName; +use pyrefly_python::module_path::ModuleStyle; use pyrefly_python::short_identifier::ShortIdentifier; use pyrefly_python::sys_info::SysInfo; use pyrefly_types::dimension::Int; @@ -3152,7 +3153,9 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { implicit_return, is_async, is_generator, - has_explicit_return, + returns, + function, + check_never_return, } => { let annotation = self.get_idx(*annotation).annotation.get_type().clone(); let implicit_return = self.get_idx(*implicit_return); @@ -3161,10 +3164,50 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { &annotation, *is_async, *is_generator, - *has_explicit_return, + !returns.is_empty(), range, errors, ); + if *check_never_return + && !is_generator + && !annotation.is_never() + && !annotation.is_none() + && implicit_return.ty().is_never() + && returns.iter().all(|key| self.get_idx(*key).ty().is_never()) + { + let function = self.get_idx(*function); + let flags = &function.metadata.flags; + // Overrides keep the parent's return contract even when this body never returns. + let is_override = flags.is_override + || function.defining_cls.as_ref().is_some_and(|cls| { + self.get_mro_for_class(cls) + .ancestors(self.stdlib) + .any(|base| { + self.get_class_member( + base.class_object(), + &function.metadata.kind.function_name(), + ) + .is_some() + }) + }); + if !flags.facts().is_stub() + && !flags.body_kind.is_placeholder_or_trivial() + && !flags.is_overload + && !flags.is_abstract_method + && flags.module_style != ModuleStyle::Interface + && !is_override + { + self.error( + errors, + range, + ErrorKind::UnexpectedNever, + format!( + "Function declared to return `{}` but can never return", + self.for_display(annotation.clone()) + ), + ); + } + } } BindingExpect::MatchExhaustiveness { subject_idx, diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index 11d43233c5..3decdd728b 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -1300,14 +1300,16 @@ pub enum BindingExpect { existing: Idx, name: Name, }, - /// Validate a function implementation's implicit return without making the - /// published return type depend on the function body. + /// Validate a function's return annotation without making the published + /// return type depend on the function body. ValidateImplicitReturn { annotation: Idx, implicit_return: Idx, is_async: bool, is_generator: bool, - has_explicit_return: bool, + returns: Box<[Idx]>, + function: Idx, + check_never_return: bool, }, /// Expression used in a boolean context (`bool()`, `if`, or `while`) Bool(Expr), diff --git a/pyrefly/lib/binding/function.rs b/pyrefly/lib/binding/function.rs index 5d482d2d55..eaad0328db 100644 --- a/pyrefly/lib/binding/function.rs +++ b/pyrefly/lib/binding/function.rs @@ -633,6 +633,7 @@ impl<'a> BindingsBuilder<'a> { implicit_return: Option>, should_infer_return_type: bool, is_stub: bool, + check_never_return: bool, ) { let is_generator = yields_and_returns.is_generator; let return_ann = return_ann_with_range.as_ref().map(|(_, key)| *key); @@ -642,7 +643,6 @@ impl<'a> BindingsBuilder<'a> { } else { None }; - // Collect the keys of explicit returns. let return_keys = yields_and_returns .returns @@ -692,6 +692,9 @@ impl<'a> BindingsBuilder<'a> { let return_type_binding = { let kind = match (return_ann_with_range, implicit_return, is_stub) { (Some((range, annotation)), Some(implicit_return), false) => { + let function = self.idx_for_promise(KeyUndecoratedFunction( + ShortIdentifier::new(func_name), + )); self.insert_binding( KeyExpect::ValidateImplicitReturn(range), BindingExpect::ValidateImplicitReturn { @@ -699,7 +702,9 @@ impl<'a> BindingsBuilder<'a> { implicit_return, is_async, is_generator, - has_explicit_return: !return_keys.is_empty(), + returns: return_keys, + function, + check_never_return, }, ); ReturnTypeKind::ShouldTrustAnnotation { @@ -861,16 +866,21 @@ impl<'a> BindingsBuilder<'a> { } else { body.as_slice() }; - let body_kind = match body_no_docstring { - // raise NotImplementedError(...) - [Stmt::Raise(StmtRaise { exc: Some(exc), .. })] - if self.as_special_export(match &**exc { + let raises_not_implemented = match body_no_docstring.last() { + Some(Stmt::Raise(StmtRaise { exc: Some(exc), .. })) => { + self.as_special_export(match &**exc { Expr::Call(ExprCall { func, .. }) => func, other => other, - }) == Some(SpecialExport::NotImplementedError) => - { - BodyKind::RaiseNotImplementedError + }) == Some(SpecialExport::NotImplementedError) } + _ => false, + }; + // A raise-only body or a terminal NotImplementedError can stand in for an implementation. + let check_never_return = + !raises_not_implemented && !matches!(body_no_docstring, [Stmt::Raise(_)]); + let body_kind = match body_no_docstring { + // raise NotImplementedError(...) + [Stmt::Raise(_)] if raises_not_implemented => BodyKind::RaiseNotImplementedError, // return NotImplemented [ Stmt::Return(StmtReturn { @@ -959,6 +969,7 @@ impl<'a> BindingsBuilder<'a> { implicit_return, false, facts.is_stub(), + check_never_return, ); (false, self_assignments) } else { @@ -996,6 +1007,7 @@ impl<'a> BindingsBuilder<'a> { implicit_return, should_infer, facts.is_stub(), + check_never_return, ); // Mirror the `ReturnTypeKind::ShouldInferType` arm in `analyze_return_type`: // we infer iff there's no return annotation and inference was requested. diff --git a/pyrefly/lib/test/assign.rs b/pyrefly/lib/test/assign.rs index 810db0ef21..9bef4961d8 100644 --- a/pyrefly/lib/test/assign.rs +++ b/pyrefly/lib/test/assign.rs @@ -324,7 +324,7 @@ testcase!( test_never, r#" from typing import Any, Never, NoReturn -def foo(x: Never) -> Any: +def foo(x: Never) -> Any: # E: Function declared to return `Any` but can never return y: NoReturn = x z: int = x return x diff --git a/pyrefly/lib/test/flow_branching.rs b/pyrefly/lib/test/flow_branching.rs index d9f16a8b0f..968499204a 100644 --- a/pyrefly/lib/test/flow_branching.rs +++ b/pyrefly/lib/test/flow_branching.rs @@ -1912,7 +1912,7 @@ assert_type(vari, Literal["test"]) # E: `vari` is uninitialized testcase!( test_assert_false_terminates_flow, r#" -def test1() -> int: +def test1() -> int: # E: Function declared to return `int` but can never return assert False def test2() -> int: # E: Function declared to return `int` but is missing an explicit `return` assert True diff --git a/pyrefly/lib/test/narrow.rs b/pyrefly/lib/test/narrow.rs index 44a705ea70..1f208680fb 100644 --- a/pyrefly/lib/test/narrow.rs +++ b/pyrefly/lib/test/narrow.rs @@ -1003,7 +1003,7 @@ testcase!( from typing import assert_type def make_it() -> int | tuple[int, str] | tuple[bool] | tuple[str]: - assert False + ... def test(): c = make_it() @@ -1027,7 +1027,7 @@ testcase!( from typing import assert_type def make_it() -> int | list[int] | list[bool] | list[str]: - assert False + ... def test(): c = make_it() @@ -1087,7 +1087,7 @@ testcase!( from typing import assert_type, Iterable def make_it() -> int | tuple[int, str] | list[bool] | list[str]: - assert False + ... def test(): c = make_it() diff --git a/pyrefly/lib/test/returns.rs b/pyrefly/lib/test/returns.rs index 58ddd9fb84..0c2fc1d68c 100644 --- a/pyrefly/lib/test/returns.rs +++ b/pyrefly/lib/test/returns.rs @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +use crate::test::util::TestEnv; use crate::testcase; testcase!( @@ -188,7 +189,7 @@ from typing import NoReturn def fail() -> NoReturn: raise Exception() -def f() -> int: +def f() -> int: # E: Function declared to return `int` but can never return fail() "#, ); @@ -324,6 +325,172 @@ def f() -> int: "#, ); +// Regression test for https://github.com/facebook/pyrefly/issues/1132 +testcase!( + test_return_infinite_loop_without_return, + r#" +from typing import Never, TypeAlias + +NeverAlias: TypeAlias = Never + +def f() -> int: # E: Function declared to return `int` but can never return + while True: + pass + +def returns_never() -> Never: + while True: + pass + +def returns_never_alias() -> NeverAlias: + while True: + pass + +def event_loop() -> None: + while True: + pass + +def unreachable_return() -> int: # E: Function declared to return `int` but can never return + while True: + continue + return 1 # E: This code is unreachable + +def falls_through(b: bool) -> int: # E: Function declared to return `int` but is missing an explicit `return` + if b: + while True: + pass +"#, +); + +testcase!( + test_unexpected_never_return, + r#" +from abc import ABC, abstractmethod +from typing import Any, AsyncIterator, Iterator, Never, NoReturn, Protocol, assert_type, overload, override + +def fatal() -> Never: + raise RuntimeError + +def calls_fatal() -> int: # E: Function declared to return `int` but can never return + fatal() + +def returns_fatal() -> str: # E: Function declared to return `str` but can never return + return fatal() + +def raises() -> int: + raise ValueError + +def raises_with_docstring() -> int: + """This operation is unsupported.""" + raise ValueError + +def raises_after_work() -> int: # E: Function declared to return `int` but can never return + print("starting") + raise ValueError + +def raises_none() -> None: + raise ValueError + +async def async_fatal() -> int: # E: Function declared to return `int` but can never return + return fatal() + +def finally_fatal() -> int: # E: Function declared to return `int` but can never return + try: + pass + finally: + fatal() + +def branches(flag: bool) -> int: # E: Function declared to return `int` but can never return + if flag: + return fatal() + else: + raise ValueError + +def returns_normally(flag: bool) -> int: + if flag: + fatal() + return 1 + +def gradual() -> Any: # E: Function declared to return `Any` but can never return + fatal() + +def placeholder() -> int: + """Implementation belongs in a subclass.""" + raise NotImplementedError("not implemented") + +def placeholder_with_checks(x: int) -> int: + assert_type(x, int) + raise NotImplementedError + +def stub() -> int: ... + +def intentional() -> NoReturn: + fatal() + +def generator() -> Iterator[int]: + yield 1 + fatal() + +async def async_generator() -> AsyncIterator[int]: + yield 1 + fatal() + +@overload +def overloaded(x: int) -> int: ... +@overload +def overloaded(x: str) -> str: ... +def overloaded(x: int | str) -> int | str: # E: Function declared to return `int | str` but can never return + fatal() + +class Base(ABC): + @abstractmethod + def abstract(self) -> int: + fatal() + + def implemented(self) -> int: + return 1 + +class Child(Base): + def abstract(self) -> int: + fatal() + + @override + def implemented(self) -> int: + fatal() + + def raising_none(self) -> None: + """This operation is unsupported.""" + raise ValueError + + def calls_fatal(self) -> int: # E: Function declared to return `int` but can never return + fatal() + +class Interface(Protocol): + def stub(self) -> int: ... + +# The published type must still follow the annotation. +assert_type(calls_fatal(), int) +"#, +); + +testcase!( + test_unexpected_never_in_stub_file, + { + let mut env = TestEnv::new(); + env.add_with_path( + "stub", + "stub.pyi", + "def f() -> int:\n raise RuntimeError\n", + ); + env + }, + r#" +from typing import assert_type +from stub import f + +assert_type(f(), int) +"#, +); + testcase!( test_return_while_false_break_else, r#" diff --git a/scripts/error_presets.json b/scripts/error_presets.json index ab0bae7c92..c53b664d58 100644 --- a/scripts/error_presets.json +++ b/scripts/error_presets.json @@ -134,6 +134,7 @@ "unannotated-return": ["all"], "unbound-name": ["default", "strict", "all"], "unexpected-keyword": ["basic", "legacy", "default", "strict", "all"], + "unexpected-never": ["legacy", "default", "strict", "all"], "unexpected-positional-argument": ["basic", "legacy", "default", "strict", "all"], "unimported-directive": ["legacy", "default", "strict", "all"], "uninitialized-instance-variable": ["all"], diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 48cc306b71..16194bef83 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -2216,6 +2216,34 @@ def two_args(a: int, b: int) -> int: two_args(a=1, b=2, c=3) ``` +## unexpected-never + +Default severity: `warn` + +Pyrefly emits this warning when the inferred return type is `Never`, but the +return annotation is neither `Never`, `NoReturn`, nor `None`. This includes infinite +loops, unconditional exceptions, and calls to functions that never return. +Pyrefly checks both explicit returns and paths that reach the end of the body. + +```python +from typing import Never + +def stop() -> Never: + raise RuntimeError + +def f() -> int: # Function declared to return `int` but can never return [unexpected-never] + return stop() +``` + +Pyrefly exempts placeholder bodies, stubs, overload declarations, abstract +methods, and overrides, including overrides without an `@override` decorator. +For this warning, a placeholder body can be a single `raise` statement or a +body that ends with `raise NotImplementedError`. An optional docstring does +not affect these exemptions. +It also exempts generators and functions annotated `-> None`, including +long-running event loops and methods that always raise. The declared return +type remains the type visible to callers. + ## unexpected-positional-argument A positional argument was passed for a keyword-only parameter.