From 61653b9f27887285983278077d892bc2b6f67aed Mon Sep 17 00:00:00 2001 From: tomecki Date: Fri, 17 Mar 2023 23:30:32 +0100 Subject: [PATCH] [`pylint`] Implement `useless-return` (`R1711`) (#3116) --- .../test/fixtures/pylint/useless_return.py | 50 ++++++++ crates/ruff/src/checkers/ast/mod.rs | 4 + crates/ruff/src/codes.rs | 7 +- crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + crates/ruff/src/rules/pylint/rules/mod.rs | 2 + .../src/rules/pylint/rules/useless_return.rs | 121 ++++++++++++++++++ ...int__tests__PLR1711_useless_return.py.snap | 105 +++++++++++++++ ruff.schema.json | 2 + 9 files changed, 290 insertions(+), 3 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/pylint/useless_return.py create mode 100644 crates/ruff/src/rules/pylint/rules/useless_return.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1711_useless_return.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/useless_return.py b/crates/ruff/resources/test/fixtures/pylint/useless_return.py new file mode 100644 index 0000000000..75da8d8beb --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/useless_return.py @@ -0,0 +1,50 @@ +import sys + + +def print_python_version(): + print(sys.version) + return None # [useless-return] + + +def print_python_version(): + print(sys.version) + return None # [useless-return] + + +def print_python_version(): + print(sys.version) + return None # [useless-return] + + +class SomeClass: + def print_python_version(self): + print(sys.version) + return None # [useless-return] + + +def print_python_version(): + if 2 * 2 == 4: + return + print(sys.version) + + +def print_python_version(): + if 2 * 2 == 4: + return None + return + + +def print_python_version(): + if 2 * 2 == 4: + return None + + +def print_python_version(): + """This function returns None.""" + return None + + +def print_python_version(): + """This function returns None.""" + print(sys.version) + return None # [useless-return] diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 0b24ebc6ea..d984b21289 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -436,6 +436,10 @@ where flake8_return::rules::function(self, body); } + if self.settings.rules.enabled(Rule::UselessReturn) { + pylint::rules::useless_return(self, stmt, body); + } + if self.settings.rules.enabled(Rule::ComplexStructure) { if let Some(diagnostic) = mccabe::rules::function_is_too_complex( stmt, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 0f271e60f6..41e618eddf 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -171,19 +171,18 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "E0118") => Rule::UsedPriorGlobalDeclaration, (Pylint, "E0604") => Rule::InvalidAllObject, (Pylint, "E0605") => Rule::InvalidAllFormat, - (Pylint, "W1508") => Rule::InvalidEnvvarDefault, (Pylint, "E1142") => Rule::AwaitOutsideAsync, (Pylint, "E1205") => Rule::LoggingTooManyArgs, (Pylint, "E1206") => Rule::LoggingTooFewArgs, (Pylint, "E1307") => Rule::BadStringFormatType, + (Pylint, "E1310") => Rule::BadStrStripCall, + (Pylint, "E1507") => Rule::InvalidEnvvarValue, (Pylint, "E2502") => Rule::BidirectionalUnicode, (Pylint, "E2510") => Rule::InvalidCharacterBackspace, (Pylint, "E2512") => Rule::InvalidCharacterSub, (Pylint, "E2513") => Rule::InvalidCharacterEsc, (Pylint, "E2514") => Rule::InvalidCharacterNul, (Pylint, "E2515") => Rule::InvalidCharacterZeroWidthSpace, - (Pylint, "E1310") => Rule::BadStrStripCall, - (Pylint, "E1507") => Rule::InvalidEnvvarValue, (Pylint, "R0133") => Rule::ComparisonOfConstant, (Pylint, "R0206") => Rule::PropertyWithParameters, (Pylint, "R0402") => Rule::ConsiderUsingFromImport, @@ -192,12 +191,14 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "R0913") => Rule::TooManyArguments, (Pylint, "R0915") => Rule::TooManyStatements, (Pylint, "R1701") => Rule::ConsiderMergingIsinstance, + (Pylint, "R1711") => Rule::UselessReturn, (Pylint, "R1722") => Rule::ConsiderUsingSysExit, (Pylint, "R2004") => Rule::MagicValueComparison, (Pylint, "R5501") => Rule::CollapsibleElseIf, (Pylint, "W0120") => Rule::UselessElseOnLoop, (Pylint, "W0602") => Rule::GlobalVariableNotAssigned, (Pylint, "W0603") => Rule::GlobalStatement, + (Pylint, "W1508") => Rule::InvalidEnvvarDefault, (Pylint, "W2901") => Rule::RedefinedLoopName, // flake8-builtins diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 51c5ff8a8e..fdd1188959 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -143,6 +143,7 @@ ruff_macros::register_rules!( rules::pyflakes::rules::UnusedAnnotation, rules::pyflakes::rules::RaiseNotImplemented, // pylint + rules::pylint::rules::UselessReturn, rules::pylint::rules::YieldInInit, rules::pylint::rules::InvalidAllObject, rules::pylint::rules::InvalidAllFormat, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index 0d8949fdb0..0cf76dd4e2 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -61,6 +61,7 @@ mod tests { #[test_case(Rule::UsedPriorGlobalDeclaration, Path::new("used_prior_global_declaration.py"); "PLE0118")] #[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"); "PLW0120")] #[test_case(Rule::UselessImportAlias, Path::new("import_aliasing.py"); "PLC0414")] + #[test_case(Rule::UselessReturn, Path::new("useless_return.py"); "PLR1711")] #[test_case(Rule::YieldInInit, Path::new("yield_in_init.py"); "PLE0100")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 656c527aab..1bd908b5e8 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -37,6 +37,7 @@ pub use used_prior_global_declaration::{ }; pub use useless_else_on_loop::{useless_else_on_loop, UselessElseOnLoop}; pub use useless_import_alias::{useless_import_alias, UselessImportAlias}; +pub use useless_return::{useless_return, UselessReturn}; pub use yield_in_init::{yield_in_init, YieldInInit}; mod await_outside_async; @@ -71,4 +72,5 @@ mod use_from_import; mod used_prior_global_declaration; mod useless_else_on_loop; mod useless_import_alias; +mod useless_return; mod yield_in_init; diff --git a/crates/ruff/src/rules/pylint/rules/useless_return.rs b/crates/ruff/src/rules/pylint/rules/useless_return.rs new file mode 100644 index 0000000000..3be3b36795 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/useless_return.rs @@ -0,0 +1,121 @@ +use log::error; +use rustpython_parser::ast::{Constant, ExprKind, Stmt, StmtKind}; + +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::{is_const_none, ReturnStatementVisitor}; +use ruff_python_ast::types::{Range, RefEquality}; +use ruff_python_ast::visitor::Visitor; + +use crate::autofix::helpers::delete_stmt; +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for functions that end with an unnecessary `return` or +/// `return None`, and contain no other `return` statements. +/// +/// ## Why is this bad? +/// Python implicitly assumes a `None` return at the end of a function, making +/// it unnecessary to explicitly write `return None`. +/// +/// ## Example +/// ```python +/// def f(): +/// print(5) +/// return None +/// ``` +/// +/// Use instead: +/// ```python +/// def f(): +/// print(5) +/// ``` +#[violation] +pub struct UselessReturn; + +impl AlwaysAutofixableViolation for UselessReturn { + #[derive_message_formats] + fn message(&self) -> String { + format!("Useless `return` statement at end of function") + } + + fn autofix_title(&self) -> String { + format!("Remove useless `return` statement") + } +} + +/// PLR1711 +pub fn useless_return<'a>(checker: &mut Checker<'a>, stmt: &'a Stmt, body: &'a [Stmt]) { + // Skip empty functions. + if body.is_empty() { + return; + } + + // Find the last statement in the function. + let last_stmt = body.last().unwrap(); + if !matches!(last_stmt.node, StmtKind::Return { .. }) { + return; + } + + // Skip functions that consist of a single return statement. + if body.len() == 1 { + return; + } + + // Skip functions that consist of a docstring and a return statement. + if body.len() == 2 { + if let StmtKind::Expr { value } = &body[0].node { + if matches!( + value.node, + ExprKind::Constant { + value: Constant::Str(_), + .. + } + ) { + return; + } + } + } + + // Verify that the last statement is a return statement. + let StmtKind::Return { value} = &last_stmt.node else { + return; + }; + + // Verify that the return statement is either bare or returns `None`. + if !value.as_ref().map_or(true, |expr| is_const_none(expr)) { + return; + }; + + // Finally: verify that there are no _other_ return statements in the function. + let mut visitor = ReturnStatementVisitor::default(); + visitor.visit_body(body); + if visitor.returns.len() > 1 { + return; + } + + let mut diagnostic = Diagnostic::new(UselessReturn, Range::from(last_stmt)); + if checker.patch(diagnostic.kind.rule()) { + let deleted: Vec<&Stmt> = checker.deletions.iter().map(Into::into).collect(); + match delete_stmt( + last_stmt, + Some(stmt), + &deleted, + checker.locator, + checker.indexer, + checker.stylist, + ) { + Ok(fix) => { + if fix.content.is_empty() || fix.content == "pass" { + checker.deletions.insert(RefEquality(last_stmt)); + } + diagnostic.amend(fix); + } + Err(e) => { + error!("Failed to delete `return` statement: {}", e); + } + }; + } + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1711_useless_return.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1711_useless_return.py.snap new file mode 100644 index 0000000000..46d84a2169 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1711_useless_return.py.snap @@ -0,0 +1,105 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + name: UselessReturn + body: "Useless `return` statement at end of function" + suggestion: "Remove useless `return` statement" + fixable: true + location: + row: 6 + column: 4 + end_location: + row: 6 + column: 15 + fix: + content: "" + location: + row: 6 + column: 0 + end_location: + row: 7 + column: 0 + parent: ~ +- kind: + name: UselessReturn + body: "Useless `return` statement at end of function" + suggestion: "Remove useless `return` statement" + fixable: true + location: + row: 11 + column: 4 + end_location: + row: 11 + column: 15 + fix: + content: "" + location: + row: 11 + column: 0 + end_location: + row: 12 + column: 0 + parent: ~ +- kind: + name: UselessReturn + body: "Useless `return` statement at end of function" + suggestion: "Remove useless `return` statement" + fixable: true + location: + row: 16 + column: 4 + end_location: + row: 16 + column: 15 + fix: + content: "" + location: + row: 16 + column: 0 + end_location: + row: 17 + column: 0 + parent: ~ +- kind: + name: UselessReturn + body: "Useless `return` statement at end of function" + suggestion: "Remove useless `return` statement" + fixable: true + location: + row: 22 + column: 8 + end_location: + row: 22 + column: 19 + fix: + content: "" + location: + row: 22 + column: 0 + end_location: + row: 23 + column: 0 + parent: ~ +- kind: + name: UselessReturn + body: "Useless `return` statement at end of function" + suggestion: "Remove useless `return` statement" + fixable: true + location: + row: 50 + column: 4 + end_location: + row: 50 + column: 15 + fix: + content: "" + location: + row: 50 + column: 0 + end_location: + row: 51 + column: 0 + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index 71296adabc..9c76d61ca0 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1887,6 +1887,8 @@ "PLR17", "PLR170", "PLR1701", + "PLR171", + "PLR1711", "PLR172", "PLR1722", "PLR2",