[pylint] Implement useless-return (R1711) (#3116)

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]

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,

crates/ruff/src/codes.rs

@@ -171,19 +171,18 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (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<Rule> {
         (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

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,

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());

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;

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);
+}

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: ~
+

ruff.schema.json

@@ -1887,6 +1887,8 @@
         "PLR17",
         "PLR170",
         "PLR1701",
+        "PLR171",
+        "PLR1711",
         "PLR172",
         "PLR1722",
         "PLR2",