Skip to content

Commit

Permalink
Simplify RET504 rules
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Jun 10, 2023
1 parent f401050 commit 7326d63
Show file tree
Hide file tree
Showing 7 changed files with 161 additions and 313 deletions.
19 changes: 9 additions & 10 deletions crates/ruff/src/rules/flake8_return/helpers.rs
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
use ruff_text_size::TextSize;
use rustpython_parser::ast;
use rustpython_parser::ast::{Expr, Ranged, Stmt};

use ruff_python_ast::source_code::Locator;
use ruff_python_whitespace::UniversalNewlines;

/// Return `true` if a function's return statement include at least one
/// non-`None` value.
pub(super) fn result_exists(returns: &[(&Stmt, Option<&Expr>)]) -> bool {
returns.iter().any(|(_, expr)| {
expr.map(|expr| {
pub(super) fn result_exists(returns: &[&ast::StmtReturn]) -> bool {
returns.iter().any(|stmt| {
stmt.value.as_deref().map_or(false, |value| {
!matches!(
expr,
Expr::Constant(ref constant) if constant.value.is_none()
value,
Expr::Constant(constant) if constant.value.is_none()
)
})
.unwrap_or(false)
})
}

Expand All @@ -26,12 +26,11 @@ pub(super) fn result_exists(returns: &[(&Stmt, Option<&Expr>)]) -> bool {
/// This method assumes that the statement is the last statement in its body; specifically, that
/// the statement isn't followed by a semicolon, followed by a multi-line statement.
pub(super) fn end_of_last_statement(stmt: &Stmt, locator: &Locator) -> TextSize {
// End-of-file, so just return the end of the statement.
if stmt.end() == locator.text_len() {
// End-of-file, so just return the end of the statement.
stmt.end()
}
// Otherwise, find the end of the last line that's "part of" the statement.
else {
} else {
// Otherwise, find the end of the last line that's "part of" the statement.
let contents = locator.after(stmt.end());

for line in contents.universal_newlines() {
Expand Down
190 changes: 52 additions & 138 deletions crates/ruff/src/rules/flake8_return/rules/function.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use itertools::Itertools;
use ruff_text_size::{TextRange, TextSize};
use rustpython_parser::ast::{self, Constant, Expr, Ranged, Stmt};

use ruff_diagnostics::{AlwaysAutofixableViolation, Violation};
Expand Down Expand Up @@ -139,8 +137,8 @@ impl AlwaysAutofixableViolation for ImplicitReturn {
}

/// ## What it does
/// Checks for variable assignments that are unused between the assignment and
/// a `return` of the variable.
/// Checks for variable assignments that immediately precede a `return` of the
/// assigned variable.
///
/// ## Why is this bad?
/// The variable assignment is not necessary as the value can be returned
Expand All @@ -159,12 +157,15 @@ impl AlwaysAutofixableViolation for ImplicitReturn {
/// return 1
/// ```
#[violation]
pub struct UnnecessaryAssign;
pub struct UnnecessaryAssign {
name: String,
}

impl Violation for UnnecessaryAssign {
#[derive_message_formats]
fn message(&self) -> String {
format!("Unnecessary variable assignment before `return` statement")
let UnnecessaryAssign { name } = self;
format!("Unnecessary assignment to `{name}` before `return` statement")
}
}

Expand Down Expand Up @@ -326,8 +327,8 @@ impl Violation for SuperfluousElseBreak {

/// RET501
fn unnecessary_return_none(checker: &mut Checker, stack: &Stack) {
for (stmt, expr) in &stack.returns {
let Some(expr) = expr else {
for stmt in &stack.returns {
let Some(expr) = stmt.value.as_deref() else {
continue;
};
if !matches!(
Expand All @@ -339,10 +340,9 @@ fn unnecessary_return_none(checker: &mut Checker, stack: &Stack) {
) {
continue;
}
let mut diagnostic = Diagnostic::new(UnnecessaryReturnNone, stmt.range());
let mut diagnostic = Diagnostic::new(UnnecessaryReturnNone, stmt.range);
if checker.patch(diagnostic.kind.rule()) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
"return".to_string(),
stmt.range(),
)));
Expand All @@ -353,16 +353,15 @@ fn unnecessary_return_none(checker: &mut Checker, stack: &Stack) {

/// RET502
fn implicit_return_value(checker: &mut Checker, stack: &Stack) {
for (stmt, expr) in &stack.returns {
if expr.is_some() {
for stmt in &stack.returns {
if stmt.value.is_some() {
continue;
}
let mut diagnostic = Diagnostic::new(ImplicitReturnValue, stmt.range());
let mut diagnostic = Diagnostic::new(ImplicitReturnValue, stmt.range);
if checker.patch(diagnostic.kind.rule()) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
"return None".to_string(),
stmt.range(),
stmt.range,
)));
}
checker.diagnostics.push(diagnostic);
Expand Down Expand Up @@ -417,8 +416,7 @@ fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
content.push_str(checker.stylist.line_ending().as_str());
content.push_str(indent);
content.push_str("return None");
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::insertion(
diagnostic.set_fix(Fix::suggested(Edit::insertion(
content,
end_of_last_statement(stmt, checker.locator),
)));
Expand Down Expand Up @@ -456,8 +454,7 @@ fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
content.push_str(checker.stylist.line_ending().as_str());
content.push_str(indent);
content.push_str("return None");
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::insertion(
diagnostic.set_fix(Fix::suggested(Edit::insertion(
content,
end_of_last_statement(stmt, checker.locator),
)));
Expand Down Expand Up @@ -494,8 +491,7 @@ fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
content.push_str(checker.stylist.line_ending().as_str());
content.push_str(indent);
content.push_str("return None");
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::insertion(
diagnostic.set_fix(Fix::suggested(Edit::insertion(
content,
end_of_last_statement(stmt, checker.locator),
)));
Expand All @@ -506,129 +502,51 @@ fn implicit_return(checker: &mut Checker, stmt: &Stmt) {
}
}

/// Return `true` if the `id` has multiple declarations within the function.
fn has_multiple_declarations(id: &str, stack: &Stack) -> bool {
stack
.declarations
.get(&id)
.map_or(false, |declarations| declarations.len() > 1)
}

/// Return `true` if the `id` has a (read) reference between the `return_location` and its
/// preceding declaration.
fn has_references_before_next_declaration(
id: &str,
return_range: TextRange,
stack: &Stack,
) -> bool {
let mut declaration_before_return: Option<TextSize> = None;
let mut declaration_after_return: Option<TextSize> = None;
if let Some(assignments) = stack.declarations.get(&id) {
for location in assignments.iter().sorted() {
if *location > return_range.start() {
declaration_after_return = Some(*location);
break;
}
declaration_before_return = Some(*location);
}
}

// If there is no declaration before the return, then the variable must be declared in
// some other way (e.g., a function argument). No need to check for references.
let Some(declaration_before_return) = declaration_before_return else {
return true;
};

if let Some(references) = stack.references.get(&id) {
for location in references {
if return_range.contains(*location) {
continue;
}

if declaration_before_return < *location {
if let Some(declaration_after_return) = declaration_after_return {
if *location <= declaration_after_return {
return true;
}
} else {
return true;
}
}
}
}
/// RET504
fn unnecessary_assign(checker: &mut Checker, stack: &Stack) {
for (stmt_assign, stmt_return) in &stack.assignments {
// Identify, e.g., `return x`.
let Some(value) = stmt_return.value.as_ref() else {
continue;
};

false
}
let Expr::Name(ast::ExprName { id: returned_id, .. }) = value.as_ref() else {
continue;
};

/// Return `true` if the `id` has a read or write reference within a `try` or loop body.
fn has_references_or_declarations_within_try_or_loop(id: &str, stack: &Stack) -> bool {
if let Some(references) = stack.references.get(&id) {
for location in references {
for try_range in &stack.tries {
if try_range.contains(*location) {
return true;
}
}
for loop_range in &stack.loops {
if loop_range.contains(*location) {
return true;
}
}
}
}
if let Some(references) = stack.declarations.get(&id) {
for location in references {
for try_range in &stack.tries {
if try_range.contains(*location) {
return true;
}
}
for loop_range in &stack.loops {
if loop_range.contains(*location) {
return true;
}
}
// Identify, e.g., `x = 1`.
if stmt_assign.targets.len() > 1 {
continue;
}
}
false
}

/// RET504
fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) {
if let Expr::Name(ast::ExprName { id, .. }) = expr {
if !stack.assigned_names.contains(id.as_str()) {
return;
}
let Some(target) = stmt_assign.targets.first() else {
continue;
};

if !stack.references.contains_key(id.as_str()) {
checker
.diagnostics
.push(Diagnostic::new(UnnecessaryAssign, expr.range()));
return;
}
let Expr::Name(ast::ExprName { id: assigned_id, .. }) = target else {
continue;
};

if has_multiple_declarations(id, stack)
|| has_references_before_next_declaration(id, expr.range(), stack)
|| has_references_or_declarations_within_try_or_loop(id, stack)
{
return;
if returned_id != assigned_id {
continue;
}

if stack.non_locals.contains(id.as_str()) {
return;
if stack.non_locals.contains(assigned_id.as_str()) {
continue;
}

checker
.diagnostics
.push(Diagnostic::new(UnnecessaryAssign, expr.range()));
checker.diagnostics.push(Diagnostic::new(
UnnecessaryAssign {
name: assigned_id.to_string(),
},
value.range(),
));
}
}

/// RET505, RET506, RET507, RET508
fn superfluous_else_node(checker: &mut Checker, stmt: &Stmt, branch: Branch) -> bool {
let Stmt::If(ast::StmtIf { body, .. }) = stmt else {
return false;
};
fn superfluous_else_node(checker: &mut Checker, stmt: &ast::StmtIf, branch: Branch) -> bool {
let ast::StmtIf { body, .. } = stmt;
for child in body {
if child.is_return_stmt() {
let diagnostic = Diagnostic::new(
Expand Down Expand Up @@ -708,7 +626,7 @@ pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Ex
};

// Avoid false positives for generators.
if !stack.yields.is_empty() {
if stack.is_generator {
return;
}

Expand Down Expand Up @@ -737,11 +655,7 @@ pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Ex
}

if checker.enabled(Rule::UnnecessaryAssign) {
for (_, expr) in &stack.returns {
if let Some(expr) = expr {
unnecessary_assign(checker, &stack, expr);
}
}
unnecessary_assign(checker, &stack);
}
} else {
if checker.enabled(Rule::UnnecessaryReturnNone) {
Expand Down
4 changes: 2 additions & 2 deletions ...rc/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET501_RET501.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ RET501.py:4:5: RET501 [*] Do not explicitly `return None` in function if it is t
|
= help: Remove explicit `return None`

ℹ Suggested fix
ℹ Fix
1 1 | def x(y):
2 2 | if not y:
3 3 | return
Expand All @@ -29,7 +29,7 @@ RET501.py:14:9: RET501 [*] Do not explicitly `return None` in function if it is
|
= help: Remove explicit `return None`

ℹ Suggested fix
ℹ Fix
11 11 |
12 12 | def get(self, key: str) -> None:
13 13 | print(f"{key} not found")
Expand Down
2 changes: 1 addition & 1 deletion ...rc/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET502_RET502.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ RET502.py:3:9: RET502 [*] Do not implicitly `return None` in function able to re
|
= help: Add explicit `None` return value

ℹ Suggested fix
ℹ Fix
1 1 | def x(y):
2 2 | if not y:
3 |- return # error
Expand Down
Loading

0 comments on commit 7326d63

Please sign in to comment.