From 2a63760fbdb73a5228423613e2351d4347d8cec6 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Sun, 5 Feb 2023 13:58:31 -0800 Subject: [PATCH 1/8] Add basic start for autofix, update README. --- README.md | 2 +- src/rules/flake8_return/rules.rs | 21 +++++++++++++++++---- src/rules/flake8_return/visitor.rs | 8 ++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 7ba20e9bf546c..f24349de23484 100644 --- a/README.md +++ b/README.md @@ -1175,7 +1175,7 @@ For more, see [flake8-return](https://pypi.org/project/flake8-return/) on PyPI. | RET501 | unnecessary-return-none | Do not explicitly `return None` in function if it is the only possible return value | 🛠 | | RET502 | implicit-return-value | Do not implicitly `return None` in function able to return non-`None` value | 🛠 | | RET503 | implicit-return | Missing explicit `return` at the end of function able to return non-`None` value | 🛠 | -| RET504 | unnecessary-assign | Unnecessary variable assignment before `return` statement | | +| RET504 | unnecessary-assign | Unnecessary variable assignment before `return` statement | 🛠 | | RET505 | superfluous-else-return | Unnecessary `{branch}` after `return` statement | | | RET506 | superfluous-else-raise | Unnecessary `{branch}` after `raise` statement | | | RET507 | superfluous-else-continue | Unnecessary `{branch}` after `continue` statement | | diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index 7ec1203ba6cca..a42235aa28b09 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -4,7 +4,7 @@ use rustpython_ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind}; use super::branch::Branch; use super::helpers::result_exists; use super::visitor::{ReturnVisitor, Stack}; -use crate::ast::helpers::elif_else_range; +use crate::ast::helpers::{elif_else_range, unparse_expr}; use crate::ast::types::Range; use crate::ast::visitor::Visitor; use crate::ast::whitespace::indentation; @@ -12,7 +12,7 @@ use crate::checkers::ast::Checker; use crate::define_violation; use crate::fix::Fix; use crate::registry::{Diagnostic, Rule}; -use crate::violation::{AlwaysAutofixableViolation, Violation}; +use crate::violation::{AlwaysAutofixableViolation, AutofixKind, Availability, Violation}; use ruff_macros::derive_message_formats; define_violation!( @@ -63,10 +63,15 @@ define_violation!( pub struct UnnecessaryAssign; ); impl Violation for UnnecessaryAssign { + const AUTOFIX: Option = Some(AutofixKind {available: Availability::Sometimes} ); #[derive_message_formats] fn message(&self) -> String { format!("Unnecessary variable assignment before `return` statement") } + + fn autofix_title_formatter(&self) -> Option String> { + Some(|_| "Remove variable definition and place the constant in the `return` statement".to_string()) + } } define_violation!( @@ -302,6 +307,7 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { } if !stack.refs.contains_key(id.as_str()) { + // No autofix, these cases are complicated enough to require human refactoring. checker.diagnostics.push(Diagnostic::new( UnnecessaryAssign, Range::from_located(expr), @@ -319,10 +325,17 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { return; } - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( UnnecessaryAssign, Range::from_located(expr), - )); + ); + + if checker.patch(diagnostic.kind.rule()){ + if let Some(assign_expr) = stack.assign_values.get(id.as_str()) { + diagnostic.amend(Fix::replacement(unparse_expr(assign_expr, checker.stylist), expr.location, expr.end_location.expect("Expression has no end location"))); + } + } + checker.diagnostics.push(diagnostic); } } diff --git a/src/rules/flake8_return/visitor.rs b/src/rules/flake8_return/visitor.rs index 4e58d06547e4e..84129bb4feae6 100644 --- a/src/rules/flake8_return/visitor.rs +++ b/src/rules/flake8_return/visitor.rs @@ -15,6 +15,7 @@ pub struct Stack<'a> { pub assigns: FxHashMap<&'a str, Vec>, pub loops: Vec<(Location, Location)>, pub tries: Vec<(Location, Location)>, + pub assign_values: FxHashMap<&'a str, rustpython_ast::Located>, } #[derive(Default)] @@ -84,6 +85,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { visitor::walk_stmt(self, stmt); } StmtKind::Assign { targets, value, .. } => { + if let ExprKind::Name { id, .. } = &value.node { self.stack .refs @@ -102,6 +104,12 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { return; } + // TODO: Make this only run if the autofix is enabled, as this incurs a + // slight performance overhead by using `.clone` + if let ExprKind::Name { id, .. } = &target.node { + self.stack.assign_values.insert(id, *value.clone()); + } + self.visit_assign_target(target); } } From c1c59bafa770437ec5f2131597d6a32f57af2767 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Sun, 5 Feb 2023 14:09:44 -0800 Subject: [PATCH 2/8] fmt + clippy --- src/rules/flake8_return/rules.rs | 22 ++++++++++++++-------- src/rules/flake8_return/visitor.rs | 3 +-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index a42235aa28b09..c867c20046d9e 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -63,14 +63,19 @@ define_violation!( pub struct UnnecessaryAssign; ); impl Violation for UnnecessaryAssign { - const AUTOFIX: Option = Some(AutofixKind {available: Availability::Sometimes} ); + const AUTOFIX: Option = Some(AutofixKind { + available: Availability::Sometimes, + }); #[derive_message_formats] fn message(&self) -> String { format!("Unnecessary variable assignment before `return` statement") } fn autofix_title_formatter(&self) -> Option String> { - Some(|_| "Remove variable definition and place the constant in the `return` statement".to_string()) + Some(|_| { + "Remove variable definition and place the constant in the `return` statement" + .to_string() + }) } } @@ -325,14 +330,15 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { return; } - let mut diagnostic = Diagnostic::new( - UnnecessaryAssign, - Range::from_located(expr), - ); + let mut diagnostic = Diagnostic::new(UnnecessaryAssign, Range::from_located(expr)); - if checker.patch(diagnostic.kind.rule()){ + if checker.patch(diagnostic.kind.rule()) { if let Some(assign_expr) = stack.assign_values.get(id.as_str()) { - diagnostic.amend(Fix::replacement(unparse_expr(assign_expr, checker.stylist), expr.location, expr.end_location.expect("Expression has no end location"))); + diagnostic.amend(Fix::replacement( + unparse_expr(assign_expr, checker.stylist), + expr.location, + expr.end_location.expect("Expression has no end location"), + )); } } checker.diagnostics.push(diagnostic); diff --git a/src/rules/flake8_return/visitor.rs b/src/rules/flake8_return/visitor.rs index 84129bb4feae6..63369b82167f2 100644 --- a/src/rules/flake8_return/visitor.rs +++ b/src/rules/flake8_return/visitor.rs @@ -85,7 +85,6 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { visitor::walk_stmt(self, stmt); } StmtKind::Assign { targets, value, .. } => { - if let ExprKind::Name { id, .. } = &value.node { self.stack .refs @@ -105,7 +104,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { } // TODO: Make this only run if the autofix is enabled, as this incurs a - // slight performance overhead by using `.clone` + // slight performance overhead by using `.clone` if let ExprKind::Name { id, .. } = &target.node { self.stack.assign_values.insert(id, *value.clone()); } From 79a3f86727ceb1086385b1004ba9cf8b70b4f212 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Sun, 5 Feb 2023 18:56:41 -0800 Subject: [PATCH 3/8] Nearly done -- just need to unindent one level --- src/rules/flake8_return/rules.rs | 41 +++++++++++++++++++++++++----- src/rules/flake8_return/visitor.rs | 23 +++++++++-------- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index c867c20046d9e..ecfd97e46e772 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -242,7 +242,8 @@ fn has_refs_before_next_assign(id: &str, return_location: Location, stack: &Stac 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() { + for range in assigns.iter().sorted() { + let location = &range.location; if location.row() > return_location.row() { after_assign = Some(location); break; @@ -287,7 +288,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 { + for range in refs { + let location = &range.location; for (try_location, try_end_location) in &stack.tries { if try_location.row() < location.row() && location.row() <= try_end_location.row() { return true; @@ -334,11 +336,36 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { if checker.patch(diagnostic.kind.rule()) { if let Some(assign_expr) = stack.assign_values.get(id.as_str()) { - diagnostic.amend(Fix::replacement( - unparse_expr(assign_expr, checker.stylist), - expr.location, - expr.end_location.expect("Expression has no end location"), - )); + if let Some(assign_locations) = stack.assigns.get(id.as_str()) { + let expr_loc = expr.end_location.unwrap(); + if let Some(last_assign_loc) = assign_locations + .into_iter() + .rev() + .find(|x| x.location < expr_loc) + { + let in_between_source = checker + .locator + .slice_source_code_range(&Range::new( + last_assign_loc.end_location, + expr.location, + )) + .to_owned(); + let fixed_source = match in_between_source.strip_prefix(|chr| { + checker.stylist.line_ending().as_str().contains(chr) || chr == ';' + }) { + Some(val) => val.to_owned(), + None => in_between_source, + }; // Avoid trailing newline and causing invalid syntax with semicolon usage + println!("{:?}\n{:?}\n{:?}\n",fixed_source.clone() + &unparse_expr(&assign_expr.clone(), checker.stylist), + last_assign_loc.location, + expr.end_location.unwrap()); + diagnostic.amend(Fix::replacement( + fixed_source + &unparse_expr(&assign_expr.clone(), checker.stylist), + last_assign_loc.location, + expr.end_location.unwrap(), + )); + } + } } } checker.diagnostics.push(diagnostic); diff --git a/src/rules/flake8_return/visitor.rs b/src/rules/flake8_return/visitor.rs index 63369b82167f2..3bd95d2994628 100644 --- a/src/rules/flake8_return/visitor.rs +++ b/src/rules/flake8_return/visitor.rs @@ -1,6 +1,7 @@ use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{Expr, ExprKind, Location, Stmt, StmtKind}; +use crate::ast::types::Range; use crate::ast::visitor; use crate::ast::visitor::Visitor; @@ -12,10 +13,10 @@ pub struct Stack<'a> { pub elifs: Vec<&'a Stmt>, pub refs: FxHashMap<&'a str, Vec>, pub non_locals: FxHashSet<&'a str>, - pub assigns: FxHashMap<&'a str, Vec>, + pub assigns: FxHashMap<&'a str, Vec>, pub loops: Vec<(Location, Location)>, pub tries: Vec<(Location, Location)>, - pub assign_values: FxHashMap<&'a str, rustpython_ast::Located>, + pub assign_values: FxHashMap<&'a str, Expr>, } #[derive(Default)] @@ -32,14 +33,6 @@ impl<'a> ReturnVisitor<'a> { } return; } - ExprKind::Name { id, .. } => { - self.stack - .assigns - .entry(id) - .or_insert_with(Vec::new) - .push(expr.location); - return; - } ExprKind::Attribute { .. } => { // Attribute assignments are often side-effects (e.g., `self.property = value`), // so we conservatively treat them as references to every known @@ -109,6 +102,16 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { self.stack.assign_values.insert(id, *value.clone()); } + // This is hacky, but I don't kmow how to pass the "context" of the + // statement in another way. + if let ExprKind::Name { id, .. } = &target.node { + self.stack + .assigns + .entry(&id) + .or_insert_with(Vec::new) + .push(Range::from_located(stmt)); + return; + } self.visit_assign_target(target); } } From 6cf8c607514086b3482f1291e2d53326ffdd2749 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Thu, 9 Feb 2023 16:50:19 -0800 Subject: [PATCH 4/8] Snap updated, 90% working, one false positive --- src/rules/flake8_return/rules.rs | 28 ++++++---- ...lake8_return__tests__RET504_RET504.py.snap | 51 +++++++++++++++++-- src/rules/flake8_return/visitor.rs | 2 +- 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index ecfd97e46e772..fc28d016c5384 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -339,7 +339,7 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { if let Some(assign_locations) = stack.assigns.get(id.as_str()) { let expr_loc = expr.end_location.unwrap(); if let Some(last_assign_loc) = assign_locations - .into_iter() + .iter() .rev() .find(|x| x.location < expr_loc) { @@ -350,15 +350,23 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { expr.location, )) .to_owned(); - let fixed_source = match in_between_source.strip_prefix(|chr| { - checker.stylist.line_ending().as_str().contains(chr) || chr == ';' - }) { - Some(val) => val.to_owned(), - None => in_between_source, - }; // Avoid trailing newline and causing invalid syntax with semicolon usage - println!("{:?}\n{:?}\n{:?}\n",fixed_source.clone() + &unparse_expr(&assign_expr.clone(), checker.stylist), - last_assign_loc.location, - expr.end_location.unwrap()); + // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage + let fixed_source = match in_between_source + .strip_prefix(checker.stylist.line_ending().as_str()) + { + Some(newline_stripped) => { + match newline_stripped + .strip_prefix(checker.stylist.indentation().as_str()) + { + Some(unindented) => unindented.to_owned(), + None => in_between_source, + } + } + None => match in_between_source.strip_prefix(';') { + Some(no_semicolon) => no_semicolon.to_owned(), + None => in_between_source, + }, + }; diagnostic.amend(Fix::replacement( fixed_source + &unparse_expr(&assign_expr.clone(), checker.stylist), last_assign_loc.location, diff --git a/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap b/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap index 8b9969a243162..0f515cf10de49 100644 --- a/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap +++ b/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap @@ -10,7 +10,15 @@ expression: diagnostics end_location: row: 6 column: 12 - fix: ~ + fix: + content: + - return 1 + location: + row: 5 + column: 4 + end_location: + row: 6 + column: 12 parent: ~ - kind: UnnecessaryAssign: ~ @@ -20,7 +28,15 @@ expression: diagnostics end_location: row: 13 column: 12 - fix: ~ + fix: + content: + - return 2 + location: + row: 12 + column: 4 + end_location: + row: 13 + column: 12 parent: ~ - kind: UnnecessaryAssign: ~ @@ -30,7 +46,16 @@ expression: diagnostics end_location: row: 19 column: 16 - fix: ~ + fix: + content: + - "if True:" + - " return 2" + location: + row: 17 + column: 4 + end_location: + row: 19 + column: 16 parent: ~ - kind: UnnecessaryAssign: ~ @@ -40,7 +65,15 @@ expression: diagnostics end_location: row: 31 column: 17 - fix: ~ + fix: + content: + - return str(obj.bar) + location: + row: 30 + column: 8 + end_location: + row: 31 + column: 17 parent: ~ - kind: UnnecessaryAssign: ~ @@ -50,6 +83,14 @@ expression: diagnostics end_location: row: 39 column: 20 - fix: ~ + fix: + content: + - "return formatted.replace(\"()\", \"\").replace(\" \", \" \").strip()" + location: + row: 38 + column: 4 + end_location: + row: 39 + column: 20 parent: ~ diff --git a/src/rules/flake8_return/visitor.rs b/src/rules/flake8_return/visitor.rs index 3bd95d2994628..73b3c46bd6a87 100644 --- a/src/rules/flake8_return/visitor.rs +++ b/src/rules/flake8_return/visitor.rs @@ -107,7 +107,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { if let ExprKind::Name { id, .. } = &target.node { self.stack .assigns - .entry(&id) + .entry(id) .or_insert_with(Vec::new) .push(Range::from_located(stmt)); return; From 058538bfe0ebb600b62af5f6a8baf278e83b6ef6 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Thu, 9 Feb 2023 20:47:23 -0800 Subject: [PATCH 5/8] Nearly done -- hanging it up for today though --- src/rules/flake8_return/helpers.rs | 24 ++++++++++++ src/rules/flake8_return/rules.rs | 63 +++++++++++++++++++----------- 2 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/rules/flake8_return/helpers.rs b/src/rules/flake8_return/helpers.rs index 484c23f686c6d..a2563ffb21cc7 100644 --- a/src/rules/flake8_return/helpers.rs +++ b/src/rules/flake8_return/helpers.rs @@ -1,5 +1,8 @@ use rustpython_ast::{Constant, Expr, ExprKind, Stmt}; +use rustpython_parser::lexer; +use rustpython_parser::lexer::Tok; + /// Return `true` if a function's return statement include at least one /// non-`None` value. pub fn result_exists(returns: &[(&Stmt, Option<&Expr>)]) -> bool { @@ -16,3 +19,24 @@ pub fn result_exists(returns: &[(&Stmt, Option<&Expr>)]) -> bool { .unwrap_or(false) }) } + +/// Check if the specified range is composed of exclusively comments +/// and a single return statement. +pub fn code_is_only_comments(code: &str) -> bool { + let code_tokens = lexer::make_tokenizer(code).flatten(); + for (_, tok, _) in code_tokens { + if !matches!( + tok, + Tok::Comment(..) + | Tok::String { .. } + | Tok::Pass + | Tok::Newline + | Tok::NonLogicalNewline + ) { + println!("{:?}", tok); + return false; + } + } + println!("{:?}", code); + true +} diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index fc28d016c5384..e28d48ab85a8b 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -1,8 +1,9 @@ use itertools::Itertools; +use libcst_native::Tuple; use rustpython_ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind}; use super::branch::Branch; -use super::helpers::result_exists; +use super::helpers::{code_is_only_comments, result_exists}; use super::visitor::{ReturnVisitor, Stack}; use crate::ast::helpers::{elif_else_range, unparse_expr}; use crate::ast::types::Range; @@ -350,28 +351,46 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { expr.location, )) .to_owned(); - // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage - let fixed_source = match in_between_source - .strip_prefix(checker.stylist.line_ending().as_str()) - { - Some(newline_stripped) => { - match newline_stripped - .strip_prefix(checker.stylist.indentation().as_str()) - { - Some(unindented) => unindented.to_owned(), - None => in_between_source, + // Ensure that all indentation is the same (no if or while statement). + let lines_iter = (checker + .locator + .slice_source_code_range(&Range::new( + Location::new(last_assign_loc.location.row(), 0), + Location::new(last_assign_loc.location.row() + 1, 0), + )) + .chars() + .take_while(|c| checker.stylist.indentation().contains(*c)) + .join("") + .to_owned() + + &in_between_source) + .lines().tuple_windows().any(|ln: (&str, &str)| ln == ln); + // Check if the code without the final return statement is only comments + if code_is_only_comments( + in_between_source.trim_end().trim_end_matches("return"), + ) { + // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage + let fixed_source = match in_between_source + .strip_prefix(checker.stylist.line_ending().as_str()) + { + Some(newline_stripped) => { + match newline_stripped + .strip_prefix(checker.stylist.indentation().as_str()) + { + Some(unindented) => unindented.to_owned(), + None => in_between_source, + } } - } - None => match in_between_source.strip_prefix(';') { - Some(no_semicolon) => no_semicolon.to_owned(), - None => in_between_source, - }, - }; - diagnostic.amend(Fix::replacement( - fixed_source + &unparse_expr(&assign_expr.clone(), checker.stylist), - last_assign_loc.location, - expr.end_location.unwrap(), - )); + None => match in_between_source.strip_prefix(';') { + Some(no_semicolon) => no_semicolon.to_owned(), + None => in_between_source, + }, + }; + diagnostic.amend(Fix::replacement( + fixed_source + &unparse_expr(&assign_expr.clone(), checker.stylist), + last_assign_loc.location, + expr.end_location.unwrap(), + )); + } } } } From e4fca869d468fbf1f9e48e9f5ea8a09119c13bbe Mon Sep 17 00:00:00 2001 From: Sawbez Date: Thu, 9 Feb 2023 21:00:22 -0800 Subject: [PATCH 6/8] Working but making an attempt to use better method --- src/rules/flake8_return/rules.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index e28d48ab85a8b..c61f2a1f815f6 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -1,5 +1,4 @@ use itertools::Itertools; -use libcst_native::Tuple; use rustpython_ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind}; use super::branch::Branch; @@ -352,7 +351,7 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { )) .to_owned(); // Ensure that all indentation is the same (no if or while statement). - let lines_iter = (checker + let all_indentation_same = (checker .locator .slice_source_code_range(&Range::new( Location::new(last_assign_loc.location.row(), 0), @@ -363,9 +362,20 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { .join("") .to_owned() + &in_between_source) - .lines().tuple_windows().any(|ln: (&str, &str)| ln == ln); + .lines() + .tuple_windows() + .any(|ln: (&str, &str)| { + ln.0.chars() + .take_while(|c| checker.stylist.indentation().contains(*c)) + .collect::() + == ln + .1 + .chars() + .take_while(|c| checker.stylist.indentation().contains(*c)) + .collect::() + }); // Check if the code without the final return statement is only comments - if code_is_only_comments( + if all_indentation_same && code_is_only_comments( in_between_source.trim_end().trim_end_matches("return"), ) { // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage From ebd2659adb73d0b8b0e4372f0860c06d275f6c07 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Thu, 9 Feb 2023 21:38:18 -0800 Subject: [PATCH 7/8] Fixed bad implementation, added tests + fix snap --- .../test/fixtures/flake8_return/RET504.py | 12 +++++ src/rules/flake8_return/helpers.rs | 17 +++++-- src/rules/flake8_return/rules.rs | 25 ++++------ ...lake8_return__tests__RET504_RET504.py.snap | 47 +++++++++++-------- 4 files changed, 61 insertions(+), 40 deletions(-) diff --git a/resources/test/fixtures/flake8_return/RET504.py b/resources/test/fixtures/flake8_return/RET504.py index 92bd411d8d0ab..5ccfb302dda93 100644 --- a/resources/test/fixtures/flake8_return/RET504.py +++ b/resources/test/fixtures/flake8_return/RET504.py @@ -22,6 +22,18 @@ def x(): return a +def x(): + var = [5, 4, 3, 2, 1] + """ + This is a + block comment + we want to keep + """ + # This is a comment that we want to keep. + return var # Another critical comment + # Last important comment + + # Can be refactored false positives # https://github.com/afonasev/flake8-return/issues/47#issuecomment-1122571066 def get_bar_if_exists(obj): diff --git a/src/rules/flake8_return/helpers.rs b/src/rules/flake8_return/helpers.rs index a2563ffb21cc7..2f76cd6055d11 100644 --- a/src/rules/flake8_return/helpers.rs +++ b/src/rules/flake8_return/helpers.rs @@ -3,6 +3,8 @@ use rustpython_ast::{Constant, Expr, ExprKind, Stmt}; use rustpython_parser::lexer; use rustpython_parser::lexer::Tok; +use crate::source_code::Stylist; + /// Return `true` if a function's return statement include at least one /// non-`None` value. pub fn result_exists(returns: &[(&Stmt, Option<&Expr>)]) -> bool { @@ -20,10 +22,10 @@ pub fn result_exists(returns: &[(&Stmt, Option<&Expr>)]) -> bool { }) } -/// Check if the specified range is composed of exclusively comments -/// and a single return statement. +/// Check if the given piece code is composed of exclusively comments pub fn code_is_only_comments(code: &str) -> bool { - let code_tokens = lexer::make_tokenizer(code).flatten(); + let dedented = textwrap::dedent(code); + let code_tokens = lexer::make_tokenizer(&dedented).flatten(); for (_, tok, _) in code_tokens { if !matches!( tok, @@ -33,10 +35,15 @@ pub fn code_is_only_comments(code: &str) -> bool { | Tok::Newline | Tok::NonLogicalNewline ) { - println!("{:?}", tok); return false; } } - println!("{:?}", code); true } + +/// Extract the indentation from a given line +pub fn extract_indentation(line: &str, stylist: &Stylist) -> String { + line.chars() + .take_while(|c| stylist.indentation().contains(*c)) + .collect() +} diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index c61f2a1f815f6..8ab5d28524e00 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -2,7 +2,7 @@ use itertools::Itertools; use rustpython_ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind}; use super::branch::Branch; -use super::helpers::{code_is_only_comments, result_exists}; +use super::helpers::{code_is_only_comments, extract_indentation, result_exists}; use super::visitor::{ReturnVisitor, Stack}; use crate::ast::helpers::{elif_else_range, unparse_expr}; use crate::ast::types::Range; @@ -350,7 +350,7 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { expr.location, )) .to_owned(); - // Ensure that all indentation is the same (no if or while statement). + // Ensure that all indentation is identical (no if or while statement). let all_indentation_same = (checker .locator .slice_source_code_range(&Range::new( @@ -360,24 +360,19 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { .chars() .take_while(|c| checker.stylist.indentation().contains(*c)) .join("") - .to_owned() + &in_between_source) .lines() .tuple_windows() - .any(|ln: (&str, &str)| { - ln.0.chars() - .take_while(|c| checker.stylist.indentation().contains(*c)) - .collect::() - == ln - .1 - .chars() - .take_while(|c| checker.stylist.indentation().contains(*c)) - .collect::() + .all(|(ln1, ln2): (&str, &str)| { + extract_indentation(ln1, checker.stylist) + == extract_indentation(ln2, checker.stylist) }); // Check if the code without the final return statement is only comments - if all_indentation_same && code_is_only_comments( - in_between_source.trim_end().trim_end_matches("return"), - ) { + if all_indentation_same + && code_is_only_comments( + in_between_source.trim_end().trim_end_matches("return"), + ) + { // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage let fixed_source = match in_between_source .strip_prefix(checker.stylist.line_ending().as_str()) diff --git a/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap b/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap index 0f515cf10de49..16e664bafab6d 100644 --- a/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap +++ b/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET504_RET504.py.snap @@ -46,51 +46,58 @@ expression: diagnostics end_location: row: 19 column: 16 + fix: ~ + parent: ~ +- kind: + UnnecessaryAssign: ~ + location: + row: 33 + column: 11 + end_location: + row: 33 + column: 14 fix: content: - - "if True:" - - " return 2" + - "\"\"\"" + - " This is a" + - " block comment" + - " we want to keep" + - " \"\"\"" + - " # This is a comment that we want to keep." + - " return [5, 4, 3, 2, 1]" location: - row: 17 + row: 26 column: 4 end_location: - row: 19 - column: 16 + row: 33 + column: 14 parent: ~ - kind: UnnecessaryAssign: ~ location: - row: 31 + row: 43 column: 11 end_location: - row: 31 + row: 43 column: 17 - fix: - content: - - return str(obj.bar) - location: - row: 30 - column: 8 - end_location: - row: 31 - column: 17 + fix: ~ parent: ~ - kind: UnnecessaryAssign: ~ location: - row: 39 + row: 51 column: 11 end_location: - row: 39 + row: 51 column: 20 fix: content: - "return formatted.replace(\"()\", \"\").replace(\" \", \" \").strip()" location: - row: 38 + row: 50 column: 4 end_location: - row: 39 + row: 51 column: 20 parent: ~ From 3ffdab9a0647383bf75c8e70b7147b90c20a9f55 Mon Sep 17 00:00:00 2001 From: Sawbez Date: Tue, 14 Feb 2023 22:05:41 -0800 Subject: [PATCH 8/8] Small improvements. --- crates/ruff/src/rules/flake8_return/rules.rs | 113 +++++++++--------- .../ruff/src/rules/flake8_return/visitor.rs | 7 +- 2 files changed, 59 insertions(+), 61 deletions(-) diff --git a/crates/ruff/src/rules/flake8_return/rules.rs b/crates/ruff/src/rules/flake8_return/rules.rs index e3eecc28f84a8..33f51ddce0e91 100644 --- a/crates/ruff/src/rules/flake8_return/rules.rs +++ b/crates/ruff/src/rules/flake8_return/rules.rs @@ -372,67 +372,66 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { let mut diagnostic = Diagnostic::new(UnnecessaryAssign, Range::from_located(expr)); if checker.patch(diagnostic.kind.rule()) { + // assign_expr is the Expr referring to the value of the last assignment (e.g. `5+5` for `var = 5+5`) + // TODO: This might cause a problem if there is an assignment after, needs further testing if let Some(assign_expr) = stack.assign_values.get(id.as_str()) { - if let Some(assign_locations) = stack.assigns.get(id.as_str()) { - let expr_loc = expr.end_location.unwrap(); - if let Some(last_assign_loc) = assign_locations - .iter() - .rev() - .find(|x| x.location < expr_loc) + // assign_locations is a &Vec that has each range for each whole assignment (`var = 5+5`) + let assign_locations = stack.assigns.get(id.as_str()).unwrap(); + let expr_loc = expr.end_location.unwrap(); + if let Some(last_assign_loc) = assign_locations + .iter() + .rev() + .find(|x| x.location < expr_loc) + { + let in_between_source = checker + .locator + .slice(&Range::new(last_assign_loc.end_location, expr.location)) + .to_owned(); + // Ensure that all indentation is identical (no if or while statement). + let all_indentation_same = (checker + .locator + .slice(&Range::new( + Location::new(last_assign_loc.location.row(), 0), + Location::new(last_assign_loc.location.row() + 1, 0), + )) + .chars() + .take_while(|c| checker.stylist.indentation().contains(*c)) + .join("") + + &in_between_source) + .lines() + .tuple_windows() + .all(|(ln1, ln2): (&str, &str)| { + extract_indentation(ln1, checker.stylist) + == extract_indentation(ln2, checker.stylist) + }); + // Check if the code without the final return statement is only comments + if all_indentation_same + && code_is_only_comments( + in_between_source.trim_end().trim_end_matches("return"), + ) { - let in_between_source = checker - .locator - .slice_source_code_range(&Range::new( - last_assign_loc.end_location, - expr.location, - )) - .to_owned(); - // Ensure that all indentation is identical (no if or while statement). - let all_indentation_same = (checker - .locator - .slice_source_code_range(&Range::new( - Location::new(last_assign_loc.location.row(), 0), - Location::new(last_assign_loc.location.row() + 1, 0), - )) - .chars() - .take_while(|c| checker.stylist.indentation().contains(*c)) - .join("") - + &in_between_source) - .lines() - .tuple_windows() - .all(|(ln1, ln2): (&str, &str)| { - extract_indentation(ln1, checker.stylist) - == extract_indentation(ln2, checker.stylist) - }); - // Check if the code without the final return statement is only comments - if all_indentation_same - && code_is_only_comments( - in_between_source.trim_end().trim_end_matches("return"), - ) + // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage + let fixed_source = match in_between_source + .strip_prefix(checker.stylist.line_ending().as_str()) { - // Avoid a trailing empty line and avoid causing invalid syntax with semicolon usage - let fixed_source = match in_between_source - .strip_prefix(checker.stylist.line_ending().as_str()) - { - Some(newline_stripped) => { - match newline_stripped - .strip_prefix(checker.stylist.indentation().as_str()) - { - Some(unindented) => unindented.to_owned(), - None => in_between_source, - } - } - None => match in_between_source.strip_prefix(';') { - Some(no_semicolon) => no_semicolon.to_owned(), + Some(newline_stripped) => { + match newline_stripped + .strip_prefix(checker.stylist.indentation().as_str()) + { + Some(unindented) => unindented.to_owned(), None => in_between_source, - }, - }; - diagnostic.amend(Fix::replacement( - fixed_source + &unparse_expr(&assign_expr.clone(), checker.stylist), - last_assign_loc.location, - expr.end_location.unwrap(), - )); - } + } + } + None => match in_between_source.strip_prefix(';') { + Some(no_semicolon) => no_semicolon.to_owned(), + None => in_between_source, + }, + }; + diagnostic.amend(Fix::replacement( + fixed_source + &unparse_expr(assign_expr, checker.stylist), + last_assign_loc.location, + expr.end_location.unwrap(), + )); } } } diff --git a/crates/ruff/src/rules/flake8_return/visitor.rs b/crates/ruff/src/rules/flake8_return/visitor.rs index 916b097a810e0..0f71886bd3157 100644 --- a/crates/ruff/src/rules/flake8_return/visitor.rs +++ b/crates/ruff/src/rules/flake8_return/visitor.rs @@ -16,7 +16,7 @@ pub struct Stack<'a> { pub assigns: FxHashMap<&'a str, Vec>, pub loops: Vec<(Location, Location)>, pub tries: Vec<(Location, Location)>, - pub assign_values: FxHashMap<&'a str, Expr>, + pub assign_values: FxHashMap<&'a str, &'a Expr>, } #[derive(Default)] @@ -118,10 +118,9 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { return; } - // TODO: Make this only run if the autofix is enabled, as this incurs a - // slight performance overhead by using `.clone` + // TODO: Make this only run if the autofix is enabled (or we could merge into the below statement) if let ExprKind::Name { id, .. } = &target.node { - self.stack.assign_values.insert(id, *value.clone()); + self.stack.assign_values.insert(id, value); } // This is hacky, but I don't kmow how to pass the "context" of the