[pydoclint] Fix false positive on explicit exception re-raising (DOC501, DOC502)
#21011
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| DOC501 | 92 | 92 | 0 | 0 | 0 |
Contributor
Author
|
The +109 DOC501 violations represent improved detection accuracy rather than regressions. From what I'm seeing, the fix correctly identifies cases where exceptions are explicitly re-raised through variables like Previously, DOC501 missed these patterns because it couldn't resolve the variable |
ntBre
added
bug
ntBre
requested changes
Oct 21, 2025
Contributor
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! This looks like you're on the right track, but we need to make sure the tests would actually trigger on main, expand them to cover the tricky tuple case, and then I think we can simplify the code a bit too.
This is a minor detail, but would you try to update the PR title too? As the ecosystem report shows (thanks for your analysis there, it looked accurate when I spot-checked), these changes actually have a much larger impact on DOC501 than DOC502. We should mention both rules, though.
Comment on lines
881
to
888
| if let ast::Expr::Tuple(tuple) = exceptions { | ||
| // For tuple exceptions, we'll store the first one | ||
| if let Some(first_exception) = tuple.elts.first() { | ||
| store_exception_variable(first_exception); | ||
| } | ||
| } else { | ||
| store_exception_variable(exceptions); | ||
| } |
Contributor
There was a problem hiding this comment.
Why would we only store the first element in a tuple of exceptions? It seems like we should store all of them to me.
Comment on lines
+64
to
+82
| DOC501 Raised exception `ZeroDivisionError` missing from docstring | ||
| --> DOC501_google.py:70:5 | ||
| | | ||
| 68 | # DOC501 | ||
| 69 | def calculate_speed(distance: float, time: float) -> float: | ||
| 70 | / """Calculate speed as distance divided by time. | ||
| 71 | | | ||
| 72 | | Args: | ||
| 73 | | distance: Distance traveled. | ||
| 74 | | time: Time spent traveling. | ||
| 75 | | | ||
| 76 | | Returns: | ||
| 77 | | Speed as distance divided by time. | ||
| 78 | | """ | ||
| | |_______^ | ||
| 79 | try: | ||
| 80 | return distance / time | ||
| | | ||
| help: Add `ZeroDivisionError` to the docstring |
Contributor
There was a problem hiding this comment.
We shouldn't do this in this PR, but this looks like a nice place for a secondary annotation showing where the ZeroDivisionError is raised.
crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Outdated
Show resolved
Hide resolved
Update the pydoclint rule to correctly handle exception variables bound to tuples in except clauses. Now, when an exception variable is assigned from a tuple, all exception types in the tuple are considered for docstring checks, preventing false positives for DOC502. Test fixtures and snapshots are updated to reflect the improved behavior.
danparizher
changed the title
pydoclint] Fix false positive on explicit exception re-raising (DOC502)
danparizher
changed the title
DOC501, DOC502)
danparizher
changed the title
DOC501, DOC502)pydoclint] Fix false positive on explicit exception re-raising (DOC501, DOC502)
I also changed the range used in ExceptionEntry from exc.range() to stmt.range(), like the currently_suspended_exceptions case below. this range is not used for the diagnostic but for deduplicating/sorting exceptions referring to the same type. I think either exc.range() or stmt.range() will accomplish the same goal, but it seemed slightly preferable to be consistent in that case. there's still a bit of duplication here with the same Tuple case in both branches, but that seems okay.
ntBre
approved these changes
Oct 24, 2025
Contributor
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! This looks good. I pushed one commit factoring out the closure into a method instead of duplicating it. I'll just make sure I didn't break anything in the ecosystem report and then merge this.
dcreager
added a commit
that referenced
this pull request
Oct 27, 2025
* origin/main: Respect `--output-format` with `--watch` (#21097) [`pyflakes`] Revert to stable behavior if imports for module lie in alternate branches for `F401` (#20878) Fix finding keyword range for clause header after statement ending with semicolon (#21067) [ty] Fix bug where ty would think all types had an `__mro__` attribute (#20995) Restore `indent.py` (#21094) [`flake8-django`] Apply `DJ001` to annotated fields (#20907) Clearer error message when `line-length` goes beyond threshold (#21072) Update upload and download artifacts github actions (#21083) Update dependency mdformat-mkdocs to v4.4.2 (#21088) Update cargo-bins/cargo-binstall action to v1.15.9 (#21086) Update Rust crate clap to v4.5.50 (#21090) Update Rust crate get-size2 to v0.7.1 (#21091) Update Rust crate bstr to v1.12.1 (#21089) Add missing docstring sections to the numpy list (#20931) [`pydoclint`] Fix false positive on explicit exception re-raising (`DOC501`, `DOC502`) (#21011) [ty] Use constructor parameter types as type context (#21054)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #20973 (
docstring-extraneous-exception) false positive when exceptions mentioned in docstrings are caught and explicitly re-raised usingraise eorraise 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
exceptclauses.Root Cause: The
BodyVisitorincheck_docstring.rsonly checked for direct exception references (likeraise OSError()) but didn't recognize when a variable bound to an exception in anexceptclause was being re-raised.Example of the bug:
The issue occurred because
resolve_qualified_name(e)couldn't resolve the variableeto a qualified exception name, sinceeis just a variable binding, not a direct reference to an exception class.Approach
Modified the
BodyVisitorincrates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rsto:exception_variablesfield to map exception variable names to their exception types withinexceptclausesvisit_stmtto check if araisestatement uses a variable name that's bound to an exception in the currentexceptclauseexcepthandlers to prevent cross-contaminationKey changes:
exception_variables: FxHashMap<&'a str, QualifiedName<'a>>to track variable-to-exception mappingsvisit_except_handlerto store exception variable bindings when enteringexceptclausesvisit_stmtto check for variable-based re-raising:raise e→ lookupeinexception_variablesexcepthandlers to maintain proper scope