[pydoclint] Permit yielding None in DOC402 and DOC403 (#13148)

Co-authored-by: Alex Waygood <alex.waygood@gmail.com>

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

@@ -187,3 +187,50 @@ def foo(s: str) -> str | None:
         A string.
     """
     return None
+
+
+# DOC201
+def bar() -> int | None:
+    """Bar-y method"""
+    return
+
+
+from collections.abc import Iterator, Generator
+
+
+# This is okay -- returning `None` is implied by `Iterator[str]`;
+# no need to document it
+def generator_function() -> Iterator[str]:
+    """Generate some strings"""
+    yield from "abc"
+    return
+
+
+# This is okay -- returning `None` is stated by `Generator[str, None, None]`;
+# no need to document it
+def generator_function_2() -> Generator[str, None, None]:
+    """Generate some strings"""
+    yield from "abc"
+    return
+
+
+# DOC201 -- returns None but `Generator[str, None, int | None]`
+# indicates it could sometimes return `int`
+def generator_function_3() -> Generator[str, None, int | None]:
+    """Generate some strings"""
+    yield from "abc"
+    return
+
+
+# DOC201 -- no type annotation and a non-None return
+# indicates it could sometimes return `int`
+def generator_function_4():
+    """Generate some strings"""
+    yield from "abc"
+    return 42
+
+
+# DOC201 -- no `yield` expressions, so not a generator function
+def not_a_generator() -> Iterator[int]:
+    """"No returns documented here, oh no"""
+    return (x for x in range(42))

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

@@ -73,3 +73,15 @@ class A(metaclass=abc.abcmeta):
             dict: The values
         """
         return
+
+
+# DOC202 -- never explicitly returns anything, just short-circuits
+def foo(s: str, condition: bool):
+    """Fooey things.
+
+    Returns:
+        None
+    """
+    if not condition:
+        return
+    print(s)

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

@@ -87,3 +87,22 @@ def f(num: int):
         num (int): A number
     """
     yield 1
+
+
+import collections.abc
+
+
+# DOC402
+def foo() -> collections.abc.Generator[int | None, None, None]:
+    """
+    Do something
+    """
+    yield
+
+
+# DOC402
+def bar() -> collections.abc.Iterator[int | None]:
+    """
+    Do something
+    """
+    yield

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

@@ -60,3 +60,81 @@ class Bar:
             A number
         """
         yield 'test'
+
+
+import typing
+
+
+# OK
+def foo() -> typing.Generator[None, None, None]:
+    """
+    Do something
+    """
+    yield None
+
+
+# OK
+def foo() -> typing.Generator[None, None, None]:
+    """
+    Do something
+    """
+    yield
+
+
+# DOC402
+def foo() -> typing.Generator[int | None, None, None]:
+    """
+    Do something
+    """
+    yield None
+    yield 1
+
+
+# DOC402
+def foo() -> typing.Generator[int, None, None]:
+    """
+    Do something
+    """
+    yield None
+
+
+# OK
+def foo():
+    """
+    Do something
+    """
+    yield None
+
+
+# OK
+def foo():
+    """
+    Do something
+    """
+    yield
+
+
+# DOC402
+def foo():
+    """
+    Do something
+    """
+    yield None
+    yield 1
+
+
+# DOC402
+def foo():
+    """
+    Do something
+    """
+    yield 1
+    yield
+
+
+# DOC402
+def bar() -> typing.Iterator[int | None]:
+    """
+    Do something
+    """
+    yield

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

@@ -48,3 +48,50 @@ class Bar:
             num (int): A number
         """
         print('test')
+
+
+import typing
+
+
+# OK
+def foo() -> typing.Generator[None, None, None]:
+    """
+    Do something
+
+    Yields:
+        When X.
+    """
+    yield
+
+
+# OK
+def foo() -> typing.Generator[None, None, None]:
+    """
+    Do something
+
+    Yields:
+        When X.
+    """
+    yield None
+
+
+# OK
+def foo():
+    """
+    Do something
+
+    Yields:
+        When X.
+    """
+    yield
+
+
+# OK
+def foo():
+    """
+    Do something
+
+    Yields:
+        When X.
+    """
+    yield None

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

@@ -60,3 +60,54 @@ class Bar:
             A number
         """
         print('test')
+
+
+import typing
+
+
+# OK
+def foo() -> typing.Generator[None, None, None]:
+    """
+    Do something
+
+    Yields
+    ------
+        When X.
+    """
+    yield None
+
+
+# OK
+def foo() -> typing.Generator[None, None, None]:
+    """
+    Do something
+
+    Yields
+    ------
+        When X.
+    """
+    yield
+
+
+# OK
+def foo():
+    """
+    Do something
+
+    Yields
+    ------
+        When X.
+    """
+    yield None
+
+
+# OK
+def foo():
+    """
+    Do something
+
+    Yields
+    ------
+        When X.
+    """
+    yield

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

@@ -3,6 +3,7 @@ use ruff_diagnostics::Diagnostic;
 use ruff_diagnostics::Violation;
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::map_callable;
+use ruff_python_ast::helpers::map_subscript;
 use ruff_python_ast::name::QualifiedName;
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_ast::{self as ast, visitor, Expr, Stmt};
@@ -126,7 +127,8 @@ impl Violation for DocstringExtraneousReturns {
 /// Docstrings missing yields sections are a sign of incomplete documentation
 /// or refactors.
 ///
-/// This rule is not enforced for abstract methods and stubs functions.
+/// This rule is not enforced for abstract methods, stubs functions, or
+/// functions that only yield `None`.
 ///
 /// ## Example
 /// ```python
@@ -499,6 +501,7 @@ fn parse_entries_numpy(content: &str) -> Vec<QualifiedName> {
 #[derive(Debug)]
 struct YieldEntry {
     range: TextRange,
+    is_none_yield: bool,
 }
 
 impl Ranged for YieldEntry {
@@ -507,11 +510,32 @@ impl Ranged for YieldEntry {
     }
 }
 
+#[allow(clippy::enum_variant_names)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum ReturnEntryKind {
+    NoneNone,
+    ImplicitNone,
+    ExplicitNone,
+}
+
 /// An individual `return` statement in a function body.
 #[derive(Debug)]
 struct ReturnEntry {
     range: TextRange,
-    is_none_return: bool,
+    kind: ReturnEntryKind,
+}
+
+impl ReturnEntry {
+    const fn is_none_return(&self) -> bool {
+        matches!(
+            &self.kind,
+            ReturnEntryKind::ExplicitNone | ReturnEntryKind::ImplicitNone
+        )
+    }
+
+    const fn is_implicit(&self) -> bool {
+        matches!(&self.kind, ReturnEntryKind::ImplicitNone)
+    }
 }
 
 impl Ranged for ReturnEntry {
@@ -641,7 +665,17 @@ impl<'a> Visitor<'a> for BodyVisitor<'a> {
             }) => {
                 self.returns.push(ReturnEntry {
                     range: *range,
-                    is_none_return: value.is_none_literal_expr(),
+                    kind: if value.is_none_literal_expr() {
+                        ReturnEntryKind::ExplicitNone
+                    } else {
+                        ReturnEntryKind::NoneNone
+                    },
+                });
+            }
+            Stmt::Return(ast::StmtReturn { range, value: None }) => {
+                self.returns.push(ReturnEntry {
+                    range: *range,
+                    kind: ReturnEntryKind::ImplicitNone,
                 });
             }
             Stmt::FunctionDef(_) | Stmt::ClassDef(_) => return,
@@ -655,12 +689,24 @@ impl<'a> Visitor<'a> for BodyVisitor<'a> {
         match expr {
             Expr::Yield(ast::ExprYield {
                 range,
-                value: Some(_),
+                value: Some(value),
             }) => {
-                self.yields.push(YieldEntry { range: *range });
+                self.yields.push(YieldEntry {
+                    range: *range,
+                    is_none_yield: value.is_none_literal_expr(),
+                });
+            }
+            Expr::Yield(ast::ExprYield { range, value: None }) => {
+                self.yields.push(YieldEntry {
+                    range: *range,
+                    is_none_yield: true,
+                });
             }
             Expr::YieldFrom(ast::ExprYieldFrom { range, .. }) => {
-                self.yields.push(YieldEntry { range: *range });
+                self.yields.push(YieldEntry {
+                    range: *range,
+                    is_none_yield: false,
+                });
             }
             Expr::Lambda(_) => return,
             _ => {}
@@ -711,6 +757,76 @@ fn yields_documented(
         || (matches!(convention, Some(Convention::Google)) && starts_with_yields(docstring))
 }
 
+#[derive(Debug, Copy, Clone)]
+enum GeneratorOrIteratorArguments<'a> {
+    Unparameterized,
+    Single(&'a Expr),
+    Several(&'a [Expr]),
+}
+
+impl<'a> GeneratorOrIteratorArguments<'a> {
+    fn first(self) -> Option<&'a Expr> {
+        match self {
+            Self::Unparameterized => None,
+            Self::Single(element) => Some(element),
+            Self::Several(elements) => elements.first(),
+        }
+    }
+
+    fn indicates_none_returned(self) -> bool {
+        match self {
+            Self::Unparameterized => true,
+            Self::Single(_) => true,
+            Self::Several(elements) => elements.get(2).map_or(true, Expr::is_none_literal_expr),
+        }
+    }
+}
+
+/// Returns the arguments to a generator annotation, if it exists.
+fn generator_annotation_arguments<'a>(
+    expr: &'a Expr,
+    semantic: &'a SemanticModel,
+) -> Option<GeneratorOrIteratorArguments<'a>> {
+    let qualified_name = semantic.resolve_qualified_name(map_subscript(expr))?;
+    match qualified_name.segments() {
+        ["typing" | "typing_extensions", "Iterable" | "AsyncIterable" | "Iterator" | "AsyncIterator"]
+        | ["collections", "abc", "Iterable" | "AsyncIterable" | "Iterator" | "AsyncIterator"] => {
+            match expr {
+                Expr::Subscript(ast::ExprSubscript { slice, .. }) => {
+                    Some(GeneratorOrIteratorArguments::Single(slice))
+                }
+                _ => Some(GeneratorOrIteratorArguments::Unparameterized),
+            }
+        }
+        ["typing" | "typing_extensions", "Generator" | "AsyncGenerator"]
+        | ["collections", "abc", "Generator" | "AsyncGenerator"] => match expr {
+            Expr::Subscript(ast::ExprSubscript { slice, .. }) => {
+                if let Expr::Tuple(tuple) = &**slice {
+                    Some(GeneratorOrIteratorArguments::Several(tuple.elts.as_slice()))
+                } else {
+                    // `Generator[int]` implies `Generator[int, None, None]`
+                    // as it uses a PEP-696 TypeVar with default values
+                    Some(GeneratorOrIteratorArguments::Single(slice))
+                }
+            }
+            _ => Some(GeneratorOrIteratorArguments::Unparameterized),
+        },
+        _ => None,
+    }
+}
+
+fn is_generator_function_annotated_as_returning_none(
+    entries: &BodyEntries,
+    return_annotations: &Expr,
+    semantic: &SemanticModel,
+) -> bool {
+    if entries.yields.is_empty() {
+        return false;
+    }
+    generator_annotation_arguments(return_annotations, semantic)
+        .is_some_and(GeneratorOrIteratorArguments::indicates_none_returned)
+}
+
 /// DOC201, DOC202, DOC402, DOC403, DOC501, DOC502
 pub(crate) fn check_docstring(
     checker: &mut Checker,
@@ -726,8 +842,10 @@ pub(crate) fn check_docstring(
         return;
     };
 
+    let semantic = checker.semantic();
+
     // Ignore stubs.
-    if function_type::is_stub(function_def, checker.semantic()) {
+    if function_type::is_stub(function_def, semantic) {
         return;
     }
 
@@ -743,7 +861,7 @@ pub(crate) fn check_docstring(
     };
 
     let body_entries = {
-        let mut visitor = BodyVisitor::new(checker.semantic());
+        let mut visitor = BodyVisitor::new(semantic);
         visitor.visit_body(&function_def.body);
         visitor.finish()
     };
@@ -752,16 +870,30 @@ pub(crate) fn check_docstring(
     if checker.enabled(Rule::DocstringMissingReturns) {
         if !returns_documented(docstring, &docstring_sections, convention) {
             let extra_property_decorators = checker.settings.pydocstyle.property_decorators();
-            if !definition.is_property(extra_property_decorators, checker.semantic()) {
+            if !definition.is_property(extra_property_decorators, semantic) {
                 if let Some(body_return) = body_entries.returns.first() {
                     match function_def.returns.as_deref() {
-                        Some(returns) if !Expr::is_none_literal_expr(returns) => diagnostics.push(
+                        Some(returns) => {
-                            Diagnostic::new(DocstringMissingReturns, body_return.range()),
+                            // Ignore it if it's annotated as returning `None`
-                        ),
+                            // or it's a generator function annotated as returning `None`,
+                            // i.e. any of `-> None`, `-> Iterator[...]` or `-> Generator[..., ..., None]`
+                            if !returns.is_none_literal_expr()
+                                && !is_generator_function_annotated_as_returning_none(
+                                    &body_entries,
+                                    returns,
+                                    semantic,
+                                )
+                            {
+                                diagnostics.push(Diagnostic::new(
+                                    DocstringMissingReturns,
+                                    body_return.range(),
+                                ));
+                            }
+                        }
                         None if body_entries
                             .returns
                             .iter()
-                            .any(|entry| !entry.is_none_return) =>
+                            .any(|entry| !entry.is_none_return()) =>
                         {
                             diagnostics.push(Diagnostic::new(
                                 DocstringMissingReturns,
@@ -779,8 +911,21 @@ pub(crate) fn check_docstring(
     if checker.enabled(Rule::DocstringMissingYields) {
         if !yields_documented(docstring, &docstring_sections, convention) {
             if let Some(body_yield) = body_entries.yields.first() {
-                let diagnostic = Diagnostic::new(DocstringMissingYields, body_yield.range());
+                match function_def.returns.as_deref() {
-                diagnostics.push(diagnostic);
+                    Some(returns)
+                        if !generator_annotation_arguments(returns, semantic).is_some_and(
+                            |arguments| arguments.first().map_or(true, Expr::is_none_literal_expr),
+                        ) =>
+                    {
+                        diagnostics
+                            .push(Diagnostic::new(DocstringMissingYields, body_yield.range()));
+                    }
+                    None if body_entries.yields.iter().any(|entry| !entry.is_none_yield) => {
+                        diagnostics
+                            .push(Diagnostic::new(DocstringMissingYields, body_yield.range()));
+                    }
+                    _ => {}
+                }
             }
         }
     }
@@ -817,11 +962,13 @@ pub(crate) fn check_docstring(
 
     // Avoid applying "extraneous" rules to abstract methods. An abstract method's docstring _could_
     // document that it raises an exception without including the exception in the implementation.
-    if !visibility::is_abstract(&function_def.decorator_list, checker.semantic()) {
+    if !visibility::is_abstract(&function_def.decorator_list, semantic) {
         // DOC202
         if checker.enabled(Rule::DocstringExtraneousReturns) {
             if let Some(ref docstring_returns) = docstring_sections.returns {
-                if body_entries.returns.is_empty() {
+                if body_entries.returns.is_empty()
+                    || body_entries.returns.iter().all(ReturnEntry::is_implicit)
+                {
                     let diagnostic =
                         Diagnostic::new(DocstringExtraneousReturns, docstring_returns.range());
                     diagnostics.push(diagnostic);

crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-extraneous-returns_DOC202_google.py.snap

@@ -24,3 +24,16 @@ DOC202_google.py:36:1: DOC202 Docstring should not have a returns section becaus
 39 |           print('test')
    |
    = help: Remove the "Returns" section
+
+DOC202_google.py:82:1: DOC202 Docstring should not have a returns section because the function doesn't return anything
+   |
+80 |       """Fooey things.
+81 |   
+82 | /     Returns:
+83 | |         None
+84 | |     """
+   | |____^ DOC202
+85 |       if not condition:
+86 |           return
+   |
+   = help: Remove the "Returns" section

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

@@ -58,3 +58,39 @@ DOC201_numpy.py:189:5: DOC201 `return` is not documented in docstring
     |     ^^^^^^^^^^^ DOC201
     |
     = help: Add a "Returns" section to the docstring
+
+DOC201_numpy.py:195:5: DOC201 `return` is not documented in docstring
+    |
+193 | def bar() -> int | None:
+194 |     """Bar-y method"""
+195 |     return
+    |     ^^^^^^ DOC201
+    |
+    = help: Add a "Returns" section to the docstring
+
+DOC201_numpy.py:222:5: DOC201 `return` is not documented in docstring
+    |
+220 |     """Generate some strings"""
+221 |     yield from "abc"
+222 |     return
+    |     ^^^^^^ DOC201
+    |
+    = help: Add a "Returns" section to the docstring
+
+DOC201_numpy.py:230:5: DOC201 `return` is not documented in docstring
+    |
+228 |     """Generate some strings"""
+229 |     yield from "abc"
+230 |     return 42
+    |     ^^^^^^^^^ DOC201
+    |
+    = help: Add a "Returns" section to the docstring
+
+DOC201_numpy.py:236:5: DOC201 `return` is not documented in docstring
+    |
+234 | def not_a_generator() -> Iterator[int]:
+235 |     """"No returns documented here, oh no"""
+236 |     return (x for x in range(42))
+    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ DOC201
+    |
+    = help: Add a "Returns" section to the docstring

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

@@ -38,3 +38,21 @@ DOC402_google.py:67:5: DOC402 `yield` is not documented in docstring
    |     ^^^^^^^^^^^^^^^^^^^^ DOC402
    |
    = help: Add a "Yields" section to the docstring
+
+DOC402_google.py:100:5: DOC402 `yield` is not documented in docstring
+    |
+ 98 |     Do something
+ 99 |     """
+100 |     yield
+    |     ^^^^^ DOC402
+    |
+    = help: Add a "Yields" section to the docstring
+
+DOC402_google.py:108:5: DOC402 `yield` is not documented in docstring
+    |
+106 |     Do something
+107 |     """
+108 |     yield
+    |     ^^^^^ DOC402
+    |
+    = help: Add a "Yields" section to the docstring

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

@@ -18,3 +18,51 @@ DOC402_numpy.py:62:9: DOC402 `yield` is not documented in docstring
    |         ^^^^^^^^^^^^ DOC402
    |
    = help: Add a "Yields" section to the docstring
+
+DOC402_numpy.py:89:5: DOC402 `yield` is not documented in docstring
+   |
+87 |     Do something
+88 |     """
+89 |     yield None
+   |     ^^^^^^^^^^ DOC402
+90 |     yield 1
+   |
+   = help: Add a "Yields" section to the docstring
+
+DOC402_numpy.py:98:5: DOC402 `yield` is not documented in docstring
+   |
+96 |     Do something
+97 |     """
+98 |     yield None
+   |     ^^^^^^^^^^ DOC402
+   |
+   = help: Add a "Yields" section to the docstring
+
+DOC402_numpy.py:122:5: DOC402 `yield` is not documented in docstring
+    |
+120 |     Do something
+121 |     """
+122 |     yield None
+    |     ^^^^^^^^^^ DOC402
+123 |     yield 1
+    |
+    = help: Add a "Yields" section to the docstring
+
+DOC402_numpy.py:131:5: DOC402 `yield` is not documented in docstring
+    |
+129 |     Do something
+130 |     """
+131 |     yield 1
+    |     ^^^^^^^ DOC402
+132 |     yield
+    |
+    = help: Add a "Yields" section to the docstring
+
+DOC402_numpy.py:140:5: DOC402 `yield` is not documented in docstring
+    |
+138 |     Do something
+139 |     """
+140 |     yield
+    |     ^^^^^ DOC402
+    |
+    = help: Add a "Yields" section to the docstring