From a1ff1c883737018717596743999a4efd364ca12c Mon Sep 17 00:00:00 2001 From: JohannesGraner Date: Sun, 26 Nov 2023 19:12:42 +0100 Subject: [PATCH 1/7] [`pylint`] Add missing return doc rule (`W9011`) This PR is part of #970, it adds the `W9011` error rule. --- .../fixtures/pylint/missing_return_doc.py | 14 +++++ .../src/checkers/ast/analyze/definitions.rs | 6 +- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/missing_return_doc.rs | 56 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ..._tests__PLW9011_missing_return_doc.py.snap | 16 ++++++ ruff.schema.json | 4 ++ 8 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py new file mode 100644 index 0000000000000..2cf93224a605f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py @@ -0,0 +1,14 @@ +def sum_good(a: int, b: int) -> int: + """Returns sum of two integers. + :param a: first integer + :param b: second integer + :return: sum of parameters a and b + """ + return a + b + +def sum_bad(a: int, b: int): # [missing-return-doc] + """Returns sum of two integers. + :param a: first integer + :param b: second integer + """ + return a + b diff --git a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs index f4c40279096b2..552ba718646ed 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs @@ -7,7 +7,7 @@ use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::docstrings::Docstring; use crate::fs::relativize_path; -use crate::rules::{flake8_annotations, flake8_pyi, pydocstyle}; +use crate::rules::{flake8_annotations, flake8_pyi, pydocstyle, pylint}; use crate::{docstrings, warn_user}; /// Run lint rules over all [`Definition`] nodes in the [`SemanticModel`]. @@ -78,6 +78,7 @@ pub(crate) fn definitions(checker: &mut Checker) { Rule::UndocumentedPublicModule, Rule::UndocumentedPublicNestedClass, Rule::UndocumentedPublicPackage, + Rule::MissingReturnDoc, ]); if !enforce_annotations && !enforce_docstrings && !enforce_stubs && !enforce_stubs_and_runtime { @@ -195,6 +196,9 @@ pub(crate) fn definitions(checker: &mut Checker) { if !pydocstyle::rules::not_empty(checker, &docstring) { continue; } + if checker.enabled(Rule::MissingReturnDoc) { + pylint::rules::missing_return_doc(checker, &docstring); + } if checker.enabled(Rule::FitsOnOneLine) { pydocstyle::rules::one_liner(checker, &docstring); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d47bb85ae3fed..33a0369a685b4 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -285,6 +285,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { #[allow(deprecated)] (Pylint, "W3201") => (RuleGroup::Nursery, rules::pylint::rules::BadDunderMethodName), (Pylint, "W3301") => (RuleGroup::Stable, rules::pylint::rules::NestedMinMax), + (Pylint, "W9011") => (RuleGroup::Stable, rules::pylint::rules::MissingReturnDoc), // flake8-async (Flake8Async, "100") => (RuleGroup::Stable, rules::flake8_async::rules::BlockingHttpCallInAsyncFunction), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 589bdbc0544b4..204a3d3940d07 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -19,6 +19,7 @@ mod tests { use crate::test::test_path; #[test_case(Rule::AndOrTernary, Path::new("and_or_ternary.py"))] + #[test_case(Rule::MissingReturnDoc, Path::new("missing_return_doc.py"))] #[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))] #[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))] #[test_case(Rule::BadOpenMode, Path::new("bad_open_mode.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs new file mode 100644 index 0000000000000..37819a5dc0417 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs @@ -0,0 +1,56 @@ +use std::fmt::Debug; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_text_size::Ranged; + +use crate::{checkers::ast::Checker, docstrings::Docstring}; + +/// ## What it does +/// Checks that ReST docstring contains documentation on what is returned. +/// +/// ## Why is this bad? +/// Docstrings are a good way to document the code, +/// and including information on the return value from a function helps to +/// understand what the function does. +/// +/// ## Example +/// ```python +/// def integer_sum(a: int, b: int): # [missing-return-doc] +/// """Returns sum of two integers +/// :param a: first integer +/// :param b: second integer +/// """ +/// return a + b +/// ``` +/// +/// Use instead: +/// ```python +/// def integer_sum(a: int, b: int) -> int: +/// """Returns sum of two integers +/// :param a: first integer +/// :param b: second integer +/// :return: sum of parameters a and b +/// """ +/// return a + b +/// ``` +#[violation] +pub struct MissingReturnDoc; + +impl Violation for MissingReturnDoc { + #[derive_message_formats] + fn message(&self) -> String { + format!("Docstring missing documentation on what is returned") + } +} + +/// PLW9011 +pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { + if docstring.contents.contains(":return:") { + return; + } + + checker + .diagnostics + .push(Diagnostic::new(MissingReturnDoc, docstring.range())); +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 978a4bd15e73d..de6cafa96a801 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -33,6 +33,7 @@ pub(crate) use logging::*; pub(crate) use magic_value_comparison::*; pub(crate) use manual_import_from::*; pub(crate) use misplaced_bare_raise::*; +pub(crate) use missing_return_doc::*; pub(crate) use named_expr_without_context::*; pub(crate) use nested_min_max::*; pub(crate) use no_self_use::*; @@ -105,6 +106,7 @@ mod logging; mod magic_value_comparison; mod manual_import_from; mod misplaced_bare_raise; +mod missing_return_doc; mod named_expr_without_context; mod nested_min_max; mod no_self_use; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap new file mode 100644 index 0000000000000..5e32f756fec8c --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap @@ -0,0 +1,16 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +missing_return_doc.py:10:5: PLW9011 Docstring missing documentation on what is returned + | + 9 | def sum_bad(a: int, b: int): # [missing-return-doc] +10 | """Returns sum of two integers. + | _____^ +11 | | :param a: first integer +12 | | :param b: second integer +13 | | """ + | |_______^ PLW9011 +14 | return a + b + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 34f6ebcd959ab..c72b0157cc4e3 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3144,6 +3144,10 @@ "PLW33", "PLW330", "PLW3301", + "PLW9", + "PLW90", + "PLW901", + "PLW9011", "PT", "PT0", "PT00", From 77ec6eb993bf99e2d569c2199d1e4d8acf599f1e Mon Sep 17 00:00:00 2001 From: JohannesGraner Date: Sun, 26 Nov 2023 20:36:54 +0100 Subject: [PATCH 2/7] Check for return statements --- .../fixtures/pylint/missing_return_doc.py | 22 ++++++++++++++-- .../rules/pylint/rules/missing_return_doc.rs | 25 ++++++++++++++----- ..._tests__PLW9011_missing_return_doc.py.snap | 14 +++++------ 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py index 2cf93224a605f..939f88f3ab2ff 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py @@ -1,4 +1,4 @@ -def sum_good(a: int, b: int) -> int: +def return_good(a: int, b: int) -> int: """Returns sum of two integers. :param a: first integer :param b: second integer @@ -6,7 +6,25 @@ def sum_good(a: int, b: int) -> int: """ return a + b -def sum_bad(a: int, b: int): # [missing-return-doc] + +def no_return_good(a: int, b: int) -> None: + """Prints the sum of two integers. + :param a: first integer + :param b: second integer + """ + print(a + b) + + +def no_return_also_good(a: int, b: int) -> None: + """Prints the sum of two integers. + :param a: first integer + :param b: second integer + """ + print(a + b) + return None + + +def return_bad(a: int, b: int): # [missing-return-doc] """Returns sum of two integers. :param a: first integer :param b: second integer diff --git a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs index 37819a5dc0417..f8f6c06fb6502 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs @@ -46,11 +46,24 @@ impl Violation for MissingReturnDoc { /// PLW9011 pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { - if docstring.contents.contains(":return:") { - return; + let is_method_with_return = docstring + .definition + .as_function_def() + .map_or(false, |function| { + function + .body + .iter() + .filter_map(|statement| statement.as_return_stmt()) + .any(|return_statement| { + return_statement + .value + .as_deref() + .is_some_and(|value| !value.is_none_literal_expr()) + }) + }); + if is_method_with_return && !docstring.contents.contains(":return:") { + checker + .diagnostics + .push(Diagnostic::new(MissingReturnDoc, docstring.range())); } - - checker - .diagnostics - .push(Diagnostic::new(MissingReturnDoc, docstring.range())); } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap index 5e32f756fec8c..8782c2b6c8230 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap @@ -1,16 +1,16 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -missing_return_doc.py:10:5: PLW9011 Docstring missing documentation on what is returned +missing_return_doc.py:28:5: PLW9011 Docstring missing documentation on what is returned | - 9 | def sum_bad(a: int, b: int): # [missing-return-doc] -10 | """Returns sum of two integers. +27 | def return_bad(a: int, b: int): # [missing-return-doc] +28 | """Returns sum of two integers. | _____^ -11 | | :param a: first integer -12 | | :param b: second integer -13 | | """ +29 | | :param a: first integer +30 | | :param b: second integer +31 | | """ | |_______^ PLW9011 -14 | return a + b +32 | return a + b | From 663ba4a855e231ac0a6e8f9c1d890cdf4b0f4744 Mon Sep 17 00:00:00 2001 From: Johannes Graner Date: Mon, 27 Nov 2023 18:57:18 +0100 Subject: [PATCH 3/7] Do not add new rule as stable. Co-authored-by: Zanie Blue --- crates/ruff_linter/src/codes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 33a0369a685b4..1a0c1596d4d51 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -285,7 +285,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { #[allow(deprecated)] (Pylint, "W3201") => (RuleGroup::Nursery, rules::pylint::rules::BadDunderMethodName), (Pylint, "W3301") => (RuleGroup::Stable, rules::pylint::rules::NestedMinMax), - (Pylint, "W9011") => (RuleGroup::Stable, rules::pylint::rules::MissingReturnDoc), + (Pylint, "W9011") => (RuleGroup::Preview, rules::pylint::rules::MissingReturnDoc), // flake8-async (Flake8Async, "100") => (RuleGroup::Stable, rules::flake8_async::rules::BlockingHttpCallInAsyncFunction), From 234cfcfad06bfb418412e7ce587d3d2d057a1389 Mon Sep 17 00:00:00 2001 From: JohannesGraner Date: Mon, 27 Nov 2023 19:55:12 +0100 Subject: [PATCH 4/7] ignore private functions --- .../fixtures/pylint/missing_return_doc.py | 8 +++++ .../rules/pylint/rules/missing_return_doc.rs | 34 ++++++++++--------- ..._tests__PLW9011_missing_return_doc.py.snap | 14 ++++---- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py index 939f88f3ab2ff..35b19b0e0cc17 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py @@ -24,6 +24,14 @@ def no_return_also_good(a: int, b: int) -> None: return None +def _private_good(a: int, b: int) -> None: + """Returns sum of two integers. + :param a: first integer + :param b: second integer + """ + return a + b + + def return_bad(a: int, b: int): # [missing-return-doc] """Returns sum of two integers. :param a: first integer diff --git a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs index f8f6c06fb6502..e2926346d5d94 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs @@ -46,22 +46,24 @@ impl Violation for MissingReturnDoc { /// PLW9011 pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { - let is_method_with_return = docstring - .definition - .as_function_def() - .map_or(false, |function| { - function - .body - .iter() - .filter_map(|statement| statement.as_return_stmt()) - .any(|return_statement| { - return_statement - .value - .as_deref() - .is_some_and(|value| !value.is_none_literal_expr()) - }) - }); - if is_method_with_return && !docstring.contents.contains(":return:") { + let is_public_method_with_return = + docstring + .definition + .as_function_def() + .map_or(false, |function| { + !function.name.starts_with('_') + && function + .body + .iter() + .filter_map(|statement| statement.as_return_stmt()) + .any(|return_statement| { + return_statement + .value + .as_deref() + .is_some_and(|value| !value.is_none_literal_expr()) + }) + }); + if is_public_method_with_return && !docstring.contents.contains(":return:") { checker .diagnostics .push(Diagnostic::new(MissingReturnDoc, docstring.range())); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap index 8782c2b6c8230..8646779e55614 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap @@ -1,16 +1,16 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -missing_return_doc.py:28:5: PLW9011 Docstring missing documentation on what is returned +missing_return_doc.py:36:5: PLW9011 Docstring missing documentation on what is returned | -27 | def return_bad(a: int, b: int): # [missing-return-doc] -28 | """Returns sum of two integers. +35 | def return_bad(a: int, b: int): # [missing-return-doc] +36 | """Returns sum of two integers. | _____^ -29 | | :param a: first integer -30 | | :param b: second integer -31 | | """ +37 | | :param a: first integer +38 | | :param b: second integer +39 | | """ | |_______^ PLW9011 -32 | return a + b +40 | return a + b | From a6bb2df656a3a8fd89a57d2764e81223a94bc72b Mon Sep 17 00:00:00 2001 From: JohannesGraner Date: Mon, 27 Nov 2023 20:22:02 +0100 Subject: [PATCH 5/7] allow google and numpy style docstrings --- .../fixtures/pylint/missing_return_doc.py | 30 +++++++++++++++++++ .../rules/pylint/rules/missing_return_doc.rs | 10 ++++++- ..._tests__PLW9011_missing_return_doc.py.snap | 14 ++++----- 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py index 35b19b0e0cc17..f4a225cc6277c 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/missing_return_doc.py @@ -24,6 +24,36 @@ def no_return_also_good(a: int, b: int) -> None: return None +def google_good(a: int, b: int): + """Returns sum of two integers. + + Args: + a: first integer + b: second integer + + Returns: + sum + """ + return a + b + + +def numpy_good(a: int, b: int): + """Returns sum of two integers. + + Parameters + ---------- + a: + first integer + b: + second integer + + Returns + ------- + sum + """ + return a + b + + def _private_good(a: int, b: int) -> None: """Returns sum of two integers. :param a: first integer diff --git a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs index e2926346d5d94..8ac7f0a44b092 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs @@ -1,3 +1,4 @@ +use regex::Regex; use std::fmt::Debug; use ruff_diagnostics::{Diagnostic, Violation}; @@ -63,7 +64,14 @@ pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { .is_some_and(|value| !value.is_none_literal_expr()) }) }); - if is_public_method_with_return && !docstring.contents.contains(":return:") { + let rest_style = ":return:"; + let numpy_style = r"Returns\n\s*-------\n"; + let google_style = r"Returns:\n"; + let has_return_documentation = [rest_style, numpy_style, google_style] + .map(|pattern| Regex::new(pattern).unwrap()) + .iter() + .any(|return_regex| return_regex.is_match(docstring.contents)); + if is_public_method_with_return && !has_return_documentation { checker .diagnostics .push(Diagnostic::new(MissingReturnDoc, docstring.range())); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap index 8646779e55614..f366188dd574c 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap @@ -1,16 +1,16 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -missing_return_doc.py:36:5: PLW9011 Docstring missing documentation on what is returned +missing_return_doc.py:66:5: PLW9011 Docstring missing documentation on what is returned | -35 | def return_bad(a: int, b: int): # [missing-return-doc] -36 | """Returns sum of two integers. +65 | def return_bad(a: int, b: int): # [missing-return-doc] +66 | """Returns sum of two integers. | _____^ -37 | | :param a: first integer -38 | | :param b: second integer -39 | | """ +67 | | :param a: first integer +68 | | :param b: second integer +69 | | """ | |_______^ PLW9011 -40 | return a + b +70 | return a + b | From b87531641f6ce1a5bd1eeee7d55379ec61377606 Mon Sep 17 00:00:00 2001 From: JohannesGraner Date: Thu, 30 Nov 2023 21:15:48 +0100 Subject: [PATCH 6/7] Attempt to optimize regex compilation --- .../rules/pylint/rules/missing_return_doc.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs index 8ac7f0a44b092..4be6973f17cdc 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs @@ -1,3 +1,4 @@ +use once_cell::sync::Lazy; use regex::Regex; use std::fmt::Debug; @@ -47,6 +48,14 @@ impl Violation for MissingReturnDoc { /// PLW9011 pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { + static REST: Lazy = Lazy::new(|| Regex::new(":return:").unwrap()); + static NUMPY: Lazy = Lazy::new(|| Regex::new(r"Returns\n\s*-------\n").unwrap()); + static GOOGLE: Lazy = Lazy::new(|| Regex::new(r"Returns:\n").unwrap()); + + let has_return_documentation = [&REST, &NUMPY, &GOOGLE] + .iter() + .any(|return_regex| return_regex.is_match(docstring.contents)); + let is_public_method_with_return = docstring .definition @@ -64,13 +73,7 @@ pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { .is_some_and(|value| !value.is_none_literal_expr()) }) }); - let rest_style = ":return:"; - let numpy_style = r"Returns\n\s*-------\n"; - let google_style = r"Returns:\n"; - let has_return_documentation = [rest_style, numpy_style, google_style] - .map(|pattern| Regex::new(pattern).unwrap()) - .iter() - .any(|return_regex| return_regex.is_match(docstring.contents)); + if is_public_method_with_return && !has_return_documentation { checker .diagnostics From 537aeaa88fb54ee4ea654fab9244f635aca0fba2 Mon Sep 17 00:00:00 2001 From: JohannesGraner Date: Wed, 6 Dec 2023 21:20:10 +0100 Subject: [PATCH 7/7] implement docstring convention --- crates/ruff_linter/src/rules/pylint/mod.rs | 36 +++++++++++ .../rules/pylint/rules/missing_return_doc.rs | 62 +++++++++++-------- .../ruff_linter/src/rules/pylint/settings.rs | 3 + ..._tests__PLW9011_missing_return_doc.py.snap | 12 ---- ...int__tests__missing_return_doc_google.snap | 50 +++++++++++++++ ...lint__tests__missing_return_doc_numpy.snap | 46 ++++++++++++++ ...int__tests__missing_return_doc_pep257.snap | 54 ++++++++++++++++ crates/ruff_workspace/src/options.rs | 12 ++++ ruff.schema.json | 11 ++++ 9 files changed, 248 insertions(+), 38 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_google.snap create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_numpy.snap create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_pep257.snap diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 9a636de23bda8..7b935fbfdaa6e 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -13,7 +13,9 @@ mod tests { use test_case::test_case; use crate::assert_messages; + use crate::message::Message; use crate::registry::Rule; + use crate::rules::pydocstyle::settings::Convention; use crate::rules::pylint; use crate::settings::types::PythonVersion; use crate::settings::LinterSettings; @@ -304,6 +306,40 @@ mod tests { Ok(()) } + fn missing_return_doc_convention(convention: Convention) -> Result> { + test_path( + Path::new("pylint/missing_return_doc.py"), + &LinterSettings { + pylint: pylint::settings::Settings { + convention: Some(convention), + ..pylint::settings::Settings::default() + }, + ..LinterSettings::for_rules(vec![Rule::MissingReturnDoc]) + }, + ) + } + + #[test] + fn missing_return_doc_pep257() -> Result<()> { + let diagnostics = missing_return_doc_convention(Convention::Pep257)?; + assert_messages!(diagnostics); + Ok(()) + } + + #[test] + fn missing_return_doc_google() -> Result<()> { + let diagnostics = missing_return_doc_convention(Convention::Google)?; + assert_messages!(diagnostics); + Ok(()) + } + + #[test] + fn missing_return_doc_numpy() -> Result<()> { + let diagnostics = missing_return_doc_convention(Convention::Numpy)?; + assert_messages!(diagnostics); + Ok(()) + } + #[test] fn too_many_public_methods() -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs index 4be6973f17cdc..40702540804f6 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/missing_return_doc.rs @@ -6,10 +6,12 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_text_size::Ranged; -use crate::{checkers::ast::Checker, docstrings::Docstring}; +use crate::{ + checkers::ast::Checker, docstrings::Docstring, rules::pydocstyle::settings::Convention, +}; /// ## What it does -/// Checks that ReST docstring contains documentation on what is returned. +/// Checks that docstring contains documentation on what is returned. /// /// ## Why is this bad? /// Docstrings are a good way to document the code, @@ -36,6 +38,9 @@ use crate::{checkers::ast::Checker, docstrings::Docstring}; /// """ /// return a + b /// ``` +/// +/// ## Options +/// - `pylint.convention` #[violation] pub struct MissingReturnDoc; @@ -52,31 +57,36 @@ pub(crate) fn missing_return_doc(checker: &mut Checker, docstring: &Docstring) { static NUMPY: Lazy = Lazy::new(|| Regex::new(r"Returns\n\s*-------\n").unwrap()); static GOOGLE: Lazy = Lazy::new(|| Regex::new(r"Returns:\n").unwrap()); - let has_return_documentation = [&REST, &NUMPY, &GOOGLE] - .iter() - .any(|return_regex| return_regex.is_match(docstring.contents)); + if let Some(convention) = checker.settings.pylint.convention { + let has_return_documentation = match convention { + Convention::Google => &GOOGLE, + Convention::Numpy => &NUMPY, + Convention::Pep257 => &REST, + } + .is_match(docstring.contents); - let is_public_method_with_return = - docstring - .definition - .as_function_def() - .map_or(false, |function| { - !function.name.starts_with('_') - && function - .body - .iter() - .filter_map(|statement| statement.as_return_stmt()) - .any(|return_statement| { - return_statement - .value - .as_deref() - .is_some_and(|value| !value.is_none_literal_expr()) - }) - }); + let is_public_method_with_return = + docstring + .definition + .as_function_def() + .map_or(false, |function| { + !function.name.starts_with('_') + && function + .body + .iter() + .filter_map(|statement| statement.as_return_stmt()) + .any(|return_statement| { + return_statement + .value + .as_deref() + .is_some_and(|value| !value.is_none_literal_expr()) + }) + }); - if is_public_method_with_return && !has_return_documentation { - checker - .diagnostics - .push(Diagnostic::new(MissingReturnDoc, docstring.range())); + if is_public_method_with_return && !has_return_documentation { + checker + .diagnostics + .push(Diagnostic::new(MissingReturnDoc, docstring.range())); + } } } diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index cb9846b11e056..11d98f9126a6b 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -3,6 +3,7 @@ use rustc_hash::FxHashSet; use serde::{Deserialize, Serialize}; +use crate::rules::pydocstyle::settings::Convention; use ruff_macros::CacheKey; use ruff_python_ast::{ExprNumberLiteral, LiteralExpressionRef, Number}; @@ -38,6 +39,7 @@ impl ConstantType { pub struct Settings { pub allow_magic_value_types: Vec, pub allow_dunder_method_names: FxHashSet, + pub convention: Option, pub max_args: usize, pub max_returns: usize, pub max_bool_expr: usize, @@ -51,6 +53,7 @@ impl Default for Settings { Self { allow_magic_value_types: vec![ConstantType::Str, ConstantType::Bytes], allow_dunder_method_names: FxHashSet::default(), + convention: None, max_args: 5, max_returns: 6, max_bool_expr: 5, diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap index f366188dd574c..6c123427ab2b8 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW9011_missing_return_doc.py.snap @@ -1,16 +1,4 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -missing_return_doc.py:66:5: PLW9011 Docstring missing documentation on what is returned - | -65 | def return_bad(a: int, b: int): # [missing-return-doc] -66 | """Returns sum of two integers. - | _____^ -67 | | :param a: first integer -68 | | :param b: second integer -69 | | """ - | |_______^ PLW9011 -70 | return a + b - | - diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_google.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_google.snap new file mode 100644 index 0000000000000..bd04a39161984 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_google.snap @@ -0,0 +1,50 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +missing_return_doc.py:2:5: PLW9011 Docstring missing documentation on what is returned + | +1 | def return_good(a: int, b: int) -> int: +2 | """Returns sum of two integers. + | _____^ +3 | | :param a: first integer +4 | | :param b: second integer +5 | | :return: sum of parameters a and b +6 | | """ + | |_______^ PLW9011 +7 | return a + b + | + +missing_return_doc.py:41:5: PLW9011 Docstring missing documentation on what is returned + | +40 | def numpy_good(a: int, b: int): +41 | """Returns sum of two integers. + | _____^ +42 | | +43 | | Parameters +44 | | ---------- +45 | | a: +46 | | first integer +47 | | b: +48 | | second integer +49 | | +50 | | Returns +51 | | ------- +52 | | sum +53 | | """ + | |_______^ PLW9011 +54 | return a + b + | + +missing_return_doc.py:66:5: PLW9011 Docstring missing documentation on what is returned + | +65 | def return_bad(a: int, b: int): # [missing-return-doc] +66 | """Returns sum of two integers. + | _____^ +67 | | :param a: first integer +68 | | :param b: second integer +69 | | """ + | |_______^ PLW9011 +70 | return a + b + | + + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_numpy.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_numpy.snap new file mode 100644 index 0000000000000..321217788fa83 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_numpy.snap @@ -0,0 +1,46 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +missing_return_doc.py:2:5: PLW9011 Docstring missing documentation on what is returned + | +1 | def return_good(a: int, b: int) -> int: +2 | """Returns sum of two integers. + | _____^ +3 | | :param a: first integer +4 | | :param b: second integer +5 | | :return: sum of parameters a and b +6 | | """ + | |_______^ PLW9011 +7 | return a + b + | + +missing_return_doc.py:28:5: PLW9011 Docstring missing documentation on what is returned + | +27 | def google_good(a: int, b: int): +28 | """Returns sum of two integers. + | _____^ +29 | | +30 | | Args: +31 | | a: first integer +32 | | b: second integer +33 | | +34 | | Returns: +35 | | sum +36 | | """ + | |_______^ PLW9011 +37 | return a + b + | + +missing_return_doc.py:66:5: PLW9011 Docstring missing documentation on what is returned + | +65 | def return_bad(a: int, b: int): # [missing-return-doc] +66 | """Returns sum of two integers. + | _____^ +67 | | :param a: first integer +68 | | :param b: second integer +69 | | """ + | |_______^ PLW9011 +70 | return a + b + | + + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_pep257.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_pep257.snap new file mode 100644 index 0000000000000..fb4d99b68d4fb --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__missing_return_doc_pep257.snap @@ -0,0 +1,54 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +missing_return_doc.py:28:5: PLW9011 Docstring missing documentation on what is returned + | +27 | def google_good(a: int, b: int): +28 | """Returns sum of two integers. + | _____^ +29 | | +30 | | Args: +31 | | a: first integer +32 | | b: second integer +33 | | +34 | | Returns: +35 | | sum +36 | | """ + | |_______^ PLW9011 +37 | return a + b + | + +missing_return_doc.py:41:5: PLW9011 Docstring missing documentation on what is returned + | +40 | def numpy_good(a: int, b: int): +41 | """Returns sum of two integers. + | _____^ +42 | | +43 | | Parameters +44 | | ---------- +45 | | a: +46 | | first integer +47 | | b: +48 | | second integer +49 | | +50 | | Returns +51 | | ------- +52 | | sum +53 | | """ + | |_______^ PLW9011 +54 | return a + b + | + +missing_return_doc.py:66:5: PLW9011 Docstring missing documentation on what is returned + | +65 | def return_bad(a: int, b: int): # [missing-return-doc] +66 | """Returns sum of two integers. + | _____^ +67 | | :param a: first integer +68 | | :param b: second integer +69 | | """ + | |_______^ PLW9011 +70 | return a + b + | + + diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 05f4522710cd9..340d53e422470 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2620,6 +2620,17 @@ pub struct PylintOptions { )] pub allow_dunder_method_names: Option>, + /// Whether to use Google-style, NumPy-style, or PEP 257 (reST) docstring conventions. + #[option( + default = r#"null"#, + value_type = r#""google" | "numpy" | "pep257""#, + example = r#" + # Use Google-style docstrings. + convention = "google" + "# + )] + pub convention: Option, + /// Maximum number of branches allowed for a function or method body (see: /// `PLR0912`). #[option(default = r"12", value_type = "int", example = r"max-branches = 12")] @@ -2662,6 +2673,7 @@ impl PylintOptions { .allow_magic_value_types .unwrap_or(defaults.allow_magic_value_types), allow_dunder_method_names: self.allow_dunder_method_names.unwrap_or_default(), + convention: self.convention, max_args: self.max_args.unwrap_or(defaults.max_args), max_bool_expr: self.max_bool_expr.unwrap_or(defaults.max_bool_expr), max_returns: self.max_returns.unwrap_or(defaults.max_returns), diff --git a/ruff.schema.json b/ruff.schema.json index 690e0b4de78bd..52eb85c4390cc 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2342,6 +2342,17 @@ "$ref": "#/definitions/ConstantType" } }, + "convention": { + "description": "Whether to use Google-style, NumPy-style, or PEP 257 (reST) docstring conventions.", + "anyOf": [ + { + "$ref": "#/definitions/Convention" + }, + { + "type": "null" + } + ] + }, "max-args": { "description": "Maximum number of arguments allowed for a function or method definition (see: `PLR0913`).", "type": [