Ignore argument assignments when enforcing RET504 (#4004)

crates/ruff/resources/test/fixtures/flake8_return/RET504.py

@@ -3,7 +3,7 @@
 ###
 def x():
     a = 1
-    return a  # error
+    return a  # RET504
 
 
 # Can be refactored false positives
@@ -211,10 +211,10 @@ def nonlocal_assignment():
 def decorator() -> Flask:
     app = Flask(__name__)
 
-    @app.route('/hello')
+    @app.route("/hello")
     def hello() -> str:
         """Hello endpoint."""
-        return 'Hello, World!'
+        return "Hello, World!"
 
     return app
 
@@ -222,12 +222,13 @@ def decorator() -> Flask:
 def default():
     y = 1
 
-    def f(x = y) -> X:
+    def f(x=y) -> X:
         return x
 
     return y
 
 
+# Multiple assignment
 def get_queryset(option_1, option_2):
     queryset: Any = None
     queryset = queryset.filter(a=1)
@@ -246,4 +247,28 @@ def get_queryset():
 
 def get_queryset():
     queryset = Model.filter(a=1)
-    return queryset  # error
+    return queryset  # RET504
+
+
+# Function arguments
+def str_to_bool(val):
+    if isinstance(val, bool):
+        return val
+    val = val.strip().lower()
+    if val in ("1", "true", "yes"):
+        return True
+
+    return False
+
+
+def str_to_bool(val):
+    if isinstance(val, bool):
+        return val
+    val = 1
+    return val  # RET504
+
+
+def str_to_bool(val):
+    if isinstance(val, bool):
+        return some_obj
+    return val

crates/ruff/src/rules/flake8_return/rules.rs

@@ -306,8 +306,9 @@ fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
     }
 }
 
+/// Return `true` if the `id` has multiple assignments within the function.
 fn has_multiple_assigns(id: &str, stack: &Stack) -> bool {
-    if let Some(assigns) = stack.assigns.get(&id) {
+    if let Some(assigns) = stack.assignments.get(&id) {
         if assigns.len() > 1 {
             return true;
         }
@@ -315,41 +316,53 @@ fn has_multiple_assigns(id: &str, stack: &Stack) -> bool {
     false
 }
 
+/// Return `true` if the `id` has a (read) reference between the `return_location` and its
+/// preceding assignment.
 fn has_refs_before_next_assign(id: &str, return_location: Location, stack: &Stack) -> bool {
-    let mut before_assign: &Location = &Location::default();
+    let mut assignment_before_return: Option<&Location> = None;
-    let mut after_assign: Option<&Location> = None;
+    let mut assignment_after_return: Option<&Location> = None;
-    if let Some(assigns) = stack.assigns.get(&id) {
+    if let Some(assignments) = stack.assignments.get(&id) {
-        for location in assigns.iter().sorted() {
+        for location in assignments.iter().sorted() {
             if location.row() > return_location.row() {
-                after_assign = Some(location);
+                assignment_after_return = Some(location);
                 break;
             }
             if location.row() <= return_location.row() {
-                before_assign = location;
+                assignment_before_return = Some(location);
             }
         }
     }
 
-    if let Some(refs) = stack.refs.get(&id) {
+    // If there is no assignment before the return, then the variable must be defined in
-        for location in refs {
+    // some other way (e.g., a function argument). No need to check for references.
+    let Some(assignment_before_return) = assignment_before_return else {
+        return true;
+    };
+
+    if let Some(references) = stack.references.get(&id) {
+        for location in references {
             if location.row() == return_location.row() {
                 continue;
             }
-            if let Some(after_assign) = after_assign {
+            if let Some(assignment_after_return) = assignment_after_return {
-                if before_assign.row() < location.row() && location.row() <= after_assign.row() {
+                if assignment_before_return.row() < location.row()
+                    && location.row() <= assignment_after_return.row()
+                {
                     return true;
                 }
-            } else if before_assign.row() < location.row() {
+            } else if assignment_before_return.row() < location.row() {
                 return true;
             }
         }
     }
+
     false
 }
 
+/// Return `true` if the `id` has a read or write reference within a `try` or loop body.
 fn has_refs_or_assigns_within_try_or_loop(id: &str, stack: &Stack) -> bool {
-    if let Some(refs) = stack.refs.get(&id) {
+    if let Some(references) = stack.references.get(&id) {
-        for location in refs {
+        for location in references {
             for (try_location, try_end_location) in &stack.tries {
                 if try_location.row() < location.row() && location.row() <= try_end_location.row() {
                     return true;
@@ -363,8 +376,8 @@ fn has_refs_or_assigns_within_try_or_loop(id: &str, stack: &Stack) -> bool {
             }
         }
     }
-    if let Some(refs) = stack.assigns.get(&id) {
+    if let Some(assignments) = stack.assignments.get(&id) {
-        for location in refs {
+        for location in assignments {
             for (try_location, try_end_location) in &stack.tries {
                 if try_location.row() < location.row() && location.row() <= try_end_location.row() {
                     return true;
@@ -384,11 +397,11 @@ fn has_refs_or_assigns_within_try_or_loop(id: &str, stack: &Stack) -> bool {
 /// RET504
 fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) {
     if let ExprKind::Name { id, .. } = &expr.node {
-        if !stack.assigns.contains_key(id.as_str()) {
+        if !stack.assignments.contains_key(id.as_str()) {
             return;
         }
 
-        if !stack.refs.contains_key(id.as_str()) {
+        if !stack.references.contains_key(id.as_str()) {
             checker
                 .diagnostics
                 .push(Diagnostic::new(UnnecessaryAssign, Range::from(expr)));

crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap

@@ -5,16 +5,24 @@ RET504.py:6:12: RET504 Unnecessary variable assignment before `return` statement
   |
 6 | def x():
 7 |     a = 1
-8 |     return a  # error
+8 |     return a  # RET504
   |            ^ RET504
   |
 
-RET504.py:249:12: RET504 Unnecessary variable assignment before `return` statement
+RET504.py:250:12: RET504 Unnecessary variable assignment before `return` statement
     |
-249 | def get_queryset():
+250 | def get_queryset():
-250 |     queryset = Model.filter(a=1)
+251 |     queryset = Model.filter(a=1)
-251 |     return queryset  # error
+252 |     return queryset  # RET504
     |            ^^^^^^^^ RET504
     |
 
+RET504.py:268:12: RET504 Unnecessary variable assignment before `return` statement
+    |
+268 |         return val
+269 |     val = 1
+270 |     return val  # RET504
+    |            ^^^ RET504
+    |
+
 

crates/ruff/src/rules/flake8_return/visitor.rs

@@ -10,9 +10,9 @@ pub struct Stack<'a> {
     pub yields: Vec<&'a Expr>,
     pub elses: Vec<&'a Stmt>,
     pub elifs: Vec<&'a Stmt>,
-    pub refs: FxHashMap<&'a str, Vec<Location>>,
+    pub references: FxHashMap<&'a str, Vec<Location>>,
     pub non_locals: FxHashSet<&'a str>,
-    pub assigns: FxHashMap<&'a str, Vec<Location>>,
+    pub assignments: FxHashMap<&'a str, Vec<Location>>,
     pub loops: Vec<(Location, Location)>,
     pub tries: Vec<(Location, Location)>,
 }
@@ -34,7 +34,7 @@ impl<'a> ReturnVisitor<'a> {
             }
             ExprKind::Name { id, .. } => {
                 self.stack
-                    .assigns
+                    .assignments
                     .entry(id)
                     .or_insert_with(Vec::new)
                     .push(expr.location);
@@ -44,9 +44,9 @@ impl<'a> ReturnVisitor<'a> {
                 // Attribute assignments are often side-effects (e.g., `self.property = value`),
                 // so we conservatively treat them as references to every known
                 // variable.
-                for name in self.stack.assigns.keys() {
+                for name in self.stack.assignments.keys() {
                     self.stack
-                        .refs
+                        .references
                         .entry(name)
                         .or_insert_with(Vec::new)
                         .push(expr.location);
@@ -126,7 +126,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> {
             StmtKind::Assign { targets, value, .. } => {
                 if let ExprKind::Name { id, .. } = &value.node {
                     self.stack
-                        .refs
+                        .references
                         .entry(id)
                         .or_insert_with(Vec::new)
                         .push(value.location);
@@ -176,9 +176,9 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> {
             ExprKind::Call { .. } => {
                 // Arbitrary function calls can have side effects, so we conservatively treat
                 // every function call as a reference to every known variable.
-                for name in self.stack.assigns.keys() {
+                for name in self.stack.assignments.keys() {
                     self.stack
-                        .refs
+                        .references
                         .entry(name)
                         .or_insert_with(Vec::new)
                         .push(expr.location);
@@ -186,7 +186,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> {
             }
             ExprKind::Name { id, .. } => {
                 self.stack
-                    .refs
+                    .references
                     .entry(id)
                     .or_insert_with(Vec::new)
                     .push(expr.location);