Skip to content

Commit 1ade9a5

Browse files
danparizherntBre
andauthored
[pydoclint] Fix false positive on explicit exception re-raising (DOC501, DOC502) (#21011)
## Summary Fixes #20973 (`docstring-extraneous-exception`) false positive when exceptions mentioned in docstrings are caught and explicitly re-raised using `raise e` or `raise e from None`. ## Problem Analysis The DOC502 rule was incorrectly flagging exceptions mentioned in docstrings as "not explicitly raised" when they were actually being explicitly re-raised through exception variables bound in `except` clauses. **Root Cause**: The `BodyVisitor` in `check_docstring.rs` only checked for direct exception references (like `raise OSError()`) but didn't recognize when a variable bound to an exception in an `except` clause was being re-raised. **Example of the bug**: ```python def f(): """Do nothing. Raises ------ OSError If the OS errors. """ try: pass except OSError as e: raise e # This was incorrectly flagged as not explicitly raising OSError ``` The issue occurred because `resolve_qualified_name(e)` couldn't resolve the variable `e` to a qualified exception name, since `e` is just a variable binding, not a direct reference to an exception class. ## Approach Modified the `BodyVisitor` in `crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs` to: 1. **Track exception variable bindings**: Added `exception_variables` field to map exception variable names to their exception types within `except` clauses 2. **Enhanced raise statement detection**: Updated `visit_stmt` to check if a `raise` statement uses a variable name that's bound to an exception in the current `except` clause 3. **Proper scope management**: Clear exception variable mappings when leaving `except` handlers to prevent cross-contamination **Key changes**: - Added `exception_variables: FxHashMap<&'a str, QualifiedName<'a>>` to track variable-to-exception mappings - Enhanced `visit_except_handler` to store exception variable bindings when entering `except` clauses - Modified `visit_stmt` to check for variable-based re-raising: `raise e` → lookup `e` in `exception_variables` - Clear mappings when exiting `except` handlers to maintain proper scope --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
1 parent 304ac22 commit 1ade9a5

File tree

4 files changed

+218
-16
lines changed

4 files changed

+218
-16
lines changed

crates/ruff_linter/resources/test/fixtures/pydoclint/DOC502_google.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,3 +81,55 @@ def calculate_speed(distance: float, time: float) -> float:
8181
except TypeError:
8282
print("Not a number? Shame on you!")
8383
raise
84+
85+
86+
# This should NOT trigger DOC502 because OSError is explicitly re-raised
87+
def f():
88+
"""Do nothing.
89+
90+
Raises:
91+
OSError: If the OS errors.
92+
"""
93+
try:
94+
pass
95+
except OSError as e:
96+
raise e
97+
98+
99+
# This should NOT trigger DOC502 because OSError is explicitly re-raised with from None
100+
def g():
101+
"""Do nothing.
102+
103+
Raises:
104+
OSError: If the OS errors.
105+
"""
106+
try:
107+
pass
108+
except OSError as e:
109+
raise e from None
110+
111+
112+
# This should NOT trigger DOC502 because ValueError is explicitly re-raised from tuple exception
113+
def h():
114+
"""Do nothing.
115+
116+
Raises:
117+
ValueError: If something goes wrong.
118+
"""
119+
try:
120+
pass
121+
except (ValueError, TypeError) as e:
122+
raise e
123+
124+
125+
# This should NOT trigger DOC502 because TypeError is explicitly re-raised from tuple exception
126+
def i():
127+
"""Do nothing.
128+
129+
Raises:
130+
TypeError: If something goes wrong.
131+
"""
132+
try:
133+
pass
134+
except (ValueError, TypeError) as e:
135+
raise e

crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use ruff_python_semantic::{Definition, SemanticModel};
99
use ruff_python_stdlib::identifiers::is_identifier;
1010
use ruff_source_file::{LineRanges, NewlineWithTrailingNewline};
1111
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
12+
use rustc_hash::FxHashMap;
1213

1314
use crate::Violation;
1415
use crate::checkers::ast::Checker;
@@ -823,6 +824,8 @@ struct BodyVisitor<'a> {
823824
currently_suspended_exceptions: Option<&'a ast::Expr>,
824825
raised_exceptions: Vec<ExceptionEntry<'a>>,
825826
semantic: &'a SemanticModel<'a>,
827+
/// Maps exception variable names to their exception expressions in the current except clause
828+
exception_variables: FxHashMap<&'a str, &'a ast::Expr>,
826829
}
827830

828831
impl<'a> BodyVisitor<'a> {
@@ -833,6 +836,7 @@ impl<'a> BodyVisitor<'a> {
833836
currently_suspended_exceptions: None,
834837
raised_exceptions: Vec::new(),
835838
semantic,
839+
exception_variables: FxHashMap::default(),
836840
}
837841
}
838842

@@ -864,49 +868,75 @@ impl<'a> BodyVisitor<'a> {
864868
raised_exceptions,
865869
}
866870
}
871+
872+
/// Store `exception` if its qualified name does not correspond to one of the exempt types.
873+
fn maybe_store_exception(&mut self, exception: &'a Expr, range: TextRange) {
874+
let Some(qualified_name) = self.semantic.resolve_qualified_name(exception) else {
875+
return;
876+
};
877+
if is_exception_or_base_exception(&qualified_name) {
878+
return;
879+
}
880+
self.raised_exceptions.push(ExceptionEntry {
881+
qualified_name,
882+
range,
883+
});
884+
}
867885
}
868886

869887
impl<'a> Visitor<'a> for BodyVisitor<'a> {
870888
fn visit_except_handler(&mut self, handler: &'a ast::ExceptHandler) {
871889
let ast::ExceptHandler::ExceptHandler(handler_inner) = handler;
872890
self.currently_suspended_exceptions = handler_inner.type_.as_deref();
891+
892+
// Track exception variable bindings
893+
if let Some(name) = handler_inner.name.as_ref() {
894+
if let Some(exceptions) = self.currently_suspended_exceptions {
895+
// Store the exception expression(s) for later resolution
896+
self.exception_variables
897+
.insert(name.id.as_str(), exceptions);
898+
}
899+
}
900+
873901
visitor::walk_except_handler(self, handler);
874902
self.currently_suspended_exceptions = None;
903+
// Clear exception variables when leaving the except handler
904+
self.exception_variables.clear();
875905
}
876906

877907
fn visit_stmt(&mut self, stmt: &'a Stmt) {
878908
match stmt {
879909
Stmt::Raise(ast::StmtRaise { exc, .. }) => {
880910
if let Some(exc) = exc.as_ref() {
911+
// First try to resolve the exception directly
881912
if let Some(qualified_name) =
882913
self.semantic.resolve_qualified_name(map_callable(exc))
883914
{
884915
self.raised_exceptions.push(ExceptionEntry {
885916
qualified_name,
886917
range: exc.range(),
887918
});
919+
} else if let ast::Expr::Name(name) = exc.as_ref() {
920+
// If it's a variable name, check if it's bound to an exception in the
921+
// current except clause
922+
if let Some(exception_expr) = self.exception_variables.get(name.id.as_str())
923+
{
924+
if let ast::Expr::Tuple(tuple) = exception_expr {
925+
for exception in tuple {
926+
self.maybe_store_exception(exception, stmt.range());
927+
}
928+
} else {
929+
self.maybe_store_exception(exception_expr, stmt.range());
930+
}
931+
}
888932
}
889933
} else if let Some(exceptions) = self.currently_suspended_exceptions {
890-
let mut maybe_store_exception = |exception| {
891-
let Some(qualified_name) = self.semantic.resolve_qualified_name(exception)
892-
else {
893-
return;
894-
};
895-
if is_exception_or_base_exception(&qualified_name) {
896-
return;
897-
}
898-
self.raised_exceptions.push(ExceptionEntry {
899-
qualified_name,
900-
range: stmt.range(),
901-
});
902-
};
903-
904934
if let ast::Expr::Tuple(tuple) = exceptions {
905935
for exception in tuple {
906-
maybe_store_exception(exception);
936+
self.maybe_store_exception(exception, stmt.range());
907937
}
908938
} else {
909-
maybe_store_exception(exceptions);
939+
self.maybe_store_exception(exceptions, stmt.range());
910940
}
911941
}
912942
}

crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_google.py.snap

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,66 @@ DOC501 Raised exception `FasterThanLightError` missing from docstring
6161
|
6262
help: Add `FasterThanLightError` to the docstring
6363

64+
DOC501 Raised exception `ZeroDivisionError` missing from docstring
65+
--> DOC501_google.py:70:5
66+
|
67+
68 | # DOC501
68+
69 | def calculate_speed(distance: float, time: float) -> float:
69+
70 | / """Calculate speed as distance divided by time.
70+
71 | |
71+
72 | | Args:
72+
73 | | distance: Distance traveled.
73+
74 | | time: Time spent traveling.
74+
75 | |
75+
76 | | Returns:
76+
77 | | Speed as distance divided by time.
77+
78 | | """
78+
| |_______^
79+
79 | try:
80+
80 | return distance / time
81+
|
82+
help: Add `ZeroDivisionError` to the docstring
83+
84+
DOC501 Raised exception `ValueError` missing from docstring
85+
--> DOC501_google.py:88:5
86+
|
87+
86 | # DOC501
88+
87 | def calculate_speed(distance: float, time: float) -> float:
89+
88 | / """Calculate speed as distance divided by time.
90+
89 | |
91+
90 | | Args:
92+
91 | | distance: Distance traveled.
93+
92 | | time: Time spent traveling.
94+
93 | |
95+
94 | | Returns:
96+
95 | | Speed as distance divided by time.
97+
96 | | """
98+
| |_______^
99+
97 | try:
100+
98 | return distance / time
101+
|
102+
help: Add `ValueError` to the docstring
103+
104+
DOC501 Raised exception `ZeroDivisionError` missing from docstring
105+
--> DOC501_google.py:88:5
106+
|
107+
86 | # DOC501
108+
87 | def calculate_speed(distance: float, time: float) -> float:
109+
88 | / """Calculate speed as distance divided by time.
110+
89 | |
111+
90 | | Args:
112+
91 | | distance: Distance traveled.
113+
92 | | time: Time spent traveling.
114+
93 | |
115+
94 | | Returns:
116+
95 | | Speed as distance divided by time.
117+
96 | | """
118+
| |_______^
119+
97 | try:
120+
98 | return distance / time
121+
|
122+
help: Add `ZeroDivisionError` to the docstring
123+
64124
DOC501 Raised exception `AnotherError` missing from docstring
65125
--> DOC501_google.py:106:5
66126
|

crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-exception_DOC501_google.py_ignore_one_line.snap

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,66 @@ DOC501 Raised exception `FasterThanLightError` missing from docstring
6161
|
6262
help: Add `FasterThanLightError` to the docstring
6363

64+
DOC501 Raised exception `ZeroDivisionError` missing from docstring
65+
--> DOC501_google.py:70:5
66+
|
67+
68 | # DOC501
68+
69 | def calculate_speed(distance: float, time: float) -> float:
69+
70 | / """Calculate speed as distance divided by time.
70+
71 | |
71+
72 | | Args:
72+
73 | | distance: Distance traveled.
73+
74 | | time: Time spent traveling.
74+
75 | |
75+
76 | | Returns:
76+
77 | | Speed as distance divided by time.
77+
78 | | """
78+
| |_______^
79+
79 | try:
80+
80 | return distance / time
81+
|
82+
help: Add `ZeroDivisionError` to the docstring
83+
84+
DOC501 Raised exception `ValueError` missing from docstring
85+
--> DOC501_google.py:88:5
86+
|
87+
86 | # DOC501
88+
87 | def calculate_speed(distance: float, time: float) -> float:
89+
88 | / """Calculate speed as distance divided by time.
90+
89 | |
91+
90 | | Args:
92+
91 | | distance: Distance traveled.
93+
92 | | time: Time spent traveling.
94+
93 | |
95+
94 | | Returns:
96+
95 | | Speed as distance divided by time.
97+
96 | | """
98+
| |_______^
99+
97 | try:
100+
98 | return distance / time
101+
|
102+
help: Add `ValueError` to the docstring
103+
104+
DOC501 Raised exception `ZeroDivisionError` missing from docstring
105+
--> DOC501_google.py:88:5
106+
|
107+
86 | # DOC501
108+
87 | def calculate_speed(distance: float, time: float) -> float:
109+
88 | / """Calculate speed as distance divided by time.
110+
89 | |
111+
90 | | Args:
112+
91 | | distance: Distance traveled.
113+
92 | | time: Time spent traveling.
114+
93 | |
115+
94 | | Returns:
116+
95 | | Speed as distance divided by time.
117+
96 | | """
118+
| |_______^
119+
97 | try:
120+
98 | return distance / time
121+
|
122+
help: Add `ZeroDivisionError` to the docstring
123+
64124
DOC501 Raised exception `AnotherError` missing from docstring
65125
--> DOC501_google.py:106:5
66126
|

0 commit comments

Comments
 (0)