From 22b9b37b6ccda9c19be9757ee2c7e0a86a836b96 Mon Sep 17 00:00:00 2001 From: jsh9 <25124332+jsh9@users.noreply.github.com> Date: Sun, 12 Jan 2025 18:26:19 -0500 Subject: [PATCH] Fix false positive DOC405 & DOC201 for bare returns (#205) --- CHANGELOG.md | 6 ++ pydoclint/utils/return_yield_raise.py | 12 ++++ pydoclint/visitor.py | 15 ++++- .../23_bare_return_stmt_with_yield/google.py | 34 +++++++++++ tests/test_main.py | 57 +++++++++++++++++++ tests/utils/test_returns_yields_raise.py | 52 +++++++++++++++++ 6 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 tests/data/edge_cases/23_bare_return_stmt_with_yield/google.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ded091..7bf2171 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Change Log +## [Unpublished] + +- Fixed + - False positive DOC405 and DOC201 when we have bare return statements + together with `yield` statements + ## [0.5.18] - 2025-01-12 - Fixed diff --git a/pydoclint/utils/return_yield_raise.py b/pydoclint/utils/return_yield_raise.py index 8a43ad6..1d2c397 100644 --- a/pydoclint/utils/return_yield_raise.py +++ b/pydoclint/utils/return_yield_raise.py @@ -86,6 +86,18 @@ def isThisNodeAReturnStmt(node_: ast.AST) -> bool: return _hasExpectedStatements(node, isThisNodeAReturnStmt) +def hasBareReturnStatements(node: FuncOrAsyncFuncDef) -> bool: + """ + Check whether the function node has bare return statements (i.e., + just a "return" without anything behind it) + """ + + def isThisNodeABareReturnStmt(node_: ast.AST) -> bool: + return isinstance(node_, ast.Return) and node_.value is None + + return _hasExpectedStatements(node, isThisNodeABareReturnStmt) + + def hasRaiseStatements(node: FuncOrAsyncFuncDef) -> bool: """Check whether the function node has any raise statements""" diff --git a/pydoclint/visitor.py b/pydoclint/visitor.py index cac267e..4b4e2e5 100644 --- a/pydoclint/visitor.py +++ b/pydoclint/visitor.py @@ -19,6 +19,7 @@ from pydoclint.utils.return_arg import ReturnArg from pydoclint.utils.return_yield_raise import ( getRaisedExceptions, + hasBareReturnStatements, hasGeneratorAsReturnAnnotation, hasIteratorOrIterableAsReturnAnnotation, hasRaiseStatements, @@ -736,6 +737,11 @@ def my_function(num: int) -> Generator[int, None, str]: onlyHasYieldStmt: bool = hasYieldStmt and not hasReturnStmt hasReturnAnno: bool = hasReturnAnnotation(node) + if hasReturnStmt: + hasBareReturnStmt: bool = hasBareReturnStatements(node) + else: + hasBareReturnStmt = False # to save some time + returnAnno = ReturnAnnotation(unparseName(node.returns)) returnSec: list[ReturnArg] = doc.returnSection @@ -752,6 +758,11 @@ def my_function(num: int) -> Generator[int, None, str]: hasReturnAnno and not hasGenAsRetAnno )) + # If the return statement in the function body is a bare + # return, we don't throw DOC201 or DOC405. See more at: + # https://github.com/jsh9/pydoclint/issues/126#issuecomment-2136497913 + and not hasBareReturnStmt + # fmt: on ): retTypeInGenerator = extractReturnTypeFromGenerator( @@ -813,7 +824,9 @@ def my_function(num: int) -> Generator[int, None, str]: else: violations.append(v405) else: - if not hasGenAsRetAnno or not hasIterAsRetAnno: + if ( + not hasGenAsRetAnno or not hasIterAsRetAnno + ) and not hasBareReturnStmt: violations.append(v405) return violations diff --git a/tests/data/edge_cases/23_bare_return_stmt_with_yield/google.py b/tests/data/edge_cases/23_bare_return_stmt_with_yield/google.py new file mode 100644 index 0000000..5f757e2 --- /dev/null +++ b/tests/data/edge_cases/23_bare_return_stmt_with_yield/google.py @@ -0,0 +1,34 @@ +# From: https://github.com/jsh9/pydoclint/issues/126 + +from contextlib import contextmanager + + +@contextmanager +def my_func_1(db: Optional[int]) -> Iterator[int]: + """Test a function. + + Args: + db: the database + + Yields: + Some stuff. + """ + if db is not None: + yield db + return + + db = ... + yield db + + +def my_func_2(arg1: int) -> None: + """ + Test a function. + + Args: + arg1: some argument + + Returns: + The return value + """ + pass diff --git a/tests/test_main.py b/tests/test_main.py index 8e10ec2..4710a02 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1600,6 +1600,61 @@ def testNonAscii() -> None: {'style': 'numpy'}, [], ), + ( + '23_bare_return_stmt_with_yield/google.py', + { + 'style': 'google', + 'argTypeHintsInDocstring': False, + 'checkYieldTypes': False, + 'checkReturnTypes': True, + }, + [ + 'DOC203: Function `my_func_2` return type(s) in docstring not consistent with ' + "the return annotation. Return annotation types: ['None']; docstring return " + "section types: ['']" + ], + ), + ( + '23_bare_return_stmt_with_yield/google.py', + { + 'style': 'google', + 'argTypeHintsInDocstring': False, + 'checkYieldTypes': False, + 'checkReturnTypes': False, + }, + [], + ), + ( + '23_bare_return_stmt_with_yield/google.py', + { + 'style': 'google', + 'argTypeHintsInDocstring': False, + 'checkYieldTypes': True, + 'checkReturnTypes': True, + }, + [ + 'DOC404: Function `my_func_1` yield type(s) in docstring not consistent with ' + 'the return annotation. The yield type (the 0th arg in ' + 'Generator[...]/Iterator[...]): int; docstring "yields" section types:', + 'DOC203: Function `my_func_2` return type(s) in docstring not consistent with ' + "the return annotation. Return annotation types: ['None']; docstring return " + "section types: ['']", + ], + ), + ( + '23_bare_return_stmt_with_yield/google.py', + { + 'style': 'google', + 'argTypeHintsInDocstring': False, + 'checkYieldTypes': True, + 'checkReturnTypes': False, + }, + [ + 'DOC404: Function `my_func_1` yield type(s) in docstring not consistent with ' + 'the return annotation. The yield type (the 0th arg in ' + 'Generator[...]/Iterator[...]): int; docstring "yields" section types:', + ], + ), ], ) def testEdgeCases( @@ -1627,6 +1682,8 @@ def testPlayground() -> None: filename=DATA_DIR / 'playground.py', style='google', skipCheckingRaises=True, + argTypeHintsInDocstring=False, + checkYieldTypes=False, ) expected = [] assert list(map(str, violations)) == expected diff --git a/tests/utils/test_returns_yields_raise.py b/tests/utils/test_returns_yields_raise.py index 2f35291..482f5ae 100644 --- a/tests/utils/test_returns_yields_raise.py +++ b/tests/utils/test_returns_yields_raise.py @@ -7,6 +7,7 @@ from pydoclint.utils.generic import getFunctionId from pydoclint.utils.return_yield_raise import ( getRaisedExceptions, + hasBareReturnStatements, hasGeneratorAsReturnAnnotation, hasRaiseStatements, hasReturnAnnotation, @@ -83,6 +84,33 @@ def classmethod1_child1(): """ +src8 = """ +def func8(): + return +""" + + +src9 = """ +def func9(): + # In tested function, so it doesn't + # count as having a return statement + def func9_child1(): + return +""" + + +src10 = """ +def func10(): + # When mixed, we still consider it + # as having a bare return statement + if 1 > 2: + return 501 + + if 2 > 6: + return +""" + + @pytest.mark.parametrize( 'src, expected', [ @@ -92,6 +120,9 @@ def classmethod1_child1(): (src4, False), (src5, True), (src6, True), + (src8, True), + (src9, False), + (src10, True), ], ) def testHasReturnStatements(src: str, expected: bool) -> None: @@ -101,6 +132,27 @@ def testHasReturnStatements(src: str, expected: bool) -> None: assert hasReturnStatements(tree.body[0]) == expected +@pytest.mark.parametrize( + 'src, expected', + [ + (src1, False), + (src2, False), + (src3, False), + (src4, False), + (src5, False), + (src6, False), + (src8, True), + (src9, False), + (src10, True), + ], +) +def testHasBareReturnStatements(src: str, expected: bool) -> None: + tree = ast.parse(src) + assert len(tree.body) == 1 # sanity check + assert isinstance(tree.body[0], (ast.FunctionDef, ast.AsyncFunctionDef)) + assert hasBareReturnStatements(tree.body[0]) == expected + + def testHasReturnStatements_inClass() -> None: tree = ast.parse(src7) assert len(tree.body) == 1 # sanity check