diff --git a/crates/ruff/resources/test/fixtures/flake8_return/RET504.py b/crates/ruff/resources/test/fixtures/flake8_return/RET504.py index f54e499408..96bc755687 100644 --- a/crates/ruff/resources/test/fixtures/flake8_return/RET504.py +++ b/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 diff --git a/crates/ruff/src/rules/flake8_return/rules.rs b/crates/ruff/src/rules/flake8_return/rules.rs index 2c44ddd56c..fda0985d85 100644 --- a/crates/ruff/src/rules/flake8_return/rules.rs +++ b/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 after_assign: Option<&Location> = None; - if let Some(assigns) = stack.assigns.get(&id) { - for location in assigns.iter().sorted() { + let mut assignment_before_return: Option<&Location> = None; + let mut assignment_after_return: Option<&Location> = None; + if let Some(assignments) = stack.assignments.get(&id) { + 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) { - for location in refs { + // If there is no assignment before the return, then the variable must be defined in + // 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 before_assign.row() < location.row() && location.row() <= after_assign.row() { + if let Some(assignment_after_return) = assignment_after_return { + 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) { - for location in refs { + if let Some(references) = stack.references.get(&id) { + 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) { - for location in refs { + if let Some(assignments) = stack.assignments.get(&id) { + 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))); diff --git a/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap b/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap index f1968d62c6..5fd916181f 100644 --- a/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap +++ b/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 | queryset = Model.filter(a=1) -251 | return queryset # error +250 | def get_queryset(): +251 | queryset = Model.filter(a=1) +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 + | + diff --git a/crates/ruff/src/rules/flake8_return/visitor.rs b/crates/ruff/src/rules/flake8_return/visitor.rs index 9750730901..f44b76124b 100644 --- a/crates/ruff/src/rules/flake8_return/visitor.rs +++ b/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>, + pub references: FxHashMap<&'a str, Vec>, pub non_locals: FxHashSet<&'a str>, - pub assigns: FxHashMap<&'a str, Vec>, + pub assignments: FxHashMap<&'a str, Vec>, 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);