diff --git a/_bmad-output/implementation-artifacts/sprint-status.yaml b/_bmad-output/implementation-artifacts/sprint-status.yaml index 5e61bf2..6f92618 100644 --- a/_bmad-output/implementation-artifacts/sprint-status.yaml +++ b/_bmad-output/implementation-artifacts/sprint-status.yaml @@ -1,4 +1,4 @@ -# generated: 2026-03-07 +# generated: 2026-03-24 # project: docvet # project_key: NOKEY # tracking_system: both @@ -33,7 +33,7 @@ # - SM typically creates next story after previous one is 'done' to incorporate learnings # - Dev moves story to 'review', then runs code-review (fresh context, different LLM recommended) -generated: 2026-03-07 +generated: 2026-03-24 project: docvet project_key: NOKEY tracking_system: both @@ -268,7 +268,7 @@ development_status: epic-32: in-progress 32-1-fix-feasibility-spike: done 32-2-fix-core-section-scaffolding-engine: done - 32-3-fix-cli-wiring-subcommand-dry-run-and-discovery: review + 32-3-fix-cli-wiring-subcommand-dry-run-and-discovery: done 32-4-inline-suppression-comments: backlog epic-32-retrospective: optional diff --git a/src/docvet/checks/enrichment/__init__.py b/src/docvet/checks/enrichment/__init__.py index d78c117..b0f8964 100644 --- a/src/docvet/checks/enrichment/__init__.py +++ b/src/docvet/checks/enrichment/__init__.py @@ -9,7 +9,8 @@ deprecation notices), ``_reverse`` (extra Raises/Yields/Returns), and ``_late_rules`` (trivial docstrings, return types, init params, scaffold-incomplete). This module retains section parsing, shared -constants, and the ``check_enrichment`` dispatch orchestrator. +constants, the ``_should_skip_reverse_check`` guard, and the +``check_enrichment`` dispatch orchestrator. Supports both Google-style and Sphinx/RST docstring conventions via the ``style`` parameter on :func:`check_enrichment`. NumPy-style section @@ -253,6 +254,7 @@ def _extract_section_content(docstring: str, section_name: str) -> str | None: # --------------------------------------------------------------------------- from ._forward import ( # noqa: E402 + _ABSTRACT_DECORATORS, # noqa: F401 – re-exported for tests _build_node_index, _check_missing_other_parameters, _check_missing_raises, @@ -261,10 +263,12 @@ def _extract_section_content(docstring: str, section_name: str) -> str | None: _check_missing_warns, _check_missing_yields, _extract_exception_name, # noqa: F401 – re-exported for submodules - _has_decorator, + _has_decorator, # noqa: F401 – re-exported for submodules + _is_abstract, _is_meaningful_return, # noqa: F401 – re-exported for submodules _is_property, # noqa: F401 – re-exported for submodules _is_stub_function, + _is_stub_statement, # noqa: F401 – re-exported for tests _is_warn_call, # noqa: F401 – re-exported for submodules _NodeT, ) @@ -277,8 +281,11 @@ def _should_skip_reverse_check(node: _NodeT) -> bool: intended behaviour for implementers, not current implementation. This aligns with ruff DOC202/403/502 and pydoclint skip logic. - Skips when the node is ``@abstractmethod`` or a stub function - (body is ``pass``, ``...``, or ``raise NotImplementedError``). + Skips when the node has any abstract decorator (``@abstractmethod`` + and deprecated variants ``@abstractclassmethod``, + ``@abstractstaticmethod``, ``@abstractproperty``) or is a stub + function (body is ``pass``, ``...``, ``raise NotImplementedError``, + or docstring-only). Args: node: The AST node for the symbol. @@ -286,7 +293,7 @@ def _should_skip_reverse_check(node: _NodeT) -> bool: Returns: ``True`` when reverse checks should be skipped for this node. """ - if _has_decorator(node, "abstractmethod"): + if _is_abstract(node): return True if _is_stub_function(node): return True diff --git a/src/docvet/checks/enrichment/_forward.py b/src/docvet/checks/enrichment/_forward.py index d39ca03..472f826 100644 --- a/src/docvet/checks/enrichment/_forward.py +++ b/src/docvet/checks/enrichment/_forward.py @@ -3,7 +3,10 @@ Detects missing docstring sections (Raises, Returns, Yields, Receives, Warns, Other Parameters) by walking the function's AST subtree. Each check function follows the uniform five-parameter dispatch signature -used by ``_RULE_DISPATCH`` in the enrichment orchestrator. +used by ``_RULE_DISPATCH`` in the enrichment orchestrator. Also provides +shared helpers for stub detection (``_is_stub_function``, +``_is_stub_statement``) and abstract decorator detection +(``_is_abstract``, ``_ABSTRACT_DECORATORS``). See Also: [`docvet.checks.enrichment`][]: Orchestrator and dispatch table. @@ -222,14 +225,70 @@ def _has_decorator(node: _NodeT, name: str) -> bool: return False +_ABSTRACT_DECORATORS = frozenset( + { + "abstractmethod", + "abstractclassmethod", + "abstractstaticmethod", + "abstractproperty", + } +) + + +def _is_abstract(node: _NodeT) -> bool: + """Check whether a function has any abstract decorator. + + Detects ``@abstractmethod`` and its deprecated predecessors + (``@abstractclassmethod``, ``@abstractstaticmethod``, + ``@abstractproperty``). Aligned with ruff's ``is_abstract()`` + in ``crates/ruff_python_semantic/src/analyze/visibility.rs``. + + Args: + node: The function or class AST node to inspect. + + Returns: + ``True`` when the node has any abstract decorator. + """ + return any(_has_decorator(node, name) for name in _ABSTRACT_DECORATORS) + + +def _is_stub_statement(stmt: ast.stmt) -> bool: + """Check whether a single AST statement is a stub-like statement. + + Recognises ``pass``, ``...`` (Ellipsis), ``raise NotImplementedError``, + and string literals (extra docstrings). Aligned with ruff's ``is_stub()`` + in ``crates/ruff_python_semantic/src/analyze/function_type.rs``. + + Args: + stmt: The AST statement node to inspect. + + Returns: + ``True`` when the statement is a stub-like statement. + """ + if isinstance(stmt, ast.Pass): + return True + if isinstance(stmt, ast.Expr) and isinstance(stmt.value, ast.Constant): + # Ellipsis (...) or string literal (extra docstring). + return stmt.value.value is ... or isinstance(stmt.value.value, str) + if isinstance(stmt, ast.Raise) and isinstance(stmt.exc, (ast.Name, ast.Call)): + exc = stmt.exc.func if isinstance(stmt.exc, ast.Call) else stmt.exc + return isinstance(exc, ast.Name) and exc.id == "NotImplementedError" + return False + + def _is_stub_function(node: _NodeT) -> bool: """Check whether a function body is a stub (no real implementation). - Stubs are single-statement bodies consisting of ``pass``, - ``...`` (Ellipsis), or ``raise NotImplementedError``. A leading - docstring ``Expr(Constant(str))`` is stripped before evaluating - body length so the helper works for both documented and - undocumented functions. + Stubs are bodies consisting entirely of stub-like statements: + ``pass``, ``...`` (Ellipsis), ``raise NotImplementedError``, or + string literals. A leading docstring ``Expr(Constant(str))`` is + stripped before evaluating so the helper works for both documented + and undocumented functions. A docstring-only body (empty after + stripping) is also a stub — this covers ``typing.Protocol`` methods + and interface contracts. + + Multi-statement stub bodies (e.g. ``pass; ...``) are accepted, + aligning with ruff's ``is_stub()`` semantics. Args: node: The function AST node to inspect. @@ -246,21 +305,9 @@ def _is_stub_function(node: _NodeT) -> bool: and isinstance(body[0].value.value, str) ): body = body[1:] - if len(body) != 1: - return False - stmt = body[0] - if isinstance(stmt, ast.Pass): - return True - if ( - isinstance(stmt, ast.Expr) - and isinstance(stmt.value, ast.Constant) - and stmt.value.value is ... - ): - return True - if isinstance(stmt, ast.Raise) and isinstance(stmt.exc, (ast.Name, ast.Call)): - exc = stmt.exc.func if isinstance(stmt.exc, ast.Call) else stmt.exc - return isinstance(exc, ast.Name) and exc.id == "NotImplementedError" - return False + if not body: + return True # Docstring-only body is a stub. + return all(_is_stub_statement(stmt) for stmt in body) def _should_skip_returns_check( @@ -273,8 +320,9 @@ def _should_skip_returns_check( Centralises guard clauses for :func:`_check_missing_returns` to keep the main function focused on AST walking. Skips when the symbol already has a ``Returns:`` section, is a dunder method, - ``@property``/``@cached_property``, ``@abstractmethod``, - ``@overload``, or a stub function. + ``@property``/``@cached_property``, any abstract decorator + (``@abstractmethod`` and deprecated variants), ``@overload``, or a + stub function. Args: symbol: The documented symbol to inspect. @@ -292,8 +340,8 @@ def _should_skip_returns_check( # @property and @cached_property methods if _is_property(node): return True - # @abstractmethod (interface contracts, not implementations) - if _has_decorator(node, "abstractmethod"): + # Abstract methods (interface contracts, not implementations) + if _is_abstract(node): return True # Stub functions (pass, ..., raise NotImplementedError) if _is_stub_function(node): diff --git a/tests/unit/checks/test_missing_returns.py b/tests/unit/checks/test_missing_returns.py index 4009fc1..fc43945 100644 --- a/tests/unit/checks/test_missing_returns.py +++ b/tests/unit/checks/test_missing_returns.py @@ -10,7 +10,9 @@ from docvet.checks.enrichment import ( _build_node_index, _check_missing_returns, + _is_abstract, _is_stub_function, + _is_stub_statement, _parse_sections, check_enrichment, ) @@ -645,3 +647,163 @@ def test_is_stub_function_documented_stub_with_raise(): func_node = tree.body[0] assert isinstance(func_node, ast.FunctionDef) assert _is_stub_function(func_node) is True + + +# --------------------------------------------------------------------------- +# _is_stub_function: docstring-only body (#387) +# --------------------------------------------------------------------------- + + +def test_is_stub_function_docstring_only_body(): + """Docstring-only body is a stub (Protocol methods, #387).""" + tree = ast.parse('def f():\n """Interface contract."""\n') + func_node = tree.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_stub_function(func_node) is True + + +def test_is_stub_function_docstring_only_method_in_protocol(): + """Protocol method with docstring-only body is a stub (#387).""" + source = ( + "from typing import Protocol\n" + "class P(Protocol):\n" + " def compute(self, x: int) -> float:\n" + ' """Compute a value.\n\n' + " Returns:\n" + " The computed float result.\n" + ' """\n' + ) + tree = ast.parse(source) + class_node = tree.body[1] + assert isinstance(class_node, ast.ClassDef) + func_node = class_node.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_stub_function(func_node) is True + + +# --------------------------------------------------------------------------- +# _is_stub_function: multi-statement stub bodies (#388) +# --------------------------------------------------------------------------- + + +def test_is_stub_function_multi_pass(): + """Multiple pass statements should be recognized as a stub (#388).""" + tree = ast.parse("def f():\n pass\n pass\n") + func_node = tree.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_stub_function(func_node) is True + + +def test_is_stub_function_pass_and_ellipsis(): + """Pass + ellipsis combination is a stub (#388).""" + tree = ast.parse("def f():\n pass\n ...\n") + func_node = tree.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_stub_function(func_node) is True + + +def test_is_stub_function_documented_multi_statement_stub(): + """Docstring + pass + ellipsis is a stub (#388).""" + tree = ast.parse('def f():\n """Doc."""\n pass\n ...\n') + func_node = tree.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_stub_function(func_node) is True + + +def test_is_stub_function_mixed_stub_and_real_not_stub(): + """Pass + real statement is NOT a stub.""" + tree = ast.parse("def f():\n pass\n return 1\n") + func_node = tree.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_stub_function(func_node) is False + + +# --------------------------------------------------------------------------- +# _is_stub_statement helper tests +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("code", "expected"), + [ + ("pass", True), + ("...", True), + ("raise NotImplementedError", True), + ("raise NotImplementedError('msg')", True), + ('"extra docstring"', True), + ("x = 1", False), + ("return 42", False), + ], + ids=[ + "pass", + "ellipsis", + "raise-NIE", + "raise-NIE-msg", + "string-literal", + "assign", + "return", + ], +) +def test_is_stub_statement(code, expected): + """Direct test of _is_stub_statement helper.""" + tree = ast.parse(code) + stmt = tree.body[0] + assert _is_stub_statement(stmt) is expected + + +# --------------------------------------------------------------------------- +# _is_abstract helper tests (#389) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("decorator", "expected"), + [ + ("abstractmethod", True), + ("abstractclassmethod", True), + ("abstractstaticmethod", True), + ("abstractproperty", True), + ("staticmethod", False), + ("classmethod", False), + ("property", False), + ], + ids=[ + "abstractmethod", + "abstractclassmethod", + "abstractstaticmethod", + "abstractproperty", + "staticmethod", + "classmethod", + "property", + ], +) +def test_is_abstract(decorator, expected): + """_is_abstract detects all abstract decorators (#389).""" + tree = ast.parse(f"@{decorator}\ndef f():\n pass\n") + func_node = tree.body[0] + assert isinstance(func_node, ast.FunctionDef) + assert _is_abstract(func_node) is expected + + +def test_is_abstract_qualified_decorator(): + """_is_abstract detects abc.abstractmethod (qualified form).""" + tree = ast.parse("import abc\n@abc.abstractmethod\ndef f():\n pass\n") + func_node = tree.body[1] + assert isinstance(func_node, ast.FunctionDef) + assert _is_abstract(func_node) is True + + +def test_missing_returns_skips_abstractclassmethod(): + """missing-returns skips @abstractclassmethod (#389).""" + source = ( + "import abc\n" + "class C(abc.ABC):\n" + " @abc.abstractclassmethod\n" + " def create(cls) -> 'C':\n" + ' """Create instance."""\n' + " return cls()\n" + ) + tree = ast.parse(source) + config = EnrichmentConfig() + findings = check_enrichment(source, tree, config, "test.py") + assert not any(f.rule == "missing-returns" for f in findings) diff --git a/tests/unit/checks/test_reverse_enrichment.py b/tests/unit/checks/test_reverse_enrichment.py index 79ada0a..0fadf93 100644 --- a/tests/unit/checks/test_reverse_enrichment.py +++ b/tests/unit/checks/test_reverse_enrichment.py @@ -1011,3 +1011,208 @@ def foo(x: int) -> int: findings = check_enrichment(textwrap.dedent(source), tree, config, "test.py") assert not any(f.rule == "extra-returns-in-docstring" for f in findings) + + +# =========================================================================== +# Bug fix tests (#387, #388, #389) +# =========================================================================== + + +class TestDocstringOnlyBodySkip: + """Tests for docstring-only stub body skip (#387).""" + + def test_protocol_method_docstring_only_returns_skipped(self): + """Protocol method with docstring-only body — no extra-returns finding (#387).""" + source = '''\ + from typing import Protocol + + class MyProtocol(Protocol): + def compute(self, x: int) -> float: + """Compute a value. + + Returns: + The computed float result. + """ + ''' + from docvet.ast_utils import get_documented_symbols + + tree = ast.parse(textwrap.dedent(source)) + symbols = get_documented_symbols(tree) + node_index = _build_node_index(tree) + symbol = [s for s in symbols if s.name == "compute"][0] + assert symbol.docstring is not None + sections = _parse_sections(symbol.docstring) + config = EnrichmentConfig() + + result = _check_extra_returns_in_docstring( + symbol, sections, node_index, config, "test.py" + ) + + assert result is None + + def test_docstring_only_body_yields_skipped(self): + """Docstring-only body — no extra-yields finding (#387).""" + source = '''\ + def gen(): + """Generate values. + + Yields: + int: Numbers. + """ + ''' + symbol, node_index, _ = _make_symbol_and_index(source) + sections = _parse_sections(symbol.docstring) + config = EnrichmentConfig() + + result = _check_extra_yields_in_docstring( + symbol, sections, node_index, config, "test.py" + ) + + assert result is None + + def test_docstring_only_body_raises_skipped(self): + """Docstring-only body — no extra-raises finding (#387).""" + source = '''\ + def validate(): + """Validate input. + + Raises: + ValueError: If invalid. + """ + ''' + symbol, node_index, _ = _make_symbol_and_index(source) + sections = _parse_sections(symbol.docstring) + config = EnrichmentConfig() + + result = _check_extra_raises_in_docstring( + symbol, sections, node_index, config, "test.py" + ) + + assert result is None + + def test_docstring_only_skip_via_orchestrator(self): + """Orchestrator-level: docstring-only Protocol method emits no reverse findings (#387).""" + source = textwrap.dedent('''\ + from typing import Protocol + + class MyProtocol(Protocol): + def compute(self, x: int) -> float: + """Compute a value. + + Returns: + The computed float result. + """ + ''') + tree = ast.parse(source) + config = EnrichmentConfig() + + findings = check_enrichment(source, tree, config, "test.py") + + reverse_rules = {f.rule for f in findings if f.rule.startswith("extra-")} + assert not reverse_rules + + +class TestDeprecatedAbstractDecorators: + """Tests for deprecated abstract decorator skip (#389).""" + + @pytest.mark.parametrize( + "decorator", + [ + "abstractclassmethod", + "abstractstaticmethod", + "abstractproperty", + ], + ) + def test_deprecated_abstract_returns_skipped(self, decorator): + """Deprecated abstract decorator — no extra-returns finding (#389).""" + source = f'''\ + import abc + + class Base(abc.ABC): + @abc.{decorator} + def method(self): + """Summary. + + Returns: + str: The result. + """ + x = 1 + ''' + from docvet.ast_utils import get_documented_symbols + + tree = ast.parse(textwrap.dedent(source)) + symbols = get_documented_symbols(tree) + node_index = _build_node_index(tree) + symbol = [s for s in symbols if s.name == "method"][0] + assert symbol.docstring is not None + sections = _parse_sections(symbol.docstring) + config = EnrichmentConfig() + + result = _check_extra_returns_in_docstring( + symbol, sections, node_index, config, "test.py" + ) + + assert result is None + + @pytest.mark.parametrize( + "decorator", + [ + "abstractclassmethod", + "abstractstaticmethod", + "abstractproperty", + ], + ) + def test_deprecated_abstract_skip_directly(self, decorator): + """_should_skip_reverse_check returns True for deprecated decorators (#389).""" + source = f'''\ + import abc + + class Base(abc.ABC): + @abc.{decorator} + def method(self): + """Summary.""" + return 42 + ''' + from docvet.ast_utils import get_documented_symbols + + tree = ast.parse(textwrap.dedent(source)) + symbols = get_documented_symbols(tree) + node_index = _build_node_index(tree) + target = [s for s in symbols if s.kind in ("function", "method")][0] + node = node_index.get(target.line) + assert node is not None + + result = _should_skip_reverse_check(node) + + assert result is True + + def test_deprecated_abstract_yields_skipped(self): + """@abstractstaticmethod — no extra-yields finding (#389).""" + source = '''\ + import abc + + class Base(abc.ABC): + @abc.abstractstaticmethod + def gen(): + """Generate values. + + Yields: + int: Numbers. + """ + x = 1 + ''' + from docvet.ast_utils import get_documented_symbols + + tree = ast.parse(textwrap.dedent(source)) + symbols = get_documented_symbols(tree) + node_index = _build_node_index(tree) + symbol = [s for s in symbols if s.name == "gen"][0] + assert symbol.docstring is not None + sections = _parse_sections(symbol.docstring) + config = EnrichmentConfig() + + result = _check_extra_yields_in_docstring( + symbol, sections, node_index, config, "test.py" + ) + + assert result is None