From 67d1f745872e8e3e805e508d0a9685637a6ea934 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 28 Feb 2023 11:56:29 -0500 Subject: [PATCH] Avoid raising TRY200 violations within new scopes (#3275) --- .../test/fixtures/tryceratops/TRY200.py | 8 +++ crates/ruff/src/ast/helpers.rs | 44 +++++++++++- .../rules/raise_without_from_inside_except.rs | 72 ++++++------------- .../tryceratops/rules/reraise_no_cause.rs | 42 +++-------- 4 files changed, 82 insertions(+), 84 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY200.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY200.py index 825b97ee51..4b00c3cef0 100644 --- a/crates/ruff/resources/test/fixtures/tryceratops/TRY200.py +++ b/crates/ruff/resources/test/fixtures/tryceratops/TRY200.py @@ -30,3 +30,11 @@ def good(): raise e # This is verbose violation, shouldn't trigger no cause except Exception: raise # Just re-raising don't need 'from' + + +def good(): + try: + from mod import f + except ImportError: + def f(): + raise MyException() # Raising within a new scope is fine diff --git a/crates/ruff/src/ast/helpers.rs b/crates/ruff/src/ast/helpers.rs index c8c906e58a..67b6b3f5df 100644 --- a/crates/ruff/src/ast/helpers.rs +++ b/crates/ruff/src/ast/helpers.rs @@ -765,7 +765,7 @@ pub fn from_relative_import<'a>(module: &'a [String], name: &'a str) -> CallPath call_path } -/// A [`Visitor`] that collects all return statements in a function or method. +/// A [`Visitor`] that collects all `return` statements in a function or method. #[derive(Default)] pub struct ReturnStatementVisitor<'a> { pub returns: Vec>, @@ -786,6 +786,48 @@ where } } +/// A [`Visitor`] that collects all `raise` statements in a function or method. +#[derive(Default)] +pub struct RaiseStatementVisitor<'a> { + pub raises: Vec<(Range, Option<&'a Expr>, Option<&'a Expr>)>, +} + +impl<'a, 'b> Visitor<'b> for RaiseStatementVisitor<'b> +where + 'b: 'a, +{ + fn visit_stmt(&mut self, stmt: &'b Stmt) { + match &stmt.node { + StmtKind::Raise { exc, cause } => { + self.raises + .push((Range::from_located(stmt), exc.as_deref(), cause.as_deref())); + } + StmtKind::ClassDef { .. } + | StmtKind::FunctionDef { .. } + | StmtKind::AsyncFunctionDef { .. } + | StmtKind::Try { .. } + | StmtKind::TryStar { .. } => {} + StmtKind::If { body, orelse, .. } => { + visitor::walk_body(self, body); + visitor::walk_body(self, orelse); + } + StmtKind::While { body, .. } + | StmtKind::With { body, .. } + | StmtKind::AsyncWith { body, .. } + | StmtKind::For { body, .. } + | StmtKind::AsyncFor { body, .. } => { + visitor::walk_body(self, body); + } + StmtKind::Match { cases, .. } => { + for case in cases { + visitor::walk_body(self, &case.body); + } + } + _ => {} + } + } +} + /// Convert a location within a file (relative to `base`) to an absolute /// position. pub fn to_absolute(relative: Location, base: Location) -> Location { diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/raise_without_from_inside_except.rs b/crates/ruff/src/rules/flake8_bugbear/rules/raise_without_from_inside_except.rs index 6dccf850ba..a4c0a28943 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/raise_without_from_inside_except.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/raise_without_from_inside_except.rs @@ -1,10 +1,10 @@ +use rustpython_parser::ast::{ExprKind, Stmt}; + use ruff_macros::{define_violation, derive_message_formats}; use ruff_python::str::is_lower; -use rustpython_parser::ast::{ExprKind, Stmt, StmtKind}; -use crate::ast::types::Range; +use crate::ast::helpers::RaiseStatementVisitor; use crate::ast::visitor; -use crate::ast::visitor::Visitor; use crate::checkers::ast::Checker; use crate::registry::Diagnostic; use crate::violation::Violation; @@ -16,62 +16,32 @@ impl Violation for RaiseWithoutFromInsideExcept { #[derive_message_formats] fn message(&self) -> String { format!( - "Within an except clause, raise exceptions with `raise ... from err` or `raise ... \ + "Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... \ from None` to distinguish them from errors in exception handling" ) } } -struct RaiseVisitor { - diagnostics: Vec, -} +/// B904 +pub fn raise_without_from_inside_except(checker: &mut Checker, body: &[Stmt]) { + let raises = { + let mut visitor = RaiseStatementVisitor::default(); + visitor::walk_body(&mut visitor, body); + visitor.raises + }; -impl<'a> Visitor<'a> for RaiseVisitor { - fn visit_stmt(&mut self, stmt: &'a Stmt) { - match &stmt.node { - StmtKind::Raise { - exc: Some(exc), - cause: None, - } => match &exc.node { - ExprKind::Name { id, .. } if is_lower(id) => {} - _ => { - self.diagnostics.push(Diagnostic::new( - RaiseWithoutFromInsideExcept, - Range::from_located(stmt), - )); - } - }, - StmtKind::ClassDef { .. } - | StmtKind::FunctionDef { .. } - | StmtKind::AsyncFunctionDef { .. } - | StmtKind::Try { .. } - | StmtKind::TryStar { .. } => {} - StmtKind::If { body, orelse, .. } => { - visitor::walk_body(self, body); - visitor::walk_body(self, orelse); - } - StmtKind::While { body, .. } - | StmtKind::With { body, .. } - | StmtKind::AsyncWith { body, .. } - | StmtKind::For { body, .. } - | StmtKind::AsyncFor { body, .. } => { - visitor::walk_body(self, body); - } - StmtKind::Match { cases, .. } => { - for case in cases { - visitor::walk_body(self, &case.body); + for (range, exc, cause) in raises { + if cause.is_none() { + if let Some(exc) = exc { + match &exc.node { + ExprKind::Name { id, .. } if is_lower(id) => {} + _ => { + checker + .diagnostics + .push(Diagnostic::new(RaiseWithoutFromInsideExcept, range)); + } } } - _ => {} } } } - -/// B904 -pub fn raise_without_from_inside_except(checker: &mut Checker, body: &[Stmt]) { - let mut visitor = RaiseVisitor { - diagnostics: vec![], - }; - visitor::walk_body(&mut visitor, body); - checker.diagnostics.extend(visitor.diagnostics); -} diff --git a/crates/ruff/src/rules/tryceratops/rules/reraise_no_cause.rs b/crates/ruff/src/rules/tryceratops/rules/reraise_no_cause.rs index d9054fed3b..2485424a2d 100644 --- a/crates/ruff/src/rules/tryceratops/rules/reraise_no_cause.rs +++ b/crates/ruff/src/rules/tryceratops/rules/reraise_no_cause.rs @@ -1,8 +1,9 @@ -use ruff_macros::{define_violation, derive_message_formats}; -use rustpython_parser::ast::{ExprKind, Stmt, StmtKind}; +use rustpython_parser::ast::{ExprKind, Stmt}; -use crate::ast::types::Range; -use crate::ast::visitor::{self, Visitor}; +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::helpers::RaiseStatementVisitor; +use crate::ast::visitor::Visitor; use crate::checkers::ast::Checker; use crate::registry::Diagnostic; use crate::violation::Violation; @@ -17,23 +18,6 @@ impl Violation for ReraiseNoCause { } } -#[derive(Default)] -struct RaiseStatementVisitor<'a> { - raises: Vec<&'a Stmt>, -} - -impl<'a, 'b> Visitor<'b> for RaiseStatementVisitor<'a> -where - 'b: 'a, -{ - fn visit_stmt(&mut self, stmt: &'b Stmt) { - match stmt.node { - StmtKind::Raise { .. } => self.raises.push(stmt), - _ => visitor::walk_stmt(self, stmt), - } - } -} - /// TRY200 pub fn reraise_no_cause(checker: &mut Checker, body: &[Stmt]) { let raises = { @@ -44,17 +28,11 @@ pub fn reraise_no_cause(checker: &mut Checker, body: &[Stmt]) { visitor.raises }; - for stmt in raises { - if let StmtKind::Raise { exc, cause, .. } = &stmt.node { - if exc - .as_ref() - .map_or(false, |expr| matches!(expr.node, ExprKind::Call { .. })) - && cause.is_none() - { - checker - .diagnostics - .push(Diagnostic::new(ReraiseNoCause, Range::from_located(stmt))); - } + for (range, exc, cause) in raises { + if exc.map_or(false, |expr| matches!(expr.node, ExprKind::Call { .. })) && cause.is_none() { + checker + .diagnostics + .push(Diagnostic::new(ReraiseNoCause, range)); } } }