From 4fb0c6e3adb39d9242ed955a51a6303936c7e7bc Mon Sep 17 00:00:00 2001 From: alm Date: Mon, 23 Jan 2023 00:08:57 +0200 Subject: [PATCH] feat: Implement TRY201 (#2073) --- README.md | 1 + resources/test/fixtures/tryceratops/TRY201.py | 54 +++++++++++++++ ruff.schema.json | 3 + src/checkers/ast.rs | 3 + src/registry.rs | 1 + src/rules/tryceratops/mod.rs | 1 + src/rules/tryceratops/rules/mod.rs | 2 + src/rules/tryceratops/rules/verbose_raise.rs | 68 +++++++++++++++++++ ...atops__tests__verbose-raise_TRY201.py.snap | 35 ++++++++++ 9 files changed, 168 insertions(+) create mode 100644 resources/test/fixtures/tryceratops/TRY201.py create mode 100644 src/rules/tryceratops/rules/verbose_raise.rs create mode 100644 src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap diff --git a/README.md b/README.md index dc2093d7b2..ee06f76b6b 100644 --- a/README.md +++ b/README.md @@ -1196,6 +1196,7 @@ For more, see [tryceratops](https://pypi.org/project/tryceratops/1.1.0/) on PyPI | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | | TRY004 | prefer-type-error | Prefer `TypeError` exception for invalid type | 🛠 | +| TRY201 | verbose-raise | Use `raise` without specifying exception name | | | TRY300 | try-consider-else | Consider `else` block | | ### flake8-use-pathlib (PTH) diff --git a/resources/test/fixtures/tryceratops/TRY201.py b/resources/test/fixtures/tryceratops/TRY201.py new file mode 100644 index 0000000000..8b9ef4913d --- /dev/null +++ b/resources/test/fixtures/tryceratops/TRY201.py @@ -0,0 +1,54 @@ +""" +Violation: + +Raising an exception using its assigned name is verbose and unrequired +""" +import logging + +logger = logging.getLogger(__name__) + + +class MyException(Exception): + pass + + +def bad(): + try: + process() + except MyException as e: + logger.exception("process failed") + raise e + + +def good(): + try: + process() + except MyException: + logger.exception("process failed") + raise + +def still_good(): + try: + process() + except MyException as e: + print(e) + raise + + +def bad_that_needs_recursion(): + try: + process() + except MyException as e: + logger.exception("process failed") + if True: + raise e + + +def bad_that_needs_recursion_2(): + try: + process() + except MyException as e: + logger.exception("process failed") + if True: + def foo(): + raise e diff --git a/ruff.schema.json b/ruff.schema.json index 792a4096ad..bda3ed907d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1782,6 +1782,9 @@ "TRY0", "TRY00", "TRY004", + "TRY2", + "TRY20", + "TRY201", "TRY3", "TRY30", "TRY300", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 94c5045e94..c31e677d51 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1578,6 +1578,9 @@ where if self.settings.rules.enabled(&Rule::TryConsiderElse) { tryceratops::rules::try_consider_else(self, body, orelse); } + if self.settings.rules.enabled(&Rule::VerboseRaise) { + tryceratops::rules::verbose_raise(self, handlers); + } } StmtKind::Assign { targets, value, .. } => { if self.settings.rules.enabled(&Rule::DoNotAssignLambda) { diff --git a/src/registry.rs b/src/registry.rs index 4d66462b28..87bb072107 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -428,6 +428,7 @@ ruff_macros::define_rule_mapping!( TYP005 => rules::flake8_type_checking::rules::EmptyTypeCheckingBlock, // tryceratops TRY004 => rules::tryceratops::rules::PreferTypeError, + TRY201 => rules::tryceratops::rules::VerboseRaise, TRY300 => rules::tryceratops::rules::TryConsiderElse, // flake8-use-pathlib PTH100 => rules::flake8_use_pathlib::violations::PathlibAbspath, diff --git a/src/rules/tryceratops/mod.rs b/src/rules/tryceratops/mod.rs index 9d2738ff0a..e07e945ca0 100644 --- a/src/rules/tryceratops/mod.rs +++ b/src/rules/tryceratops/mod.rs @@ -14,6 +14,7 @@ mod tests { use crate::settings; #[test_case(Rule::PreferTypeError, Path::new("TRY004.py"); "TRY004")] + #[test_case(Rule::VerboseRaise, Path::new("TRY201.py"); "TRY201")] #[test_case(Rule::TryConsiderElse, Path::new("TRY300.py"); "TRY300")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); diff --git a/src/rules/tryceratops/rules/mod.rs b/src/rules/tryceratops/rules/mod.rs index 5fd6a64cdf..fad3dfa6f5 100644 --- a/src/rules/tryceratops/rules/mod.rs +++ b/src/rules/tryceratops/rules/mod.rs @@ -1,5 +1,7 @@ pub use prefer_type_error::{prefer_type_error, PreferTypeError}; pub use try_consider_else::{try_consider_else, TryConsiderElse}; +pub use verbose_raise::{verbose_raise, VerboseRaise}; mod prefer_type_error; mod try_consider_else; +mod verbose_raise; diff --git a/src/rules/tryceratops/rules/verbose_raise.rs b/src/rules/tryceratops/rules/verbose_raise.rs new file mode 100644 index 0000000000..1130e974ec --- /dev/null +++ b/src/rules/tryceratops/rules/verbose_raise.rs @@ -0,0 +1,68 @@ +use ruff_macros::derive_message_formats; +use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind, Stmt, StmtKind}; + +use crate::ast::types::Range; +use crate::ast::visitor; +use crate::ast::visitor::Visitor; +use crate::checkers::ast::Checker; +use crate::define_violation; +use crate::registry::Diagnostic; +use crate::violation::Violation; + +define_violation!( + pub struct VerboseRaise; +); +impl Violation for VerboseRaise { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use `raise` without specifying exception name") + } +} + +#[derive(Default)] +struct RaiseStatementVisitor<'a> { + raises: Vec>, +} + +impl<'a, 'b> Visitor<'b> for RaiseStatementVisitor<'a> +where + 'b: 'a, +{ + fn visit_stmt(&mut self, stmt: &'b Stmt) { + match &stmt.node { + StmtKind::Raise { exc, .. } => self.raises.push(exc.as_ref().map(|expr| &**expr)), + _ => visitor::walk_stmt(self, stmt), + } + } +} + +/// TRY201 +pub fn verbose_raise(checker: &mut Checker, handlers: &[Excepthandler]) { + for handler in handlers { + // If the handler assigned a name to the exception... + if let ExcepthandlerKind::ExceptHandler { + name: Some(exception_name), + body, + .. + } = &handler.node + { + let raises = { + let mut visitor = RaiseStatementVisitor::default(); + for stmt in body { + visitor.visit_stmt(stmt); + } + visitor.raises + }; + for expr in raises.into_iter().flatten() { + // ...and the raised object is bound to the same name... + if let ExprKind::Name { id, .. } = &expr.node { + if id == exception_name { + checker + .diagnostics + .push(Diagnostic::new(VerboseRaise, Range::from_located(expr))); + } + } + } + } + } +} diff --git a/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap b/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap new file mode 100644 index 0000000000..3eca36ed56 --- /dev/null +++ b/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap @@ -0,0 +1,35 @@ +--- +source: src/rules/tryceratops/mod.rs +expression: diagnostics +--- +- kind: + VerboseRaise: ~ + location: + row: 20 + column: 14 + end_location: + row: 20 + column: 15 + fix: ~ + parent: ~ +- kind: + VerboseRaise: ~ + location: + row: 44 + column: 18 + end_location: + row: 44 + column: 19 + fix: ~ + parent: ~ +- kind: + VerboseRaise: ~ + location: + row: 54 + column: 22 + end_location: + row: 54 + column: 23 + fix: ~ + parent: ~ +