From e34be273158e7d755e6192749c76f36d47b6c41a Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 2 Aug 2024 18:47:17 +0100 Subject: [PATCH] [`pydoclint`] Teach rules to understand reraised exceptions as being explicitly raised --- .../test/fixtures/pydoclint/DOC501_google.py | 42 ++++++++++++ .../test/fixtures/pydoclint/DOC501_numpy.py | 57 ++++++++++++++++ .../test/fixtures/pydoclint/DOC502_google.py | 25 +++++++ .../test/fixtures/pydoclint/DOC502_numpy.py | 35 ++++++++++ .../rules/pydoclint/rules/check_docstring.rs | 68 ++++++++++++++----- ...ng-missing-exception_DOC501_google.py.snap | 11 +++ ...ing-missing-exception_DOC501_numpy.py.snap | 9 +++ 7 files changed, 229 insertions(+), 18 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_google.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_google.py index c5dc038b22497..ab648696172ef 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_google.py +++ b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_google.py @@ -190,3 +190,45 @@ def foo(bar: int): something.SomeError: Wow. """ raise something.SomeError + + +# DOC501 +def calculate_speed(distance: float, time: float) -> float: + """Calculate speed as distance divided by time. + + Args: + distance: Distance traveled. + time: Time spent traveling. + + Returns: + Speed as distance divided by time. + + Raises: + TypeError: if you didn't pass a number for both parameters + """ + try: + return distance / time + except ZeroDivisionError: + print("Oh no, why would you divide something by zero?") + raise + except TypeError: + print("Not a number? Shame on you!") + raise + + +# This is fine +def calculate_speed(distance: float, time: float) -> float: + """Calculate speed as distance divided by time. + + Args: + distance: Distance traveled. + time: Time spent traveling. + + Returns: + Speed as distance divided by time. + """ + try: + return distance / time + except Exception as e: + print(f"Oh no, we encountered {e}") + raise diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_numpy.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_numpy.py index f78beaec3f701..16c6e74124d08 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_numpy.py +++ b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC501_numpy.py @@ -76,3 +76,60 @@ def calculate_speed(distance: float, time: float) -> float: raise FasterThanLightError from exc except: raise ValueError + + +# DOC501 +def calculate_speed(distance: float, time: float) -> float: + """Calculate speed as distance divided by time. + + ACalculate speed as distance divided by time. + + Parameters + ---------- + distance : float + Distance traveled. + time : float + Time spent traveling. + + Returns + ------- + float + Speed as distance divided by time. + + Raises + ------ + ZeroDivisionError + If attempting to divide by zero. + """ + try: + return distance / time + except ZeroDivisionError: + print("Oh no, why would you divide something by zero?") + raise + except TypeError: + print("Not a number? Shame on you!") + raise + + +# This is fine +def calculate_speed(distance: float, time: float) -> float: + """ + Calculate speed as distance divided by time. + + Parameters + ---------- + distance : float + Distance traveled. + time : float + Time spent traveling. + + Returns + ------- + float + Speed as distance divided by time. + """ + try: + return distance / time + except Exception as e: + print(f"Oh no, we encountered {e}") + raise diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_google.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_google.py index 639a7965134f7..f9e7f7a89f64a 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_google.py +++ b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_google.py @@ -56,3 +56,28 @@ def calculate_speed(distance: float, time: float) -> float: return distance / time except ZeroDivisionError as exc: raise FasterThanLightError from exc + + +# This is fine +def calculate_speed(distance: float, time: float) -> float: + """Calculate speed as distance divided by time. + + Args: + distance: Distance traveled. + time: Time spent traveling. + + Returns: + Speed as distance divided by time. + + Raises: + ZeroDivisionError: If you pass `0` for the time + TypeError: if you didn't pass a number for both parameters + """ + try: + return distance / time + except ZeroDivisionError: + print("Oh no, why would you divide something by zero?") + raise + except TypeError: + print("Not a number? Shame on you!") + raise diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_numpy.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_numpy.py index 95b84e813495c..5e8bf5f36ef81 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_numpy.py +++ b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_numpy.py @@ -82,3 +82,38 @@ def calculate_speed(distance: float, time: float) -> float: return distance / time except ZeroDivisionError as exc: raise FasterThanLightError from exc + + +# This is fine +def calculate_speed(distance: float, time: float) -> float: + """Calculate speed as distance divided by time. + + ACalculate speed as distance divided by time. + + Parameters + ---------- + distance : float + Distance traveled. + time : float + Time spent traveling. + + Returns + ------- + float + Speed as distance divided by time. + + Raises + ------ + TypeError + If one or both of the parameters is not a number. + ZeroDivisionError + If attempting to divide by zero. + """ + try: + return distance / time + except ZeroDivisionError: + print("Oh no, why would you divide something by zero?") + raise + except TypeError: + print("Not a number? Shame on you!") + raise diff --git a/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs b/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs index 63a98edc6a686..a0698e37476bd 100644 --- a/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs +++ b/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs @@ -2,6 +2,7 @@ use itertools::Itertools; use ruff_diagnostics::Diagnostic; use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::map_callable; use ruff_python_ast::name::QualifiedName; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, visitor, Expr, Stmt}; @@ -505,6 +506,7 @@ struct BodyEntries<'a> { struct BodyVisitor<'a> { returns: Vec, yields: Vec, + currently_suspended_exceptions: Option<&'a ast::Expr>, raised_exceptions: Vec>, semantic: &'a SemanticModel<'a>, } @@ -514,6 +516,7 @@ impl<'a> BodyVisitor<'a> { Self { returns: Vec::new(), yields: Vec::new(), + currently_suspended_exceptions: None, raised_exceptions: Vec::new(), semantic, } @@ -529,15 +532,47 @@ impl<'a> BodyVisitor<'a> { } impl<'a> Visitor<'a> for BodyVisitor<'a> { + fn visit_except_handler(&mut self, handler: &'a ast::ExceptHandler) { + let ast::ExceptHandler::ExceptHandler(handler_inner) = handler; + self.currently_suspended_exceptions = handler_inner.type_.as_deref(); + visitor::walk_except_handler(self, handler); + self.currently_suspended_exceptions = None; + } + fn visit_stmt(&mut self, stmt: &'a Stmt) { match stmt { - Stmt::Raise(ast::StmtRaise { exc: Some(exc), .. }) => { - if let Some(qualified_name) = extract_raised_exception(self.semantic, exc.as_ref()) - { - self.raised_exceptions.push(ExceptionEntry { - qualified_name, - range: exc.as_ref().range(), - }); + Stmt::Raise(ast::StmtRaise { exc, .. }) => { + if let Some(exc) = exc.as_ref() { + if let Some(qualified_name) = + self.semantic.resolve_qualified_name(map_callable(exc)) + { + self.raised_exceptions.push(ExceptionEntry { + qualified_name, + range: exc.range(), + }); + } + } else if let Some(exceptions) = self.currently_suspended_exceptions { + let mut maybe_store_exception = |exception| { + let Some(qualified_name) = self.semantic.resolve_qualified_name(exception) + else { + return; + }; + if is_exception_or_base_exception(&qualified_name) { + return; + } + self.raised_exceptions.push(ExceptionEntry { + qualified_name, + range: stmt.range(), + }); + }; + + if let ast::Expr::Tuple(tuple) = exceptions { + for exception in &tuple.elts { + maybe_store_exception(exception); + } + } else { + maybe_store_exception(exceptions); + } } } Stmt::Return(ast::StmtReturn { @@ -571,17 +606,14 @@ impl<'a> Visitor<'a> for BodyVisitor<'a> { } } -fn extract_raised_exception<'a>( - semantic: &SemanticModel<'a>, - exc: &'a Expr, -) -> Option> { - if let Some(qualified_name) = semantic.resolve_qualified_name(exc) { - return Some(qualified_name); - } - if let Expr::Call(ast::ExprCall { func, .. }) = exc { - return extract_raised_exception(semantic, func.as_ref()); - } - None +fn is_exception_or_base_exception(qualified_name: &QualifiedName) -> bool { + matches!( + qualified_name.segments(), + [ + "" | "builtins", + "BaseException" | "Exception" | "BaseExceptionGroup" | "ExceptionGroup" + ] + ) } /// DOC201, DOC202, DOC402, DOC403, DOC501, DOC502 diff --git a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_google.py.snap b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_google.py.snap index 7f1c628eadedb..0976d183c270d 100644 --- a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_google.py.snap +++ b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_google.py.snap @@ -56,3 +56,14 @@ DOC501_google.py:139:11: DOC501 Raised exception `SomeError` missing from docstr | ^^^^^^^^^^^^^^^^^^^ DOC501 | = help: Add `SomeError` to the docstring + +DOC501_google.py:213:9: DOC501 Raised exception `ZeroDivisionError` missing from docstring + | +211 | except ZeroDivisionError: +212 | print("Oh no, why would you divide something by zero?") +213 | raise + | ^^^^^ DOC501 +214 | except TypeError: +215 | print("Not a number? Shame on you!") + | + = help: Add `ZeroDivisionError` to the docstring diff --git a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_numpy.py.snap b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_numpy.py.snap index 96823553213e3..7f08fa44b38d6 100644 --- a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_numpy.py.snap +++ b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_numpy.py.snap @@ -29,3 +29,12 @@ DOC501_numpy.py:78:15: DOC501 Raised exception `ValueError` missing from docstri | ^^^^^^^^^^ DOC501 | = help: Add `ValueError` to the docstring + +DOC501_numpy.py:111:9: DOC501 Raised exception `TypeError` missing from docstring + | +109 | except TypeError: +110 | print("Not a number? Shame on you!") +111 | raise + | ^^^^^ DOC501 + | + = help: Add `TypeError` to the docstring