From c3e0137f227ffd67d6f616e91198da11562b53d0 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Thu, 2 Feb 2023 22:13:49 +0200 Subject: [PATCH] Move flake8-return violations to rules module (#2492) --- src/registry.rs | 16 ++-- src/rules/flake8_return/branch.rs | 18 ++++ src/rules/flake8_return/mod.rs | 1 + src/rules/flake8_return/rules.rs | 135 ++++++++++++++++++++++++++---- src/violations.rs | 123 --------------------------- 5 files changed, 147 insertions(+), 146 deletions(-) create mode 100644 src/rules/flake8_return/branch.rs diff --git a/src/registry.rs b/src/registry.rs index fbb8dc9170..9654271a80 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -154,14 +154,14 @@ ruff_macros::define_rule_mapping!( TID251 => rules::flake8_tidy_imports::banned_api::BannedApi, TID252 => rules::flake8_tidy_imports::relative_imports::RelativeImports, // flake8-return - RET501 => violations::UnnecessaryReturnNone, - RET502 => violations::ImplicitReturnValue, - RET503 => violations::ImplicitReturn, - RET504 => violations::UnnecessaryAssign, - RET505 => violations::SuperfluousElseReturn, - RET506 => violations::SuperfluousElseRaise, - RET507 => violations::SuperfluousElseContinue, - RET508 => violations::SuperfluousElseBreak, + RET501 => rules::flake8_return::rules::UnnecessaryReturnNone, + RET502 => rules::flake8_return::rules::ImplicitReturnValue, + RET503 => rules::flake8_return::rules::ImplicitReturn, + RET504 => rules::flake8_return::rules::UnnecessaryAssign, + RET505 => rules::flake8_return::rules::SuperfluousElseReturn, + RET506 => rules::flake8_return::rules::SuperfluousElseRaise, + RET507 => rules::flake8_return::rules::SuperfluousElseContinue, + RET508 => rules::flake8_return::rules::SuperfluousElseBreak, // flake8-implicit-str-concat ISC001 => violations::SingleLineImplicitStringConcatenation, ISC002 => violations::MultiLineImplicitStringConcatenation, diff --git a/src/rules/flake8_return/branch.rs b/src/rules/flake8_return/branch.rs new file mode 100644 index 0000000000..22866ee8eb --- /dev/null +++ b/src/rules/flake8_return/branch.rs @@ -0,0 +1,18 @@ +use std::fmt; + +use serde::{Deserialize, Serialize}; + +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] +pub enum Branch { + Elif, + Else, +} + +impl fmt::Display for Branch { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + Branch::Elif => fmt.write_str("elif"), + Branch::Else => fmt.write_str("else"), + } + } +} diff --git a/src/rules/flake8_return/mod.rs b/src/rules/flake8_return/mod.rs index cd743055da..1d0d26ed18 100644 --- a/src/rules/flake8_return/mod.rs +++ b/src/rules/flake8_return/mod.rs @@ -1,4 +1,5 @@ //! Rules from [flake8-return](https://pypi.org/project/flake8-return/). +mod branch; mod helpers; pub(crate) mod rules; mod visitor; diff --git a/src/rules/flake8_return/rules.rs b/src/rules/flake8_return/rules.rs index affcbab8d6..7ec1203ba6 100644 --- a/src/rules/flake8_return/rules.rs +++ b/src/rules/flake8_return/rules.rs @@ -1,6 +1,7 @@ use itertools::Itertools; 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; @@ -8,10 +9,117 @@ use crate::ast::types::Range; use crate::ast::visitor::Visitor; use crate::ast::whitespace::indentation; use crate::checkers::ast::Checker; +use crate::define_violation; use crate::fix::Fix; use crate::registry::{Diagnostic, Rule}; -use crate::violations; -use crate::violations::Branch; +use crate::violation::{AlwaysAutofixableViolation, Violation}; +use ruff_macros::derive_message_formats; + +define_violation!( + pub struct UnnecessaryReturnNone; +); +impl AlwaysAutofixableViolation for UnnecessaryReturnNone { + #[derive_message_formats] + fn message(&self) -> String { + format!( + "Do not explicitly `return None` in function if it is the only possible return value" + ) + } + + fn autofix_title(&self) -> String { + "Remove explicit `return None`".to_string() + } +} + +define_violation!( + pub struct ImplicitReturnValue; +); +impl AlwaysAutofixableViolation for ImplicitReturnValue { + #[derive_message_formats] + fn message(&self) -> String { + format!("Do not implicitly `return None` in function able to return non-`None` value") + } + + fn autofix_title(&self) -> String { + "Add explicit `None` return value".to_string() + } +} + +define_violation!( + pub struct ImplicitReturn; +); +impl AlwaysAutofixableViolation for ImplicitReturn { + #[derive_message_formats] + fn message(&self) -> String { + format!("Missing explicit `return` at the end of function able to return non-`None` value") + } + + fn autofix_title(&self) -> String { + "Add explicit `return` statement".to_string() + } +} + +define_violation!( + pub struct UnnecessaryAssign; +); +impl Violation for UnnecessaryAssign { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unnecessary variable assignment before `return` statement") + } +} + +define_violation!( + pub struct SuperfluousElseReturn { + pub branch: Branch, + } +); +impl Violation for SuperfluousElseReturn { + #[derive_message_formats] + fn message(&self) -> String { + let SuperfluousElseReturn { branch } = self; + format!("Unnecessary `{branch}` after `return` statement") + } +} + +define_violation!( + pub struct SuperfluousElseRaise { + pub branch: Branch, + } +); +impl Violation for SuperfluousElseRaise { + #[derive_message_formats] + fn message(&self) -> String { + let SuperfluousElseRaise { branch } = self; + format!("Unnecessary `{branch}` after `raise` statement") + } +} + +define_violation!( + pub struct SuperfluousElseContinue { + pub branch: Branch, + } +); +impl Violation for SuperfluousElseContinue { + #[derive_message_formats] + fn message(&self) -> String { + let SuperfluousElseContinue { branch } = self; + format!("Unnecessary `{branch}` after `continue` statement") + } +} + +define_violation!( + pub struct SuperfluousElseBreak { + pub branch: Branch, + } +); +impl Violation for SuperfluousElseBreak { + #[derive_message_formats] + fn message(&self) -> String { + let SuperfluousElseBreak { branch } = self; + format!("Unnecessary `{branch}` after `break` statement") + } +} /// RET501 fn unnecessary_return_none(checker: &mut Checker, stack: &Stack) { @@ -28,8 +136,7 @@ fn unnecessary_return_none(checker: &mut Checker, stack: &Stack) { ) { continue; } - let mut diagnostic = - Diagnostic::new(violations::UnnecessaryReturnNone, Range::from_located(stmt)); + let mut diagnostic = Diagnostic::new(UnnecessaryReturnNone, Range::from_located(stmt)); if checker.patch(diagnostic.kind.rule()) { diagnostic.amend(Fix::replacement( "return".to_string(), @@ -47,8 +154,7 @@ fn implicit_return_value(checker: &mut Checker, stack: &Stack) { if expr.is_some() { continue; } - let mut diagnostic = - Diagnostic::new(violations::ImplicitReturnValue, Range::from_located(stmt)); + let mut diagnostic = Diagnostic::new(ImplicitReturnValue, Range::from_located(stmt)); if checker.patch(diagnostic.kind.rule()) { diagnostic.amend(Fix::replacement( "return None".to_string(), @@ -66,7 +172,7 @@ fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) { StmtKind::If { body, orelse, .. } => { if body.is_empty() || orelse.is_empty() { checker.diagnostics.push(Diagnostic::new( - violations::ImplicitReturn, + ImplicitReturn, Range::from_located(last_stmt), )); return; @@ -104,8 +210,7 @@ fn implicit_return(checker: &mut Checker, last_stmt: &Stmt) { | StmtKind::Raise { .. } | StmtKind::Try { .. } => {} _ => { - let mut diagnostic = - Diagnostic::new(violations::ImplicitReturn, Range::from_located(last_stmt)); + let mut diagnostic = Diagnostic::new(ImplicitReturn, Range::from_located(last_stmt)); if checker.patch(diagnostic.kind.rule()) { if let Some(indent) = indentation(checker.locator, last_stmt) { let mut content = String::new(); @@ -198,7 +303,7 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { if !stack.refs.contains_key(id.as_str()) { checker.diagnostics.push(Diagnostic::new( - violations::UnnecessaryAssign, + UnnecessaryAssign, Range::from_located(expr), )); return; @@ -215,7 +320,7 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack, expr: &Expr) { } checker.diagnostics.push(Diagnostic::new( - violations::UnnecessaryAssign, + UnnecessaryAssign, Range::from_located(expr), )); } @@ -229,7 +334,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &Stmt, branch: Branch) -> for child in body { if matches!(child.node, StmtKind::Return { .. }) { let diagnostic = Diagnostic::new( - violations::SuperfluousElseReturn { branch }, + SuperfluousElseReturn { branch }, elif_else_range(stmt, checker.locator).unwrap_or_else(|| Range::from_located(stmt)), ); if checker.settings.rules.enabled(diagnostic.kind.rule()) { @@ -239,7 +344,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &Stmt, branch: Branch) -> } if matches!(child.node, StmtKind::Break) { let diagnostic = Diagnostic::new( - violations::SuperfluousElseBreak { branch }, + SuperfluousElseBreak { branch }, elif_else_range(stmt, checker.locator).unwrap_or_else(|| Range::from_located(stmt)), ); if checker.settings.rules.enabled(diagnostic.kind.rule()) { @@ -249,7 +354,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &Stmt, branch: Branch) -> } if matches!(child.node, StmtKind::Raise { .. }) { let diagnostic = Diagnostic::new( - violations::SuperfluousElseRaise { branch }, + SuperfluousElseRaise { branch }, elif_else_range(stmt, checker.locator).unwrap_or_else(|| Range::from_located(stmt)), ); if checker.settings.rules.enabled(diagnostic.kind.rule()) { @@ -259,7 +364,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &Stmt, branch: Branch) -> } if matches!(child.node, StmtKind::Continue) { let diagnostic = Diagnostic::new( - violations::SuperfluousElseContinue { branch }, + SuperfluousElseContinue { branch }, elif_else_range(stmt, checker.locator).unwrap_or_else(|| Range::from_located(stmt)), ); if checker.settings.rules.enabled(diagnostic.kind.rule()) { diff --git a/src/violations.rs b/src/violations.rs index 1699315982..430439218f 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -45,129 +45,6 @@ impl Violation for FunctionIsTooComplex { } } -// flake8-return - -define_violation!( - pub struct UnnecessaryReturnNone; -); -impl AlwaysAutofixableViolation for UnnecessaryReturnNone { - #[derive_message_formats] - fn message(&self) -> String { - format!( - "Do not explicitly `return None` in function if it is the only possible return value" - ) - } - - fn autofix_title(&self) -> String { - "Remove explicit `return None`".to_string() - } -} - -define_violation!( - pub struct ImplicitReturnValue; -); -impl AlwaysAutofixableViolation for ImplicitReturnValue { - #[derive_message_formats] - fn message(&self) -> String { - format!("Do not implicitly `return None` in function able to return non-`None` value") - } - - fn autofix_title(&self) -> String { - "Add explicit `None` return value".to_string() - } -} - -define_violation!( - pub struct ImplicitReturn; -); -impl AlwaysAutofixableViolation for ImplicitReturn { - #[derive_message_formats] - fn message(&self) -> String { - format!("Missing explicit `return` at the end of function able to return non-`None` value") - } - - fn autofix_title(&self) -> String { - "Add explicit `return` statement".to_string() - } -} - -define_violation!( - pub struct UnnecessaryAssign; -); -impl Violation for UnnecessaryAssign { - #[derive_message_formats] - fn message(&self) -> String { - format!("Unnecessary variable assignment before `return` statement") - } -} - -#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] -pub enum Branch { - Elif, - Else, -} - -impl fmt::Display for Branch { - fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - match self { - Branch::Elif => fmt.write_str("elif"), - Branch::Else => fmt.write_str("else"), - } - } -} - -define_violation!( - pub struct SuperfluousElseReturn { - pub branch: Branch, - } -); -impl Violation for SuperfluousElseReturn { - #[derive_message_formats] - fn message(&self) -> String { - let SuperfluousElseReturn { branch } = self; - format!("Unnecessary `{branch}` after `return` statement") - } -} - -define_violation!( - pub struct SuperfluousElseRaise { - pub branch: Branch, - } -); -impl Violation for SuperfluousElseRaise { - #[derive_message_formats] - fn message(&self) -> String { - let SuperfluousElseRaise { branch } = self; - format!("Unnecessary `{branch}` after `raise` statement") - } -} - -define_violation!( - pub struct SuperfluousElseContinue { - pub branch: Branch, - } -); -impl Violation for SuperfluousElseContinue { - #[derive_message_formats] - fn message(&self) -> String { - let SuperfluousElseContinue { branch } = self; - format!("Unnecessary `{branch}` after `continue` statement") - } -} - -define_violation!( - pub struct SuperfluousElseBreak { - pub branch: Branch, - } -); -impl Violation for SuperfluousElseBreak { - #[derive_message_formats] - fn message(&self) -> String { - let SuperfluousElseBreak { branch } = self; - format!("Unnecessary `{branch}` after `break` statement") - } -} - // flake8-implicit-str-concat define_violation!(