From ed2bce6ebb56c84c5ecd22cbe8aac4a887c403ca Mon Sep 17 00:00:00 2001 From: InSync Date: Thu, 19 Dec 2024 01:31:24 +0700 Subject: [PATCH] [red-knot] Report invalid exceptions (#15042) Co-authored-by: Alex Waygood --- .../resources/mdtest/exception/basic.md | 80 +++++++++++++++++++ crates/red_knot_python_semantic/src/types.rs | 3 +- .../src/types/diagnostic.rs | 67 ++++++++++++++++ .../src/types/infer.rs | 37 ++++++--- 4 files changed, 177 insertions(+), 10 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/exception/basic.md b/crates/red_knot_python_semantic/resources/mdtest/exception/basic.md index 9610d403c8afb..1c53bc754cc0a 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/exception/basic.md +++ b/crates/red_knot_python_semantic/resources/mdtest/exception/basic.md @@ -90,3 +90,83 @@ def foo( # TODO: should emit a diagnostic here: reveal_type(g) # revealed: @Todo(full tuple[...] support) ``` + +## Object raised is not an exception + +```py +try: + raise AttributeError() # fine +except: + ... + +try: + raise FloatingPointError # fine +except: + ... + +try: + raise 1 # error: [invalid-raise] +except: + ... + +try: + raise int # error: [invalid-raise] +except: + ... + +def _(e: Exception | type[Exception]): + raise e # fine + +def _(e: Exception | type[Exception] | None): + raise e # error: [invalid-raise] +``` + +## Exception cause is not an exception + +```py +try: + raise EOFError() from GeneratorExit # fine +except: + ... + +try: + raise StopIteration from MemoryError() # fine +except: + ... + +try: + raise BufferError() from None # fine +except: + ... + +try: + raise ZeroDivisionError from False # error: [invalid-raise] +except: + ... + +try: + raise SystemExit from bool() # error: [invalid-raise] +except: + ... + +try: + raise +except KeyboardInterrupt as e: # fine + reveal_type(e) # revealed: KeyboardInterrupt + raise LookupError from e # fine + +try: + raise +except int as e: # error: [invalid-exception-caught] + reveal_type(e) # revealed: Unknown + raise KeyError from e + +def _(e: Exception | type[Exception]): + raise ModuleNotFoundError from e # fine + +def _(e: Exception | type[Exception] | None): + raise IndexError from e # fine + +def _(e: int | None): + raise IndexError from e # error: [invalid-raise] +``` diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 5e68263cc05de..4acce9ded5910 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -2315,10 +2315,11 @@ impl<'db> KnownClass { .unwrap_or(Type::Unknown) } - pub fn to_subclass_of(self, db: &'db dyn Db) -> Option> { + pub fn to_subclass_of(self, db: &'db dyn Db) -> Type<'db> { self.to_class_literal(db) .into_class_literal() .map(|ClassLiteralType { class }| Type::subclass_of(class)) + .unwrap_or(Type::subclass_of_base(ClassBase::Unknown)) } /// Return the module in which we should look up the definition for this class diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 21e7b1d02277e..01096d1c5ee4a 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -34,6 +34,7 @@ pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) { registry.register_lint(&INVALID_DECLARATION); registry.register_lint(&INVALID_EXCEPTION_CAUGHT); registry.register_lint(&INVALID_PARAMETER_DEFAULT); + registry.register_lint(&INVALID_RAISE); registry.register_lint(&INVALID_TYPE_FORM); registry.register_lint(&INVALID_TYPE_VARIABLE_CONSTRAINTS); registry.register_lint(&NON_SUBSCRIPTABLE); @@ -248,6 +249,49 @@ declare_lint! { } } +declare_lint! { + /// Checks for `raise` statements that raise non-exceptions or use invalid + /// causes for their raised exceptions. + /// + /// ## Why is this bad? + /// Only subclasses or instances of `BaseException` can be raised. + /// For an exception's cause, the same rules apply, except that `None` is also + /// permitted. Violating these rules results in a `TypeError` at runtime. + /// + /// ## Examples + /// ```python + /// def f(): + /// try: + /// something() + /// except NameError: + /// raise "oops!" from f + /// + /// def g(): + /// raise NotImplemented from 42 + /// ``` + /// + /// Use instead: + /// ```python + /// def f(): + /// try: + /// something() + /// except NameError as e: + /// raise RuntimeError("oops!") from e + /// + /// def g(): + /// raise NotImplementedError from None + /// ``` + /// + /// ## References + /// - [Python documentation: The `raise` statement](https://docs.python.org/3/reference/simple_stmts.html#raise) + /// - [Python documentation: Built-in Exceptions](https://docs.python.org/3/library/exceptions.html#built-in-exceptions) + pub(crate) static INVALID_RAISE = { + summary: "detects `raise` statements that raise invalid exceptions or use invalid causes", + status: LintStatus::preview("1.0.0"), + default_level: Level::Error, + } +} + declare_lint! { /// ## What it does /// Checks for invalid type expressions. @@ -721,3 +765,26 @@ pub(super) fn report_invalid_exception_caught(context: &InferContext, node: &ast ), ); } + +pub(crate) fn report_invalid_exception_raised(context: &InferContext, node: &ast::Expr, ty: Type) { + context.report_lint( + &INVALID_RAISE, + node.into(), + format_args!( + "Cannot raise object of type `{}` (must be a `BaseException` subclass or instance)", + ty.display(context.db()) + ), + ); +} + +pub(crate) fn report_invalid_exception_cause(context: &InferContext, node: &ast::Expr, ty: Type) { + context.report_lint( + &INVALID_RAISE, + node.into(), + format_args!( + "Cannot use object of type `{}` as exception cause \ + (must be a `BaseException` subclass or instance or `None`)", + ty.display(context.db()) + ), + ); +} diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 9eafda8d564ed..7ff8431f345e9 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -74,7 +74,8 @@ use crate::Db; use super::context::{InferContext, WithDiagnostics}; use super::diagnostic::{ - report_index_out_of_bounds, report_invalid_exception_caught, report_non_subscriptable, + report_index_out_of_bounds, report_invalid_exception_caught, report_invalid_exception_cause, + report_invalid_exception_raised, report_non_subscriptable, report_possibly_unresolved_reference, report_slice_step_size_zero, report_unresolved_reference, }; use super::string_annotation::{ @@ -1574,9 +1575,7 @@ impl<'db> TypeInferenceBuilder<'db> { // If it's an `except*` handler, this won't actually be the type of the bound symbol; // it will actually be the type of the generic parameters to `BaseExceptionGroup` or `ExceptionGroup`. let symbol_ty = if let Type::Tuple(tuple) = node_ty { - let type_base_exception = KnownClass::BaseException - .to_subclass_of(self.db()) - .unwrap_or(Type::Unknown); + let type_base_exception = KnownClass::BaseException.to_subclass_of(self.db()); let mut builder = UnionBuilder::new(self.db()); for element in tuple.elements(self.db()).iter().copied() { builder = builder.add( @@ -1594,9 +1593,7 @@ impl<'db> TypeInferenceBuilder<'db> { } else if node_ty.is_subtype_of(self.db(), KnownClass::Tuple.to_instance(self.db())) { todo_type!("Homogeneous tuple in exception handler") } else { - let type_base_exception = KnownClass::BaseException - .to_subclass_of(self.db()) - .unwrap_or(Type::Unknown); + let type_base_exception = KnownClass::BaseException.to_subclass_of(self.db()); if node_ty.is_assignable_to(self.db(), type_base_exception) { node_ty.to_instance(self.db()) } else { @@ -2198,8 +2195,30 @@ impl<'db> TypeInferenceBuilder<'db> { exc, cause, } = raise; - self.infer_optional_expression(exc.as_deref()); - self.infer_optional_expression(cause.as_deref()); + + let base_exception_type = KnownClass::BaseException.to_subclass_of(self.db()); + let base_exception_instance = base_exception_type.to_instance(self.db()); + + let can_be_raised = + UnionType::from_elements(self.db(), [base_exception_type, base_exception_instance]); + let can_be_exception_cause = + UnionType::from_elements(self.db(), [can_be_raised, Type::none(self.db())]); + + if let Some(raised) = exc { + let raised_type = self.infer_expression(raised); + + if !raised_type.is_assignable_to(self.db(), can_be_raised) { + report_invalid_exception_raised(&self.context, raised, raised_type); + } + } + + if let Some(cause) = cause { + let cause_type = self.infer_expression(cause); + + if !cause_type.is_assignable_to(self.db(), can_be_exception_cause) { + report_invalid_exception_cause(&self.context, cause, cause_type); + } + } } /// Given a `from .foo import bar` relative import, resolve the relative module