From ad72d0c9fc08f9e231e86375192097bad58f2140 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sun, 23 Jul 2023 12:05:06 +0530 Subject: [PATCH 1/5] Make `TRY201` always autofixable --- .../rules/tryceratops/rules/verbose_raise.rs | 29 +++++++++-- ...atops__tests__verbose-raise_TRY201.py.snap | 49 +++++++++++++++++-- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs index c2829ca70742b..3ad07a1ee352e 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs @@ -1,10 +1,12 @@ +use ruff_text_size::TextSize; use rustpython_parser::ast::{self, ExceptHandler, Expr, Ranged, Stmt}; -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor}; use crate::checkers::ast::Checker; +use crate::registry::AsRule; /// ## What it does /// Checks for needless exception names in `raise` statements. @@ -33,11 +35,15 @@ use crate::checkers::ast::Checker; #[violation] pub struct VerboseRaise; -impl Violation for VerboseRaise { +impl AlwaysAutofixableViolation for VerboseRaise { #[derive_message_formats] fn message(&self) -> String { format!("Use `raise` without specifying exception name") } + + fn autofix_title(&self) -> String { + format!("Remove exception name") + } } #[derive(Default)] @@ -78,6 +84,7 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { for handler in handlers { // If the handler assigned a name to the exception... if let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + type_: Some(expr), name: Some(exception_name), body, .. @@ -96,9 +103,21 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { // ...and the raised object is bound to the same name... if let Expr::Name(ast::ExprName { id, .. }) = exc { if id == exception_name.as_str() { - checker - .diagnostics - .push(Diagnostic::new(VerboseRaise, exc.range())); + let mut diagnostic = Diagnostic::new(VerboseRaise, exc.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::automatic_edits( + Edit::deletion( + expr.range().end(), + exception_name.range().end(), + ), + [Edit::range_deletion( + // SAFETY: No space between `raise` and exception name + // is a syntax error. + exc.range().sub_start(TextSize::new(1)), + )], + )); + } + checker.diagnostics.push(diagnostic); } } } diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap index 302ffa8a66011..e74bff33d8910 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap @@ -1,27 +1,70 @@ --- source: crates/ruff/src/rules/tryceratops/mod.rs --- -TRY201.py:20:15: TRY201 Use `raise` without specifying exception name +TRY201.py:20:15: TRY201 [*] Use `raise` without specifying exception name | 18 | except MyException as e: 19 | logger.exception("process failed") 20 | raise e | ^ TRY201 | + = help: Remove exception name -TRY201.py:63:19: TRY201 Use `raise` without specifying exception name +ℹ Fix +15 15 | def bad(): +16 16 | try: +17 17 | process() +18 |- except MyException as e: + 18 |+ except MyException: +19 19 | logger.exception("process failed") +20 |- raise e + 20 |+ raise +21 21 | +22 22 | +23 23 | def good(): + +TRY201.py:63:19: TRY201 [*] Use `raise` without specifying exception name | 61 | logger.exception("process failed") 62 | if True: 63 | raise e | ^ TRY201 | + = help: Remove exception name + +ℹ Fix +57 57 | def bad_that_needs_recursion(): +58 58 | try: +59 59 | process() +60 |- except MyException as e: + 60 |+ except MyException: +61 61 | logger.exception("process failed") +62 62 | if True: +63 |- raise e + 63 |+ raise +64 64 | +65 65 | +66 66 | def bad_that_needs_recursion_2(): -TRY201.py:74:23: TRY201 Use `raise` without specifying exception name +TRY201.py:74:23: TRY201 [*] Use `raise` without specifying exception name | 73 | def foo(): 74 | raise e | ^ TRY201 | + = help: Remove exception name + +ℹ Fix +66 66 | def bad_that_needs_recursion_2(): +67 67 | try: +68 68 | process() +69 |- except MyException as e: + 69 |+ except MyException: +70 70 | logger.exception("process failed") +71 71 | if True: +72 72 | +73 73 | def foo(): +74 |- raise e + 74 |+ raise From 7ca46c173df628efc9c5ac7738f00cd98f1d0693 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sun, 23 Jul 2023 12:34:46 +0530 Subject: [PATCH 2/5] Generate an empty `raise` stmt for the fix --- .../rules/tryceratops/rules/verbose_raise.rs | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs index 3ad07a1ee352e..5b3651af2eb8f 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs @@ -1,4 +1,4 @@ -use ruff_text_size::TextSize; +use ruff_text_size::TextRange; use rustpython_parser::ast::{self, ExceptHandler, Expr, Ranged, Stmt}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; @@ -48,7 +48,7 @@ impl AlwaysAutofixableViolation for VerboseRaise { #[derive(Default)] struct RaiseStatementVisitor<'a> { - raises: Vec<(Option<&'a Expr>, Option<&'a Expr>)>, + raises: Vec<&'a ast::StmtRaise>, } impl<'a, 'b> StatementVisitor<'b> for RaiseStatementVisitor<'a> @@ -57,12 +57,8 @@ where { fn visit_stmt(&mut self, stmt: &'b Stmt) { match stmt { - Stmt::Raise(ast::StmtRaise { - exc, - cause, - range: _, - }) => { - self.raises.push((exc.as_deref(), cause.as_deref())); + Stmt::Raise(raise @ ast::StmtRaise { .. }) => { + self.raises.push(raise); } Stmt::Try(ast::StmtTry { body, finalbody, .. @@ -95,13 +91,13 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { visitor.visit_body(body); visitor.raises }; - for (exc, cause) in raises { - if cause.is_some() { + for raise in raises { + if raise.cause.is_some() { continue; } - if let Some(exc) = exc { + if let Some(exc) = raise.exc.as_ref() { // ...and the raised object is bound to the same name... - if let Expr::Name(ast::ExprName { id, .. }) = exc { + if let Expr::Name(ast::ExprName { id, .. }) = exc.as_ref() { if id == exception_name.as_str() { let mut diagnostic = Diagnostic::new(VerboseRaise, exc.range()); if checker.patch(diagnostic.kind.rule()) { @@ -110,10 +106,13 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { expr.range().end(), exception_name.range().end(), ), - [Edit::range_deletion( - // SAFETY: No space between `raise` and exception name - // is a syntax error. - exc.range().sub_start(TextSize::new(1)), + [Edit::range_replacement( + checker.generator().stmt(&Stmt::Raise(ast::StmtRaise { + range: TextRange::default(), + exc: None, + cause: None, + })), + raise.range(), )], )); } From 956fa4e1e64cb0ca92a2db1e6568b3228d42d7fe Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sun, 23 Jul 2023 23:28:27 +0530 Subject: [PATCH 3/5] Simplify edit, make it suggested --- .../src/rules/tryceratops/rules/verbose_raise.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs index 5b3651af2eb8f..705f2f17a80ed 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs @@ -1,4 +1,3 @@ -use ruff_text_size::TextRange; use rustpython_parser::ast::{self, ExceptHandler, Expr, Ranged, Stmt}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; @@ -101,19 +100,12 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { if id == exception_name.as_str() { let mut diagnostic = Diagnostic::new(VerboseRaise, exc.range()); if checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(Fix::automatic_edits( + diagnostic.set_fix(Fix::suggested_edits( Edit::deletion( expr.range().end(), exception_name.range().end(), ), - [Edit::range_replacement( - checker.generator().stmt(&Stmt::Raise(ast::StmtRaise { - range: TextRange::default(), - exc: None, - cause: None, - })), - raise.range(), - )], + [Edit::range_replacement("raise".to_string(), raise.range())], )); } checker.diagnostics.push(diagnostic); From ffe8f1309695cc83abe0f3c377b8d70d790edcc9 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sun, 23 Jul 2023 23:30:48 +0530 Subject: [PATCH 4/5] Update snapshots --- ..._rules__tryceratops__tests__verbose-raise_TRY201.py.snap | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap index e74bff33d8910..657e36b472c78 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap @@ -10,7 +10,7 @@ TRY201.py:20:15: TRY201 [*] Use `raise` without specifying exception name | = help: Remove exception name -ℹ Fix +ℹ Suggested fix 15 15 | def bad(): 16 16 | try: 17 17 | process() @@ -32,7 +32,7 @@ TRY201.py:63:19: TRY201 [*] Use `raise` without specifying exception name | = help: Remove exception name -ℹ Fix +ℹ Suggested fix 57 57 | def bad_that_needs_recursion(): 58 58 | try: 59 59 | process() @@ -54,7 +54,7 @@ TRY201.py:74:23: TRY201 [*] Use `raise` without specifying exception name | = help: Remove exception name -ℹ Fix +ℹ Suggested fix 66 66 | def bad_that_needs_recursion_2(): 67 67 | try: 68 68 | process() From 7f60ef07085c416628a95bc63d951926439608da Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 24 Jul 2023 07:44:17 +0530 Subject: [PATCH 5/5] Simplify fix as unused variable will be removed automatically --- .../rules/tryceratops/rules/verbose_raise.rs | 12 ++++-------- ...eratops__tests__verbose-raise_TRY201.py.snap | 17 ++--------------- 2 files changed, 6 insertions(+), 23 deletions(-) diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs index 705f2f17a80ed..0ffc6685989e2 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs @@ -79,7 +79,6 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { for handler in handlers { // If the handler assigned a name to the exception... if let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { - type_: Some(expr), name: Some(exception_name), body, .. @@ -100,13 +99,10 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { if id == exception_name.as_str() { let mut diagnostic = Diagnostic::new(VerboseRaise, exc.range()); if checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(Fix::suggested_edits( - Edit::deletion( - expr.range().end(), - exception_name.range().end(), - ), - [Edit::range_replacement("raise".to_string(), raise.range())], - )); + diagnostic.set_fix(Fix::suggested(Edit::range_replacement( + "raise".to_string(), + raise.range(), + ))); } checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap index 657e36b472c78..9ee76c288ee88 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap @@ -11,11 +11,8 @@ TRY201.py:20:15: TRY201 [*] Use `raise` without specifying exception name = help: Remove exception name ℹ Suggested fix -15 15 | def bad(): -16 16 | try: 17 17 | process() -18 |- except MyException as e: - 18 |+ except MyException: +18 18 | except MyException as e: 19 19 | logger.exception("process failed") 20 |- raise e 20 |+ raise @@ -33,11 +30,7 @@ TRY201.py:63:19: TRY201 [*] Use `raise` without specifying exception name = help: Remove exception name ℹ Suggested fix -57 57 | def bad_that_needs_recursion(): -58 58 | try: -59 59 | process() -60 |- except MyException as e: - 60 |+ except MyException: +60 60 | except MyException as e: 61 61 | logger.exception("process failed") 62 62 | if True: 63 |- raise e @@ -55,12 +48,6 @@ TRY201.py:74:23: TRY201 [*] Use `raise` without specifying exception name = help: Remove exception name ℹ Suggested fix -66 66 | def bad_that_needs_recursion_2(): -67 67 | try: -68 68 | process() -69 |- except MyException as e: - 69 |+ except MyException: -70 70 | logger.exception("process failed") 71 71 | if True: 72 72 | 73 73 | def foo():