[flake8-return] Stabilize only add return None at the end when fixing implicit-return (RET503) (#18516)

This involved slightly more code changes than usual for a stabilization
- so maybe worth double-checking the logic!

I did verify by hand that the new stable behavior on the test fixture
matches the old preview behavior, even after the internal refactor.

Author: dylwil3

crates/ruff_linter/src/preview.rs

@@ -40,11 +40,6 @@ pub(crate) const fn is_bad_version_info_in_non_stub_enabled(settings: &LinterSet
     settings.preview.is_enabled()
 }
 
-// https://github.com/astral-sh/ruff/pull/11074
-pub(crate) const fn is_only_add_return_none_at_end_enabled(settings: &LinterSettings) -> bool {
-    settings.preview.is_enabled()
-}
-
 // https://github.com/astral-sh/ruff/pull/16719
 pub(crate) const fn is_fix_manual_dict_comprehension_enabled(settings: &LinterSettings) -> bool {
     settings.preview.is_enabled()

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

@@ -11,11 +11,10 @@ mod tests {
     use anyhow::Result;
     use test_case::test_case;
 
+    use crate::assert_messages;
     use crate::registry::Rule;
     use crate::settings::LinterSettings;
-    use crate::settings::types::PreviewMode;
     use crate::test::test_path;
-    use crate::{assert_messages, settings};
 
     #[test_case(Rule::UnnecessaryReturnNone, Path::new("RET501.py"))]
     #[test_case(Rule::ImplicitReturnValue, Path::new("RET502.py"))]
@@ -34,22 +33,4 @@ mod tests {
         assert_messages!(snapshot, diagnostics);
         Ok(())
     }
-
-    #[test_case(Rule::ImplicitReturn, Path::new("RET503.py"))]
-    fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
-        let snapshot = format!(
-            "preview__{}_{}",
-            rule_code.noqa_code(),
-            path.to_string_lossy()
-        );
-        let diagnostics = test_path(
-            Path::new("flake8_return").join(path).as_path(),
-            &settings::LinterSettings {
-                preview: PreviewMode::Enabled,
-                ..settings::LinterSettings::for_rule(rule_code)
-            },
-        )?;
-        assert_messages!(snapshot, diagnostics);
-        Ok(())
-    }
 }

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

@@ -16,7 +16,6 @@ use ruff_text_size::{Ranged, TextRange, TextSize};
 use crate::checkers::ast::Checker;
 use crate::fix::edits;
 use crate::fix::edits::adjust_indentation;
-use crate::preview::is_only_add_return_none_at_end_enabled;
 use crate::registry::Rule;
 use crate::rules::flake8_return::helpers::end_of_last_statement;
 use crate::{AlwaysFixableViolation, FixAvailability, Violation};
@@ -462,96 +461,70 @@ fn add_return_none(checker: &Checker, stmt: &Stmt, range: TextRange) {
     }
 }
 
-/// Returns a list of all implicit returns in the given statement.
-///
-/// Note: The function should be refactored to `has_implicit_return` with an early return (when seeing the first implicit return)
-/// when removing the preview gating.
-fn implicit_returns<'a>(checker: &Checker, stmt: &'a Stmt) -> Vec<&'a Stmt> {
+fn has_implicit_return(checker: &Checker, stmt: &Stmt) -> bool {
     match stmt {
         Stmt::If(ast::StmtIf {
             body,
             elif_else_clauses,
             ..
         }) => {
-            let mut implicit_stmts = body
+            if body
                 .last()
-                .map(|last| implicit_returns(checker, last))
-                .unwrap_or_default();
-
-            for clause in elif_else_clauses {
-                implicit_stmts.extend(
-                    clause
-                        .body
-                        .last()
-                        .iter()
-                        .flat_map(|last| implicit_returns(checker, last)),
-                );
+                .is_some_and(|last| has_implicit_return(checker, last))
+            {
+                return true;
+            }
+
+            if elif_else_clauses.iter().any(|clause| {
+                clause
+                    .body
+                    .last()
+                    .is_some_and(|last| has_implicit_return(checker, last))
+            }) {
+                return true;
             }
 
             // Check if we don't have an else clause
-            if matches!(
+            matches!(
                 elif_else_clauses.last(),
                 None | Some(ast::ElifElseClause { test: Some(_), .. })
-            ) {
-                implicit_stmts.push(stmt);
-            }
-            implicit_stmts
+            )
         }
-        Stmt::Assert(ast::StmtAssert { test, .. }) if is_const_false(test) => vec![],
-        Stmt::While(ast::StmtWhile { test, .. }) if is_const_true(test) => vec![],
+        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_returns(checker, last_stmt)
+                has_implicit_return(checker, last_stmt)
             } else {
-                vec![stmt]
-            }
-        }
-        Stmt::Match(ast::StmtMatch { cases, .. }) => {
-            let mut implicit_stmts = vec![];
-            for case in cases {
-                implicit_stmts.extend(
-                    case.body
-                        .last()
-                        .into_iter()
-                        .flat_map(|last_stmt| implicit_returns(checker, last_stmt)),
-                );
+                true
             }
-            implicit_stmts
         }
+        Stmt::Match(ast::StmtMatch { cases, .. }) => cases.iter().any(|case| {
+            case.body
+                .last()
+                .is_some_and(|last| has_implicit_return(checker, last))
+        }),
         Stmt::With(ast::StmtWith { body, .. }) => body
             .last()
-            .map(|last_stmt| implicit_returns(checker, last_stmt))
-            .unwrap_or_default(),
-        Stmt::Return(_) | Stmt::Raise(_) | Stmt::Try(_) => vec![],
+            .is_some_and(|last_stmt| has_implicit_return(checker, last_stmt)),
+        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())
             ) =>
         {
-            vec![]
-        }
-        _ => {
-            vec![stmt]
+            false
         }
+        _ => true,
     }
 }
 
 /// RET503
 fn implicit_return(checker: &Checker, function_def: &ast::StmtFunctionDef, stmt: &Stmt) {
-    let implicit_stmts = implicit_returns(checker, stmt);
-
-    if implicit_stmts.is_empty() {
-        return;
-    }
-
-    if is_only_add_return_none_at_end_enabled(checker.settings) {
+    if has_implicit_return(checker, stmt) {
         add_return_none(checker, stmt, function_def.range());
-    } else {
-        for implicit_stmt in implicit_stmts {
-            add_return_none(checker, implicit_stmt, implicit_stmt.range());
-        }
     }
 }