From f75860489c4d4769767beabcad9b099cc2b2a88d Mon Sep 17 00:00:00 2001 From: Amar Paul Date: Mon, 2 Sep 2024 20:39:36 -0400 Subject: [PATCH] feat: introduce DOC503 for checking specific raised exceptions (#161) Co-authored-by: jsh9 <25124332+jsh9@users.noreply.github.com> --- CHANGELOG.md | 7 +- docs/violation_codes.md | 1 + pydoclint/utils/return_yield_raise.py | 63 +++++++++++- pydoclint/utils/violation.py | 1 + pydoclint/utils/visitor_helper.py | 27 +++++ pydoclint/utils/walk.py | 7 ++ pydoclint/visitor.py | 30 ++++++ setup.cfg | 2 +- tests/data/google/raises/cases.py | 39 +++++++ tests/data/numpy/raises/cases.py | 51 +++++++++ tests/data/sphinx/raises/cases.py | 32 ++++++ tests/test_main.py | 23 +++++ tests/utils/test_returns_yields_raise.py | 126 ++++++++++++++++++++++- tests/utils/test_walk.py | 48 ++++++++- 14 files changed, 452 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a71bad..f9e018d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ # Change Log -## [unpublished] - 2024-08-05 +## [0.5.7] - 2024-09-02 + +- Added + + - A new violation code, `DOC503`, which checks that exceptions in the + function body match those in the "Raises" section of the docstring - Changed - Switched from tab to 4 spaces in baseline diff --git a/docs/violation_codes.md b/docs/violation_codes.md index 063d994..c5ea278 100644 --- a/docs/violation_codes.md +++ b/docs/violation_codes.md @@ -87,6 +87,7 @@ have a return section. | -------- | --------------------------------------------------------------------------------------------------------- | | `DOC501` | Function/method has "raise" statements, but the docstring does not have a "Raises" section | | `DOC502` | Function/method has a "Raises" section in the docstring, but there are not "raise" statements in the body | +| `DOC503` | Exceptions in the "Raises" section in the docstring do not match those in the function body | ## 6. `DOC6xx`: Violations about class attributes diff --git a/pydoclint/utils/return_yield_raise.py b/pydoclint/utils/return_yield_raise.py index 3108fde..1984e61 100644 --- a/pydoclint/utils/return_yield_raise.py +++ b/pydoclint/utils/return_yield_raise.py @@ -1,5 +1,5 @@ import ast -from typing import Callable, Dict, Tuple, Type +from typing import Callable, Dict, Generator, List, Optional, Tuple, Type from pydoclint.utils import walk from pydoclint.utils.annotation import unparseAnnotation @@ -93,6 +93,67 @@ def isThisNodeARaiseStmt(node_: ast.AST) -> bool: return _hasExpectedStatements(node, isThisNodeARaiseStmt) +def getRaisedExceptions(node: FuncOrAsyncFuncDef) -> List[str]: + """Get the raised exceptions in a function node as a sorted list""" + return sorted(set(_getRaisedExceptions(node))) + + +def _getRaisedExceptions( + node: FuncOrAsyncFuncDef, +) -> Generator[str, None, None]: + """Yield the raised exceptions in a function node""" + childLineNum: int = -999 + + # key: child lineno, value: (parent lineno, is parent a function?) + familyTree: Dict[int, Tuple[int, bool]] = {} + + currentParentExceptHandler: Optional[ast.ExceptHandler] = None + + # Depth-first guarantees the last-seen exception handler + # is a parent of child. + for child, parent in walk.walk_dfs(node): + childLineNum = _updateFamilyTree(child, parent, familyTree) + + if isinstance(parent, ast.ExceptHandler): + currentParentExceptHandler = parent + + if ( + isinstance(child, ast.Raise) + and isinstance( + parent, + (ast.AsyncFunctionDef, ast.FunctionDef, BlockType), + ) + and _confirmThisStmtIsNotWithinNestedFunc( + foundStatementTemp=True, + familyTree=familyTree, + lineNumOfStatement=childLineNum, + lineNumOfThisNode=node.lineno, + ) + ): + for subnode, _ in walk.walk_dfs(child): + if isinstance(subnode, ast.Name): + yield subnode.id + break + else: + # if "raise" statement was alone, it must be inside an "except" + if currentParentExceptHandler: + yield from _extractExceptionsFromExcept( + currentParentExceptHandler, + ) + + +def _extractExceptionsFromExcept( + node: ast.ExceptHandler, +) -> Generator[str, None, None]: + if isinstance(node.type, ast.Name): + yield node.type.id + + if isinstance(node.type, ast.Tuple): + for child, _ in walk.walk(node.type): + if isinstance(child, ast.Name): + yield child.id + + def _hasExpectedStatements( node: FuncOrAsyncFuncDef, isThisNodeAnExpectedStmt: Callable[[ast.AST], bool], diff --git a/pydoclint/utils/violation.py b/pydoclint/utils/violation.py index 4a987b0..18294e0 100644 --- a/pydoclint/utils/violation.py +++ b/pydoclint/utils/violation.py @@ -52,6 +52,7 @@ 501: 'has "raise" statements, but the docstring does not have a "Raises" section', 502: 'has a "Raises" section in the docstring, but there are not "raise" statements in the body', + 503: 'exceptions in the "Raises" section in the docstring do not match those in the function body', 601: 'Class docstring contains fewer class attributes than actual class attributes.', 602: 'Class docstring contains more class attributes than in actual class attributes.', diff --git a/pydoclint/utils/visitor_helper.py b/pydoclint/utils/visitor_helper.py index 5721bdc..0ea5f38 100644 --- a/pydoclint/utils/visitor_helper.py +++ b/pydoclint/utils/visitor_helper.py @@ -521,3 +521,30 @@ def extractReturnTypeFromGenerator(returnAnnoText: str) -> str: returnType = returnAnnoText return stripQuotes(returnType) + + +def addMismatchedRaisesExceptionViolation( + *, + docRaises: List[str], + actualRaises: List[str], + violations: List[Violation], + violationForRaisesMismatch: Violation, # such as V503 + lineNum: int, + msgPrefix: str, +) -> None: + """ + Add a violation for mismatched exception type between function + body and docstring + """ + msgPostfix: str = ( + f'Raises values in the docstring: {docRaises}.' + f' Raised exceptions in the body: {actualRaises}.' + ) + violations.append( + Violation( + code=violationForRaisesMismatch.code, + line=lineNum, + msgPrefix=msgPrefix, + msgPostfix=msgPostfix, + ) + ) diff --git a/pydoclint/utils/walk.py b/pydoclint/utils/walk.py index 3c16d0a..0f809b4 100644 --- a/pydoclint/utils/walk.py +++ b/pydoclint/utils/walk.py @@ -32,6 +32,13 @@ def walk(node): yield node, parent +def walk_dfs(node): + """Depth-first traversal of AST. Modified from `walk()` in this file""" + for child, parent in iter_child_nodes(node): + yield child, parent + yield from walk_dfs(child) + + def iter_child_nodes(node): """ Yield all direct child nodes of *node*, that is, all fields that are nodes diff --git a/pydoclint/visitor.py b/pydoclint/visitor.py index f501551..ccfc5a0 100644 --- a/pydoclint/visitor.py +++ b/pydoclint/visitor.py @@ -17,6 +17,7 @@ from pydoclint.utils.return_anno import ReturnAnnotation from pydoclint.utils.return_arg import ReturnArg from pydoclint.utils.return_yield_raise import ( + getRaisedExceptions, hasGeneratorAsReturnAnnotation, hasIteratorOrIterableAsReturnAnnotation, hasRaiseStatements, @@ -32,6 +33,7 @@ ) from pydoclint.utils.violation import Violation from pydoclint.utils.visitor_helper import ( + addMismatchedRaisesExceptionViolation, checkClassAttributesAgainstClassDocstring, checkDocArgsLengthAgainstActualArgs, checkNameOrderAndTypeHintsOfDocArgsAgainstActualArgs, @@ -811,6 +813,7 @@ def checkRaises( v501 = Violation(code=501, line=lineNum, msgPrefix=msgPrefix) v502 = Violation(code=502, line=lineNum, msgPrefix=msgPrefix) + v503 = Violation(code=503, line=lineNum, msgPrefix=msgPrefix) docstringHasRaisesSection: bool = doc.hasRaisesSection hasRaiseStmt: bool = hasRaiseStatements(node) @@ -822,4 +825,31 @@ def checkRaises( if not self.isAbstractMethod: violations.append(v502) + # check that the raise statements match those in body. + if hasRaiseStmt: + docRaises: List[str] = [] + + for raises in doc.parsed.raises: + if raises.type_name: + docRaises.append(raises.type_name) + elif doc.style == 'sphinx' and raises.description: + # :raises: Exception: -> 'Exception' + splitDesc = raises.description.split(':') + if len(splitDesc) > 1 and ' ' not in splitDesc[0].strip(): + exc = splitDesc[0].strip() + docRaises.append(exc) + + docRaises.sort() + actualRaises = getRaisedExceptions(node) + + if docRaises != actualRaises: + addMismatchedRaisesExceptionViolation( + docRaises=docRaises, + actualRaises=actualRaises, + violations=violations, + violationForRaisesMismatch=v503, + lineNum=lineNum, + msgPrefix=msgPrefix, + ) + return violations diff --git a/setup.cfg b/setup.cfg index 0d618f1..030d304 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [metadata] name = pydoclint -version = 0.5.6 +version = 0.5.7 description = A Python docstring linter that checks arguments, returns, yields, and raises sections long_description = file: README.md long_description_content_type = text/markdown diff --git a/tests/data/google/raises/cases.py b/tests/data/google/raises/cases.py index 21b0565..acc898f 100644 --- a/tests/data/google/raises/cases.py +++ b/tests/data/google/raises/cases.py @@ -143,3 +143,42 @@ def inner9a(arg1) -> None: raise print(arg0) + + def func11(self, arg0) -> None: + """ + This docstring doesn't specify all the raised exceptions. + + Args: + arg0: Arg 0 + + Raises: + TypeError: if arg0 is 0. + """ + if arg0 == 0: + raise TypeError + raise ValueError + + def func12(self, arg0) -> None: + """ + There should not be any violations in this method. + + Args: + arg0: Arg 0 + + Raises: + TypeError: if arg0 is 0. + ValueError: otherwise. + """ + if arg0 == 0: + raise TypeError + raise ValueError + + def func13(self) -> None: + """ + Should raise an error due to duplicated raises. + + Raises: + ValueError: all the time. + ValueError: typo! + """ + raise ValueError diff --git a/tests/data/numpy/raises/cases.py b/tests/data/numpy/raises/cases.py index 5982582..2e7db3f 100644 --- a/tests/data/numpy/raises/cases.py +++ b/tests/data/numpy/raises/cases.py @@ -178,3 +178,54 @@ def inner9a(arg1) -> None: raise print(arg0) + + def func11(self, arg0) -> None: + """ + This docstring doesn't specify all the raised exceptions. + + Parameters + ---------- + arg0 + Arg 0 + + Raises + ------ + TypeError + if arg0 is 0. + """ + if arg0 == 0: + raise TypeError + raise ValueError + + def func12(self, arg0) -> None: + """ + There should not be any violations in this method. + + Parameters + ---------- + arg0 + Arg 0 + + Raises + ------ + TypeError + if arg0 is 0. + ValueError + otherwise. + """ + if arg0 == 0: + raise TypeError + raise ValueError + + def func13(self) -> None: + """ + Should raise an error due to duplicated raises. + + Raises + ------ + ValueError + all the time. + ValueError + typo! + """ + raise ValueError diff --git a/tests/data/sphinx/raises/cases.py b/tests/data/sphinx/raises/cases.py index 2070682..d4368fb 100644 --- a/tests/data/sphinx/raises/cases.py +++ b/tests/data/sphinx/raises/cases.py @@ -121,3 +121,35 @@ def inner9a(arg1) -> None: raise print(arg0) + + def func11(self, arg0) -> None: + """ + This docstring doesn't specify all the raised exceptions. + + :param arg0: Arg 0 + :raises TypeError: if arg0 is zero. + """ + if arg0 == 0: + raise TypeError + raise ValueError + + def func12(self, arg0) -> None: + """ + There should not be any violations in this method. + + :param arg0: Arg 0 + :raises TypeError: if arg0 is zero. + :raises ValueError: otherwise. + """ + if arg0 == 0: + raise TypeError + raise ValueError + + def func13(self) -> None: + """ + Should raise an error due to duplicated raises. + + :raises ValueError: all the time. + :raises ValueError: typo! + """ + raise ValueError diff --git a/tests/test_main.py b/tests/test_main.py index 939b7dd..7aae66a 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -609,6 +609,9 @@ def testAllowInitDocstring(style: str) -> None: 'because __init__() cannot return anything', 'DOC305: Class `C`: Class docstring has a "Raises" section; please put it in ' 'the __init__() docstring', + 'DOC503: Method `C.__init__` exceptions in the "Raises" section in the ' + 'docstring do not match those in the function body Raises values in the ' + "docstring: ['TypeError']. Raised exceptions in the body: ['ValueError'].", 'DOC306: Class `D`: The class docstring does not need a "Yields" section, ' 'because __init__() cannot yield anything', 'DOC307: Class `D`: The __init__() docstring does not need a "Yields" ' @@ -804,6 +807,12 @@ def testRaises(style: str, skipRaisesCheck: bool) -> None: expected0 = [ 'DOC501: Method `B.func1` has "raise" statements, but the docstring does not ' 'have a "Raises" section', + 'DOC503: Method `B.func1` exceptions in the "Raises" section in the docstring ' + 'do not match those in the function body Raises values in the docstring: []. ' + "Raised exceptions in the body: ['ValueError'].", + 'DOC503: Method `B.func4` exceptions in the "Raises" section in the docstring ' + 'do not match those in the function body Raises values in the docstring: ' + "['CurtomError']. Raised exceptions in the body: ['CustomError'].", 'DOC502: Method `B.func5` has a "Raises" section in the docstring, but there ' 'are not "raise" statements in the body', 'DOC502: Method `B.func7` has a "Raises" section in the docstring, but there ' @@ -812,6 +821,17 @@ def testRaises(style: str, skipRaisesCheck: bool) -> None: 'are not "raise" statements in the body', 'DOC501: Function `inner9a` has "raise" statements, but the docstring does ' 'not have a "Raises" section', + 'DOC503: Function `inner9a` exceptions in the "Raises" section in the ' + 'docstring do not match those in the function body Raises values in the ' + "docstring: []. Raised exceptions in the body: ['FileNotFoundError'].", + 'DOC503: Method `B.func11` exceptions in the "Raises" section in the ' + 'docstring do not match those in the function body Raises values in the ' + "docstring: ['TypeError']. Raised exceptions in the body: ['TypeError', " + "'ValueError'].", + 'DOC503: Method `B.func13` exceptions in the "Raises" section in the ' + 'docstring do not match those in the function body Raises values in the ' + "docstring: ['ValueError', 'ValueError']. Raised exceptions in the body: " + "['ValueError'].", ] expected1 = [] expected = expected1 if skipRaisesCheck else expected0 @@ -841,6 +861,9 @@ def testRaisesPy310plus(style: str, skipRaisesCheck: bool) -> None: expected0 = [ 'DOC501: Method `B.func10` has "raise" statements, but the docstring does not ' 'have a "Raises" section', + 'DOC503: Method `B.func10` exceptions in the "Raises" section in the ' + 'docstring do not match those in the function body Raises values in the ' + "docstring: []. Raised exceptions in the body: ['ValueError'].", ] expected1 = [] expected = expected1 if skipRaisesCheck else expected0 diff --git a/tests/utils/test_returns_yields_raise.py b/tests/utils/test_returns_yields_raise.py index d9a0168..15a151f 100644 --- a/tests/utils/test_returns_yields_raise.py +++ b/tests/utils/test_returns_yields_raise.py @@ -1,11 +1,12 @@ import ast -from typing import Dict, Tuple +from typing import Dict, List, Tuple import pytest from pydoclint.utils.astTypes import FuncOrAsyncFuncDef from pydoclint.utils.generic import getFunctionId from pydoclint.utils.return_yield_raise import ( + getRaisedExceptions, hasGeneratorAsReturnAnnotation, hasRaiseStatements, hasReturnAnnotation, @@ -118,6 +119,7 @@ def __init__(self): self.returnStatements: Dict[Tuple[int, int, str], bool] = {} self.yieldStatements: Dict[Tuple[int, int, str], bool] = {} self.raiseStatements: Dict[Tuple[int, int, str], bool] = {} + self.raisedExceptions: Dict[Tuple[int, int, str], List[str]] = {} self.returnAnnotations: Dict[Tuple[int, int, str], bool] = {} self.generatorAnnotations: Dict[Tuple[int, int, str], bool] = {} @@ -126,6 +128,7 @@ def visit_FunctionDef(self, node: FuncOrAsyncFuncDef): self.returnStatements[functionId] = hasReturnStatements(node) self.yieldStatements[functionId] = hasYieldStatements(node) self.raiseStatements[functionId] = hasRaiseStatements(node) + self.raisedExceptions[functionId] = getRaisedExceptions(node) self.returnAnnotations[functionId] = hasReturnAnnotation(node) self.generatorAnnotations[functionId] = hasGeneratorAsReturnAnnotation( node, @@ -328,6 +331,91 @@ def func6(arg1): raise TypeError return arg1 + 2 + +def func7(arg0): + if True: + raise TypeError + + try: + "foo"[-10] + except IndexError as e: + raise + + try: + 1 / 0 + except ZeroDivisionError: + raise RuntimeError("a different error") + + try: + pass + except OSError as e: + if e.args[0] == 2 and e.filename: + fp = None + else: + raise + +def func8(d): + try: + d[0][0] + except (KeyError, TypeError): + raise + finally: + pass + +def func9(d): + try: + d[0] + except IndexError: + try: + d[0][0] + except KeyError: + raise AssertionError() from e + except Exception: + pass + if True: + raise + +def func10(): + # no variable resolution is done. this function looks like it throws GError. + GError = ZeroDivisionError + try: + 1 / 0 + except GError: + raise + +def func11(a): + # Duplicated exceptions will only be reported once + if a < 1: + raise ValueError + + if a < 2: + raise ValueError + + if a < 3: + raise ValueError + + if a < 4: + raise ValueError + + if a < 5: + raise ValueError + +def func12(a): + # Exceptions will be reported in alphabetical order, regardless of + # the order they are raised within the function body + + Error1 = RuntimeError + Error2 = ValueError + Error3 = TypeError + + if a < 1: + raise Error2 + + if a < 2: + raise Error1 + + if a < 3: + raise Error3 """ @@ -345,6 +433,42 @@ def testHasRaiseStatements() -> None: (20, 0, 'func5'): False, (26, 0, 'func6'): True, (21, 4, 'func5_child1'): True, + (32, 0, 'func7'): True, + (54, 0, 'func8'): True, + (62, 0, 'func9'): True, + (75, 0, 'func10'): True, + (83, 0, 'func11'): True, + (100, 0, 'func12'): True, + } + + assert result == expected + + +def testWhichRaiseStatements() -> None: + tree = ast.parse(srcRaises) + visitor = HelperVisitor() + visitor.visit(tree) + result = visitor.raisedExceptions + + expected = { + (2, 0, 'func1'): ['ValueError'], + (7, 0, 'func2'): ['Exception'], + (10, 0, 'func3'): ['TypeError'], + (17, 0, 'func4'): ['CustomError'], + (20, 0, 'func5'): [], + (26, 0, 'func6'): ['TypeError'], + (21, 4, 'func5_child1'): ['ValueError'], + (32, 0, 'func7'): [ + 'IndexError', + 'OSError', + 'RuntimeError', + 'TypeError', + ], + (54, 0, 'func8'): ['KeyError', 'TypeError'], + (62, 0, 'func9'): ['AssertionError', 'IndexError'], + (75, 0, 'func10'): ['GError'], + (83, 0, 'func11'): ['ValueError'], + (100, 0, 'func12'): ['Error1', 'Error2', 'Error3'], } assert result == expected diff --git a/tests/utils/test_walk.py b/tests/utils/test_walk.py index 069c205..3d99040 100644 --- a/tests/utils/test_walk.py +++ b/tests/utils/test_walk.py @@ -3,7 +3,7 @@ import pytest -from pydoclint.utils.walk import walk +from pydoclint.utils.walk import walk, walk_dfs src1 = """ def func1(): @@ -96,3 +96,49 @@ def testWalk(src: str, expected: List[Tuple[str, str]]) -> None: result.append((node.name, parent_repr)) assert result == expected + + +@pytest.mark.parametrize( + 'src, expected', + [ + ( + src1, + [ + ('func1', 'ast.Module'), + ('func2', 'ast.Module'), + ('func3', 'ast.Module'), + ('func3_child1', 'func3'), + ('func3_child1_grandchild1', 'func3_child1'), + ('func4', 'ast.Module'), + ('func4_child1', 'func4'), + ('func4_child2', 'func4'), + ('func4_child2_grandchild1', 'func4_child2'), + ('func5', 'ast.Module'), + ('MyClass', 'ast.Module'), + ('__init__', 'MyClass'), + ('method1', 'MyClass'), + ('method1_child1', 'method1'), + ('classmethod1', 'MyClass'), + ('classmethod1_child1', 'classmethod1'), + ], + ), + ], +) +def testWalkDfs(src: str, expected: List[Tuple[str, str]]) -> None: + result: List[Tuple[str, str]] = [] + tree = ast.parse(src) + for node, parent in walk_dfs(tree): + if 'name' in node.__dict__: + parent_repr: str + if isinstance(parent, ast.Module): + parent_repr = 'ast.Module' + elif isinstance( + parent, (ast.AsyncFunctionDef, ast.FunctionDef, ast.ClassDef) + ): + parent_repr = parent.name + else: + parent_repr = str(type(parent)) + + result.append((node.name, parent_repr)) + + assert result == expected