From bf620dcb38b140bba05cde70b9cd3d4c51e997c3 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Sun, 1 Sep 2024 02:03:39 +0100 Subject: [PATCH] [`pydoclint`] Permit yielding `None` in DOC402 and DOC403 (#13148) Co-authored-by: Alex Waygood --- .../test/fixtures/pydoclint/DOC201_numpy.py | 47 +++++ .../test/fixtures/pydoclint/DOC202_google.py | 12 ++ .../test/fixtures/pydoclint/DOC402_google.py | 19 ++ .../test/fixtures/pydoclint/DOC402_numpy.py | 78 ++++++++ .../test/fixtures/pydoclint/DOC403_google.py | 47 +++++ .../test/fixtures/pydoclint/DOC403_numpy.py | 51 +++++ .../rules/pydoclint/rules/check_docstring.rs | 181 ++++++++++++++++-- ...g-extraneous-returns_DOC202_google.py.snap | 13 ++ ...tring-missing-returns_DOC201_numpy.py.snap | 36 ++++ ...tring-missing-yields_DOC402_google.py.snap | 18 ++ ...string-missing-yields_DOC402_numpy.py.snap | 48 +++++ 11 files changed, 533 insertions(+), 17 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_numpy.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_numpy.py index 0d1081ad87..396fc45713 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC201_numpy.py +++ b/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)) diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC202_google.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC202_google.py index 97f73a6203..a539f67b9f 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC202_google.py +++ b/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) diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC402_google.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC402_google.py index 2cad41bc41..003d3b5fa5 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC402_google.py +++ b/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 diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC402_numpy.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC402_numpy.py index bde7a2afde..ecebe5fea2 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC402_numpy.py +++ b/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 diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC403_google.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC403_google.py index 70c9c53112..d98eac6555 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC403_google.py +++ b/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 diff --git a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC403_numpy.py b/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC403_numpy.py index 5d5c646a90..2f1a918e6c 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydoclint/DOC403_numpy.py +++ b/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 diff --git a/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs b/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs index bf7d68049a..d13c825186 100644 --- a/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs +++ b/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 { #[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> { + 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( - Diagnostic::new(DocstringMissingReturns, body_return.range()), - ), + Some(returns) => { + // 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()); - diagnostics.push(diagnostic); + match function_def.returns.as_deref() { + 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); diff --git a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-extraneous-returns_DOC202_google.py.snap b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-extraneous-returns_DOC202_google.py.snap index 16aa8e5fb2..ded71e7f05 100644 --- a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-extraneous-returns_DOC202_google.py.snap +++ b/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 diff --git a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-returns_DOC201_numpy.py.snap b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-returns_DOC201_numpy.py.snap index e72763333e..37429d74f9 100644 --- a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-returns_DOC201_numpy.py.snap +++ b/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 diff --git a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-yields_DOC402_google.py.snap b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-yields_DOC402_google.py.snap index c9ebc3f280..c3748cc435 100644 --- a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-yields_DOC402_google.py.snap +++ b/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 diff --git a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-yields_DOC402_numpy.py.snap b/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-yields_DOC402_numpy.py.snap index 4737fe1603..fdf4e18cef 100644 --- a/crates/ruff_linter/src/rules/pydoclint/snapshots/ruff_linter__rules__pydoclint__tests__docstring-missing-yields_DOC402_numpy.py.snap +++ b/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