Skip to content

Commit

Permalink
Don't enforce returns and yields in abstract methods (#12771)
Browse files Browse the repository at this point in the history
## Summary

Closes #12685.
  • Loading branch information
charliermarsh committed Aug 9, 2024
1 parent 2abfab0 commit 1f51048
Show file tree
Hide file tree
Showing 7 changed files with 134 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,14 @@ def f(num: int):
num (int): A number
"""
return 1


import abc


class A(metaclass=abc.abcmeta):
# DOC201
@abc.abstractmethod
def f(self):
"""Lorem ipsum."""
return True
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,14 @@ def baz(self) -> str:
A number
"""
return 'test'


import abc


class A(metaclass=abc.abcmeta):
# DOC201
@abc.abstractmethod
def f(self):
"""Lorem ipsum."""
return True
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,17 @@ def foo(self) -> int:
x
"""
raise NotImplementedError


import abc


class A(metaclass=abc.abcmeta):
@abc.abstractmethod
def f(self):
"""Lorem ipsum
Returns:
dict: The values
"""
return
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,19 @@ def bar(self) -> str:
A number
"""
print('test')


import abc


class A(metaclass=abc.abcmeta):
@abc.abstractmethod
def f(self):
"""Lorem ipsum
Returns
-------
dict:
The values
"""
return
112 changes: 64 additions & 48 deletions crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_python_ast::helpers::map_callable;
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, visitor, Expr, Stmt};
use ruff_python_semantic::analyze::function_type;
use ruff_python_semantic::analyze::{function_type, visibility};
use ruff_python_semantic::{Definition, SemanticModel};
use ruff_text_size::{Ranged, TextRange};

Expand All @@ -25,6 +25,8 @@ use crate::rules::pydocstyle::settings::Convention;
/// Docstrings missing return sections are a sign of incomplete documentation
/// or refactors.
///
/// This rule is not enforced for abstract methods and stubs functions.
///
/// ## Example
/// ```python
/// def calculate_speed(distance: float, time: float) -> float:
Expand Down Expand Up @@ -73,6 +75,8 @@ impl Violation for DocstringMissingReturns {
/// Functions without an explicit return should not have a returns section
/// in their docstrings.
///
/// This rule is not enforced for stub functions.
///
/// ## Example
/// ```python
/// def say_hello(n: int) -> None:
Expand Down Expand Up @@ -121,6 +125,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.
///
/// ## Example
/// ```python
/// def count_to_n(n: int) -> int:
Expand Down Expand Up @@ -169,6 +175,8 @@ impl Violation for DocstringMissingYields {
/// Functions which don't yield anything should not have a yields section
/// in their docstrings.
///
/// This rule is not enforced for stub functions.
///
/// ## Example
/// ```python
/// def say_hello(n: int) -> None:
Expand Down Expand Up @@ -218,6 +226,8 @@ impl Violation for DocstringExtraneousYields {
/// it can be misleading to users and/or a sign of incomplete documentation or
/// refactors.
///
/// This rule is not enforced for abstract methods and stubs functions.
///
/// ## Example
/// ```python
/// def calculate_speed(distance: float, time: float) -> float:
Expand Down Expand Up @@ -282,6 +292,8 @@ impl Violation for DocstringMissingException {
/// Some conventions prefer non-explicit exceptions be omitted from the
/// docstring.
///
/// This rule is not enforced for stub functions.
///
/// ## Example
/// ```python
/// def calculate_speed(distance: float, time: float) -> float:
Expand Down Expand Up @@ -343,7 +355,7 @@ impl Violation for DocstringExtraneousException {
}
}

// A generic docstring section.
/// A generic docstring section.
#[derive(Debug)]
struct GenericSection {
range: TextRange,
Expand All @@ -363,7 +375,7 @@ impl GenericSection {
}
}

// A Raises docstring section.
/// A "Raises" section in a docstring.
#[derive(Debug)]
struct RaisesSection<'a> {
raised_exceptions: Vec<QualifiedName<'a>>,
Expand All @@ -378,7 +390,7 @@ impl Ranged for RaisesSection<'_> {

impl<'a> RaisesSection<'a> {
/// Return the raised exceptions for the docstring, or `None` if the docstring does not contain
/// a `Raises` section.
/// a "Raises" section.
fn from_section(section: &SectionContext<'a>, style: Option<SectionStyle>) -> Self {
Self {
raised_exceptions: parse_entries(section.following_lines_str(), style),
Expand Down Expand Up @@ -415,7 +427,7 @@ impl<'a> DocstringSections<'a> {
}
}

/// Parse the entries in a `Raises` section of a docstring.
/// Parse the entries in a "Raises" section of a docstring.
///
/// Attempts to parse using the specified [`SectionStyle`], falling back to the other style if no
/// entries are found.
Expand Down Expand Up @@ -519,7 +531,7 @@ struct BodyEntries<'a> {
struct BodyVisitor<'a> {
returns: Vec<Entry>,
yields: Vec<Entry>,
currently_suspended_exceptions: Option<&'a ast::Expr>,
currently_suspended_exceptions: Option<&'a Expr>,
raised_exceptions: Vec<ExceptionEntry<'a>>,
semantic: &'a SemanticModel<'a>,
}
Expand Down Expand Up @@ -732,17 +744,6 @@ pub(crate) fn check_docstring(
}
}

// DOC202
if checker.enabled(Rule::DocstringExtraneousReturns) {
if let Some(ref docstring_returns) = docstring_sections.returns {
if body_entries.returns.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousReturns, docstring_returns.range());
diagnostics.push(diagnostic);
}
}
}

// DOC402
if checker.enabled(Rule::DocstringMissingYields) {
if !yields_documented(docstring, &docstring_sections, convention) {
Expand All @@ -753,17 +754,6 @@ pub(crate) fn check_docstring(
}
}

// DOC403
if checker.enabled(Rule::DocstringExtraneousYields) {
if let Some(docstring_yields) = docstring_sections.yields {
if body_entries.yields.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousYields, docstring_yields.range());
diagnostics.push(diagnostic);
}
}
}

// DOC501
if checker.enabled(Rule::DocstringMissingException) {
for body_raise in &body_entries.raised_exceptions {
Expand Down Expand Up @@ -794,28 +784,54 @@ pub(crate) fn check_docstring(
}
}

// DOC502
if checker.enabled(Rule::DocstringExtraneousException) {
if let Some(docstring_raises) = docstring_sections.raises {
let mut extraneous_exceptions = Vec::new();
for docstring_raise in &docstring_raises.raised_exceptions {
if !body_entries.raised_exceptions.iter().any(|exception| {
exception
.qualified_name
.segments()
.ends_with(docstring_raise.segments())
}) {
extraneous_exceptions.push(docstring_raise.to_string());
// 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()) {
// DOC202
if checker.enabled(Rule::DocstringExtraneousReturns) {
if let Some(ref docstring_returns) = docstring_sections.returns {
if body_entries.returns.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousReturns, docstring_returns.range());
diagnostics.push(diagnostic);
}
}
if !extraneous_exceptions.is_empty() {
let diagnostic = Diagnostic::new(
DocstringExtraneousException {
ids: extraneous_exceptions,
},
docstring_raises.range(),
);
diagnostics.push(diagnostic);
}

// DOC403
if checker.enabled(Rule::DocstringExtraneousYields) {
if let Some(docstring_yields) = docstring_sections.yields {
if body_entries.yields.is_empty() {
let diagnostic =
Diagnostic::new(DocstringExtraneousYields, docstring_yields.range());
diagnostics.push(diagnostic);
}
}
}

// DOC502
if checker.enabled(Rule::DocstringExtraneousException) {
if let Some(docstring_raises) = docstring_sections.raises {
let mut extraneous_exceptions = Vec::new();
for docstring_raise in &docstring_raises.raised_exceptions {
if !body_entries.raised_exceptions.iter().any(|exception| {
exception
.qualified_name
.segments()
.ends_with(docstring_raise.segments())
}) {
extraneous_exceptions.push(docstring_raise.to_string());
}
}
if !extraneous_exceptions.is_empty() {
let diagnostic = Diagnostic::new(
DocstringExtraneousException {
ids: extraneous_exceptions,
},
docstring_raises.range(),
);
diagnostics.push(diagnostic);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,12 @@ DOC201_google.py:71:9: DOC201 `return` is not documented in docstring
73 | print("I never return")
|
= help: Add a "Returns" section to the docstring

DOC201_google.py:121:9: DOC201 `return` is not documented in docstring
|
119 | def f(self):
120 | """Lorem ipsum."""
121 | return True
| ^^^^^^^^^^^ DOC201
|
= help: Add a "Returns" section to the docstring
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,12 @@ DOC201_numpy.py:62:9: DOC201 `return` is not documented in docstring
| ^^^^^^^^^^^^^ DOC201
|
= help: Add a "Returns" section to the docstring

DOC201_numpy.py:87:9: DOC201 `return` is not documented in docstring
|
85 | def f(self):
86 | """Lorem ipsum."""
87 | return True
| ^^^^^^^^^^^ DOC201
|
= help: Add a "Returns" section to the docstring

0 comments on commit 1f51048

Please sign in to comment.