[flake8-return] Only add return None at end of function (RET503)

crates/ruff_linter/resources/test/fixtures/flake8_return/RET503.py

@@ -368,3 +368,11 @@ def bar() -> NoReturn:
     if baz() > 3:
         return 1
     bar()
+
+
+def f():
+    if a:
+        return b
+    else:
+        with c:
+            d

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -218,7 +218,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
                 Rule::SuperfluousElseContinue,
                 Rule::SuperfluousElseBreak,
             ]) {
-                flake8_return::rules::function(checker, body, returns.as_ref().map(AsRef::as_ref));
+                flake8_return::rules::function(
+                    checker,
+                    function_def,
+                    body,
+                    returns.as_ref().map(AsRef::as_ref),
+                );
             }
             if checker.enabled(Rule::UselessReturn) {
                 pylint::rules::useless_return(

crates/ruff_linter/src/rules/flake8_return/mod.rs

@@ -35,6 +35,7 @@ mod tests {
         Ok(())
     }
 
+    #[test_case(Rule::ImplicitReturn, Path::new("RET503.py"))]
     #[test_case(Rule::SuperfluousElseReturn, Path::new("RET505.py"))]
     #[test_case(Rule::SuperfluousElseRaise, Path::new("RET506.py"))]
     #[test_case(Rule::SuperfluousElseContinue, Path::new("RET507.py"))]

crates/ruff_linter/src/rules/flake8_return/rules/function.rs

@@ -440,20 +440,45 @@ fn is_noreturn_func(func: &Expr, semantic: &SemanticModel) -> bool {
     semantic.match_typing_qualified_name(&qualified_name, "NoReturn")
 }
 
-/// RET503
-fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
+fn add_return_none(checker: &mut Checker, stmt: &Stmt, range: TextRange) {
+    let mut diagnostic = Diagnostic::new(ImplicitReturn, range);
+    if let Some(indent) = indentation(checker.locator(), stmt) {
+        let mut content = String::new();
+        content.push_str(checker.stylist().line_ending().as_str());
+        content.push_str(indent);
+        content.push_str("return None");
+        diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
+            content,
+            end_of_last_statement(stmt, checker.locator()),
+        )));
+    }
+    checker.diagnostics.push(diagnostic);
+}
+
+fn has_implicit_return(checker: &mut Checker, stmt: &Stmt) -> bool {
     match stmt {
         Stmt::If(ast::StmtIf {
             body,
             elif_else_clauses,
             ..
         }) => {
+            let mut result = false;
             if let Some(last_stmt) = body.last() {
-                implicit_return(checker, last_stmt);
+                if has_implicit_return(checker, last_stmt) {
+                    result = true;
+                    if checker.settings.preview.is_enabled() {
+                        return result;
+                    }
+                }
             }
             for clause in elif_else_clauses {
                 if let Some(last_stmt) = clause.body.last() {
-                    implicit_return(checker, last_stmt);
+                    if has_implicit_return(checker, last_stmt) {
+                        result = true;
+                        if checker.settings.preview.is_enabled() {
+                            return result;
+                        }
+                    }
                 }
             }
 
@@ -462,76 +487,73 @@ fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
                 elif_else_clauses.last(),
                 None | Some(ast::ElifElseClause { test: Some(_), .. })
             ) {
-                let mut diagnostic = Diagnostic::new(ImplicitReturn, stmt.range());
-                if let Some(indent) = indentation(checker.locator(), stmt) {
-                    let mut content = String::new();
-                    content.push_str(checker.stylist().line_ending().as_str());
-                    content.push_str(indent);
-                    content.push_str("return None");
-                    diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
-                        content,
-                        end_of_last_statement(stmt, checker.locator()),
-                    )));
+                if checker.settings.preview.is_disabled() {
+                    add_return_none(checker, stmt, stmt.range());
                 }
-                checker.diagnostics.push(diagnostic);
+                return true;
             }
+            result
         }
-        Stmt::Assert(ast::StmtAssert { test, .. }) if is_const_false(test) => {}
-        Stmt::While(ast::StmtWhile { test, .. }) if is_const_true(test) => {}
+        Stmt::Assert(ast::StmtAssert { test, .. }) if is_const_false(test) => false,
+        Stmt::While(ast::StmtWhile { test, .. }) if is_const_true(test) => false,
         Stmt::For(ast::StmtFor { orelse, .. }) | Stmt::While(ast::StmtWhile { orelse, .. }) => {
             if let Some(last_stmt) = orelse.last() {
-                implicit_return(checker, last_stmt);
+                has_implicit_return(checker, last_stmt)
             } else {
-                let mut diagnostic = Diagnostic::new(ImplicitReturn, stmt.range());
-                if let Some(indent) = indentation(checker.locator(), stmt) {
-                    let mut content = String::new();
-                    content.push_str(checker.stylist().line_ending().as_str());
-                    content.push_str(indent);
-                    content.push_str("return None");
-                    diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
-                        content,
-                        end_of_last_statement(stmt, checker.locator()),
-                    )));
+                if checker.settings.preview.is_disabled() {
+                    add_return_none(checker, stmt, stmt.range());
                 }
-                checker.diagnostics.push(diagnostic);
+                true
             }
         }
         Stmt::Match(ast::StmtMatch { cases, .. }) => {
+            let mut result = false;
             for case in cases {
                 if let Some(last_stmt) = case.body.last() {
-                    implicit_return(checker, last_stmt);
+                    if has_implicit_return(checker, last_stmt) {
+                        result = true;
+                        if checker.settings.preview.is_enabled() {
+                            return result;
+                        }
+                    }
                 }
             }
+            result
         }
         Stmt::With(ast::StmtWith { body, .. }) => {
             if let Some(last_stmt) = body.last() {
-                implicit_return(checker, last_stmt);
+                if has_implicit_return(checker, last_stmt) {
+                    return true;
+                }
             }
+            false
         }
-        Stmt::Return(_) | Stmt::Raise(_) | Stmt::Try(_) => {}
+        Stmt::Return(_) | Stmt::Raise(_) | Stmt::Try(_) => false,
         Stmt::Expr(ast::StmtExpr { value, .. })
             if matches!(
                 value.as_ref(),
                 Expr::Call(ast::ExprCall { func, ..  })
                     if is_noreturn_func(func, checker.semantic())
-            ) => {}
+            ) =>
+        {
+            false
+        }
         _ => {
-            let mut diagnostic = Diagnostic::new(ImplicitReturn, stmt.range());
-            if let Some(indent) = indentation(checker.locator(), stmt) {
-                let mut content = String::new();
-                content.push_str(checker.stylist().line_ending().as_str());
-                content.push_str(indent);
-                content.push_str("return None");
-                diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
-                    content,
-                    end_of_last_statement(stmt, checker.locator()),
-                )));
+            if checker.settings.preview.is_disabled() {
+                add_return_none(checker, stmt, stmt.range());
             }
-            checker.diagnostics.push(diagnostic);
+            true
         }
     }
 }
 
+/// RET503
+fn implicit_return(checker: &mut Checker, function_def: &ast::StmtFunctionDef, stmt: &Stmt) {
+    if has_implicit_return(checker, stmt) && checker.settings.preview.is_enabled() {
+        add_return_none(checker, stmt, function_def.range());
+    }
+}
+
 /// RET504
 fn unnecessary_assign(checker: &mut Checker, stack: &Stack) {
     for (assign, return_, stmt) in &stack.assignment_return {
@@ -731,7 +753,12 @@ fn superfluous_elif_else(checker: &mut Checker, stack: &Stack) {
 }
 
 /// Run all checks from the `flake8-return` plugin.
-pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Expr>) {
+pub(crate) fn function(
+    checker: &mut Checker,
+    function_def: &ast::StmtFunctionDef,
+    body: &[Stmt],
+    returns: Option<&Expr>,
+) {
     // Find the last statement in the function.
     let Some(last_stmt) = body.last() else {
         // Skip empty functions.
@@ -777,7 +804,7 @@ pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Ex
             implicit_return_value(checker, &stack);
         }
         if checker.enabled(Rule::ImplicitReturn) {
-            implicit_return(checker, last_stmt);
+            implicit_return(checker, function_def, last_stmt);
         }
 
         if checker.enabled(Rule::UnnecessaryAssign) {

crates/ruff_linter/src/rules/flake8_return/snapshots/ruff_linter__rules__flake8_return__tests__RET503_RET503.py.snap

@@ -452,5 +452,21 @@ RET503.py:370:5: RET503 [*] Missing explicit `return` at the end of function abl
 369 369 |         return 1
 370 370 |     bar()
     371 |+    return None
+371 372 | 
+372 373 | 
+373 374 | def f():
 
+RET503.py:378:13: RET503 [*] Missing explicit `return` at the end of function able to return non-`None` value
+    |
+376 |     else:
+377 |         with c:
+378 |             d
+    |             ^ RET503
+    |
+    = help: Add explicit `return` statement
 
+ℹ Unsafe fix
+376 376 |     else:
+377 377 |         with c:
+378 378 |             d
+    379 |+            return None