Avoid trying to fix implicit returns with control flow (#2962)

2023-02-16 13:42:46 -05:00 · 2023-02-16 13:42:46 -05:00 · 059601d968
parent 2ec1701543
commit 059601d968
3 changed files with 209 additions and 57 deletions
--- a/crates/ruff/resources/test/fixtures/flake8_return/RET503.py
+++ b/crates/ruff/resources/test/fixtures/flake8_return/RET503.py
@ -251,3 +251,11 @@ def noreturn_pytest_xfail_2():
    if x > 0:
        return 1
    py_xfail("oof")
+
+
+def nested(values):
+    if not values:
+        return False
+
+    for value in values:
+        print(value)
--- a/crates/ruff/src/rules/flake8_return/rules.rs
+++ b/crates/ruff/src/rules/flake8_return/rules.rs
@ -1,10 +1,8 @@
 use itertools::Itertools;
-use ruff_macros::{define_violation, derive_message_formats};
 use rustpython_parser::ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind};

-use super::branch::Branch;
-use super::helpers::result_exists;
-use super::visitor::{ReturnVisitor, Stack};
+use ruff_macros::{define_violation, derive_message_formats};
+
 use crate::ast::helpers::elif_else_range;
 use crate::ast::types::Range;
 use crate::ast::visitor::Visitor;
@ -14,6 +12,10 @@ use crate::fix::Fix;
 use crate::registry::{Diagnostic, Rule};
 use crate::violation::{AlwaysAutofixableViolation, Violation};

+use super::branch::Branch;
+use super::helpers::result_exists;
+use super::visitor::{ReturnVisitor, Stack};
+
 define_violation!(
    pub struct UnnecessaryReturnNone;
 );
@ -196,29 +198,45 @@ fn is_noreturn_func(checker: &Checker, func: &Expr) -> bool {
 }

 /// RET503
-fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) {
-    match &last_stmt.node {
+fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
+    match &stmt.node {
        StmtKind::If { body, orelse, .. } => {
-            if body.is_empty() || orelse.is_empty() {
-                checker.diagnostics.push(Diagnostic::new(
-                    ImplicitReturn,
-                    Range::from_located(last_stmt),
-                ));
-                return;
-            }
-
            if let Some(last_stmt) = body.last() {
                implicit_return(checker, last_stmt);
            }
            if let Some(last_stmt) = orelse.last() {
                implicit_return(checker, last_stmt);
+            } else {
+                let mut diagnostic = Diagnostic::new(ImplicitReturn, Range::from_located(stmt));
+                if checker.patch(diagnostic.kind.rule()) {
+                    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.amend(Fix::insertion(content, stmt.end_location.unwrap()));
+                    }
+                }
+                checker.diagnostics.push(diagnostic);
            }
        }
-        StmtKind::For { body, orelse, .. } | StmtKind::AsyncFor { body, orelse, .. } => {
+        StmtKind::For { orelse, .. }
+        | StmtKind::AsyncFor { orelse, .. }
+        | StmtKind::While { orelse, .. } => {
            if let Some(last_stmt) = orelse.last() {
                implicit_return(checker, last_stmt);
-            } else if let Some(last_stmt) = body.last() {
-                implicit_return(checker, last_stmt);
+            } else {
+                let mut diagnostic = Diagnostic::new(ImplicitReturn, Range::from_located(stmt));
+                if checker.patch(diagnostic.kind.rule()) {
+                    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.amend(Fix::insertion(content, stmt.end_location.unwrap()));
+                    }
+                }
+                checker.diagnostics.push(diagnostic);
            }
        }
        StmtKind::With { body, .. } | StmtKind::AsyncWith { body, .. } => {
@ -234,10 +252,7 @@ fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) {
                    ..
                }
            ) => {}
-        StmtKind::Return { .. }
-        | StmtKind::While { .. }
-        | StmtKind::Raise { .. }
-        | StmtKind::Try { .. } => {}
+        StmtKind::Return { .. } | StmtKind::Raise { .. } | StmtKind::Try { .. } => {}
        StmtKind::Expr { value, .. }
            if matches!(
                &value.node,
@ -245,17 +260,14 @@ fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) {
                    if is_noreturn_func(checker, func)
            ) => {}
        _ => {
-            let mut diagnostic = Diagnostic::new(ImplicitReturn, Range::from_located(last_stmt));
+            let mut diagnostic = Diagnostic::new(ImplicitReturn, Range::from_located(stmt));
            if checker.patch(diagnostic.kind.rule()) {
-                if let Some(indent) = indentation(checker.locator, last_stmt) {
+                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");
-                    content.push_str(checker.stylist.line_ending().as_str());
-                    diagnostic.amend(Fix::insertion(
-                        content,
-                        Location::new(last_stmt.end_location.unwrap().row() + 1, 0),
-                    ));
+                    diagnostic.amend(Fix::insertion(content, stmt.end_location.unwrap()));
                }
            }
            checker.diagnostics.push(diagnostic);
--- a/crates/ruff/src/rules/flake8_return/snapshots/ruffrulesflake8_returntestsRET503_RET503.py.snap
+++ b/crates/ruff/src/rules/flake8_return/snapshots/ruffrulesflake8_returntestsRET503_RET503.py.snap
@ -10,7 +10,16 @@ expression: diagnostics
  end_location:
    row: 19
    column: 16
-  fix: ~
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 19
+      column: 16
+    end_location:
+      row: 19
+      column: 16
  parent: ~
 - kind:
    ImplicitReturn: ~
@ -22,14 +31,14 @@ expression: diagnostics
    column: 15
  fix:
    content:
-      - "        return None"
      - ""
+      - "        return None"
    location:
-      row: 26
-      column: 0
+      row: 25
+      column: 15
    end_location:
-      row: 26
-      column: 0
+      row: 25
+      column: 15
  parent: ~
 - kind:
    ImplicitReturn: ~
@ -41,24 +50,33 @@ expression: diagnostics
    column: 11
  fix:
    content:
-      - "    return None"
      - ""
+      - "    return None"
    location:
-      row: 35
-      column: 0
+      row: 34
+      column: 11
    end_location:
-      row: 35
-      column: 0
+      row: 34
+      column: 11
  parent: ~
 - kind:
    ImplicitReturn: ~
  location:
-    row: 40
-    column: 8
+    row: 39
+    column: 4
  end_location:
    row: 41
    column: 20
-  fix: ~
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 41
+      column: 20
+    end_location:
+      row: 41
+      column: 20
  parent: ~
 - kind:
    ImplicitReturn: ~
@ -70,14 +88,14 @@ expression: diagnostics
    column: 15
  fix:
    content:
-      - "        return None"
      - ""
+      - "        return None"
    location:
-      row: 51
-      column: 0
+      row: 50
+      column: 15
    end_location:
-      row: 51
-      column: 0
+      row: 50
+      column: 15
  parent: ~
 - kind:
    ImplicitReturn: ~
@ -89,14 +107,14 @@ expression: diagnostics
    column: 22
  fix:
    content:
-      - "    return None"
      - ""
+      - "    return None"
    location:
-      row: 58
-      column: 0
+      row: 57
+      column: 22
    end_location:
-      row: 58
-      column: 0
+      row: 57
+      column: 22
  parent: ~
 - kind:
    ImplicitReturn: ~
@ -108,13 +126,127 @@ expression: diagnostics
    column: 21
  fix:
    content:
-      - "    return None"
      - ""
+      - "    return None"
    location:
-      row: 65
-      column: 0
+      row: 64
+      column: 21
    end_location:
-      row: 65
-      column: 0
+      row: 64
+      column: 21
+  parent: ~
+- kind:
+    ImplicitReturn: ~
+  location:
+    row: 80
+    column: 4
+  end_location:
+    row: 83
+    column: 14
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 83
+      column: 14
+    end_location:
+      row: 83
+      column: 14
+  parent: ~
+- kind:
+    ImplicitReturn: ~
+  location:
+    row: 111
+    column: 4
+  end_location:
+    row: 114
+    column: 16
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 114
+      column: 16
+    end_location:
+      row: 114
+      column: 16
+  parent: ~
+- kind:
+    ImplicitReturn: ~
+  location:
+    row: 118
+    column: 4
+  end_location:
+    row: 124
+    column: 19
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 124
+      column: 19
+    end_location:
+      row: 124
+      column: 19
+  parent: ~
+- kind:
+    ImplicitReturn: ~
+  location:
+    row: 128
+    column: 4
+  end_location:
+    row: 131
+    column: 16
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 131
+      column: 16
+    end_location:
+      row: 131
+      column: 16
+  parent: ~
+- kind:
+    ImplicitReturn: ~
+  location:
+    row: 135
+    column: 4
+  end_location:
+    row: 141
+    column: 19
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 141
+      column: 19
+    end_location:
+      row: 141
+      column: 19
+  parent: ~
+- kind:
+    ImplicitReturn: ~
+  location:
+    row: 260
+    column: 4
+  end_location:
+    row: 261
+    column: 20
+  fix:
+    content:
+      - ""
+      - "    return None"
+    location:
+      row: 261
+      column: 20
+    end_location:
+      row: 261
+      column: 20
  parent: ~