diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B040.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B040.py new file mode 100644 index 00000000000000..7fcf4a5a38702c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B040.py @@ -0,0 +1,196 @@ +def arbitrary_fun(*args, **kwargs): ... + + +def broken(): + try: + ... + except Exception as e: + e.add_note("...") # error + +def not_broken(): + try: + ... + except Exception as e: # safe (other linters will catch this) + pass + +def not_broken2(): + try: + ... + except Exception as e: + e.add_note("...") + raise # safe + +def broken2(): + # other exception raised + try: + ... + except Exception as e: + f = ValueError() + e.add_note("...") # error + raise f + +def not_broken3(): + # raised as cause + try: + ... + except Exception as e: + f = ValueError() + e.add_note("...") # safe + raise f from e + +def not_broken4(): + # assigned to other variable + try: + ... + except Exception as e: + e.add_note("...") # safe + foo = e + +def not_broken5(): + # "used" in function call + try: + ... + except Exception as e: + e.add_note("...") # safe + # not that printing the exception is actually using it, but we treat + # it being used as a parameter to any function as "using" it + print(e) + +def not_broken6(): + try: + ... + except Exception as e: + e.add_note("...") # safe + list(e) + +def not_broken7(): + # kwarg + try: + ... + except Exception as e: + e.add_note("...") # safe + arbitrary_fun(kwarg=e) + +def not_broken8(): + # stararg + try: + ... + except Exception as e: + e.add_note("...") # safe + arbitrary_fun(*(e,)) + +def not_broken9(): + try: + ... + except Exception as e: + e.add_note("...") # safe + arbitrary_fun(**{"e": e}) + + +def broken3(): + # multiple ExceptHandlers + try: + ... + except ValueError as e: + e.add_note("") # error + except TypeError as e: + raise e + +def not_broken10(): + # exception variable used before `add_note` + mylist = [] + try: + ... + except Exception as e: # safe + mylist.append(e) + e.add_note("") + +def not_broken11(): + # AnnAssign + try: + ... + except Exception as e: # safe + e.add_note("") + ann_assign_target: Exception = e + +def broken4(): + # special case: e is only used in the `add_note` call itself + try: + ... + except Exception as e: # error + e.add_note(str(e)) + e.add_note(str(e)) + +def broken5(): + # check nesting + try: + ... + except Exception as e: # error + e.add_note("") + try: + ... + except ValueError as e: + raise + +def broken6(): + # questionable if this should error + try: + ... + except Exception as e: + e.add_note("") + e = ValueError() + + + +def broken7(): + exc = ValueError() + exc.add_note("") + + +def not_broken12(exc: ValueError): + exc.add_note("") + + + +def not_broken13(): + e2 = ValueError() + try: + ... + except Exception as e: + e2.add_note(str(e)) # safe + raise e2 + + +def broken8(): + try: + ... + except Exception as e: # should error + e.add_note("") + f = lambda e: e + +def broken9(): + try: + ... + except Exception as e: # should error + e.add_note("") + + def habla(e): + raise e + + +# *** unhandled cases *** +# We also don't track other variables +try: + ... +except Exception as e: + e3 = e + e3.add_note("") # should error + +# we currently only check the target of the except handler, even though this clearly +# is hitting the same general pattern. But handling this case without a lot of false +# alarms is very tricky without more infrastructure. +try: + ... +except Exception as e: + e2 = ValueError() # should error + e2.add_note(str(e)) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 67f28b84ba94b8..c11ae1d1e69950 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1474,6 +1474,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, stmt, body, handlers, orelse, finalbody, ); } + if checker.enabled(Rule::SuppressedException) { + flake8_bugbear::rules::suppressed_exception(checker, handlers); + } if checker.enabled(Rule::ReturnInTryExceptFinally) { flake8_simplify::rules::return_in_try_except_finally( checker, body, handlers, finalbody, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 412509c4e79091..ffe7fe94fa6774 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -347,6 +347,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bugbear, "034") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ReSubPositionalArgs), (Flake8Bugbear, "035") => (RuleGroup::Stable, rules::flake8_bugbear::rules::StaticKeyDictComprehension), (Flake8Bugbear, "039") => (RuleGroup::Preview, rules::flake8_bugbear::rules::MutableContextvarDefault), + (Flake8Bugbear, "040") => (RuleGroup::Preview, rules::flake8_bugbear::rules::SuppressedException), (Flake8Bugbear, "901") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ReturnInGenerator), (Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept), (Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index 1f122f15438b3f..a124cd615e3978 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -65,6 +65,7 @@ mod tests { #[test_case(Rule::ReturnInGenerator, Path::new("B901.py"))] #[test_case(Rule::LoopIteratorMutation, Path::new("B909.py"))] #[test_case(Rule::MutableContextvarDefault, Path::new("B039.py"))] + #[test_case(Rule::SuppressedException, Path::new("B040.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs index b4af7755dd4358..f1b55ed430fac3 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs @@ -27,6 +27,7 @@ pub(crate) use setattr_with_constant::*; pub(crate) use star_arg_unpacking_after_keyword_arg::*; pub(crate) use static_key_dict_comprehension::*; pub(crate) use strip_with_multi_characters::*; +pub(crate) use suppressed_exception::*; pub(crate) use unary_prefix_increment_decrement::*; pub(crate) use unintentional_type_annotation::*; pub(crate) use unreliable_callable_check::*; @@ -65,6 +66,7 @@ mod setattr_with_constant; mod star_arg_unpacking_after_keyword_arg; mod static_key_dict_comprehension; mod strip_with_multi_characters; +mod suppressed_exception; mod unary_prefix_increment_decrement; mod unintentional_type_annotation; mod unreliable_callable_check; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/suppressed_exception.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/suppressed_exception.rs new file mode 100644 index 00000000000000..eaa5ead07fd295 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/suppressed_exception.rs @@ -0,0 +1,174 @@ +use ruff_python_ast::{self as ast, ExceptHandler, Expr, Stmt}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::RaiseStatementVisitor; +use ruff_python_ast::statement_visitor::StatementVisitor; +use ruff_python_semantic::Binding; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for exception with added note not reraised. +/// +/// ## Why is this bad? +/// The `.add_note' function does not use the exception. +/// It will implicitly be equivalent to passing the exception. +/// +/// +/// ## Example +/// ```python +/// try: +/// print("hello") +/// except Exception as e: +/// e.add_note("test") +/// ``` +/// +/// Use instead: +/// ```python +/// try: +/// print("hello") +/// except Exception as e: +/// e.add_note("test") +/// raise e +/// ``` +/// +/// ## References +/// - [Python documentation: `raise`](https://docs.python.org/3/reference/simple_stmts.html#the-raise-statement) +#[violation] +pub struct SuppressedException; + +impl Violation for SuppressedException { + #[derive_message_formats] + fn message(&self) -> String { + format!("Caught exception with call to `add_note` not used. Did you forget to `raise` it?") + } +} + +#[derive(Debug, Clone)] +struct AddNote<'a> { + receiver: &'a ast::ExprName, +} + +fn match_str_expr(expr: &Expr) -> bool { + let Expr::Call(ast::ExprCall { func, .. }) = expr else { + return false; + }; + + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return false; + }; + + if *id == "str" { + return true; + } + false +} +fn match_add_note(stmt: &Stmt) -> Option { + let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else { + return None; + }; + + let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else { + return None; + }; + + let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { + return None; + }; + + if attr != "add_note" { + return None; + } + + let Expr::Name(receiver @ ast::ExprName { .. }) = value.as_ref() else { + return None; + }; + + Some(AddNote { receiver }) +} + +/// B040 +pub(crate) fn suppressed_exception(checker: &mut Checker, handlers: &[ExceptHandler]) { + let mut count_str_references: usize = 0; + for handler in handlers { + let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, name, .. }) = + handler; + + let raises = { + let mut visitor = RaiseStatementVisitor::default(); + visitor.visit_body(body); + visitor.raises + }; + + let mut exception_raised = false; + if let Some(exception_name) = name { + // Check if the exception was used in the raise or in the cause. This is also true for a bare raise, as the exception is used. + for (.., exc, cause) in raises { + if let Some(exc) = exc { + if !exc + .as_name_expr() + .is_some_and(|ast::ExprName { id, .. }| exception_name.id == *id) + { + exception_raised = false; + } + + if let Some(cause) = cause { + if cause + .as_name_expr() + .is_some_and(|ast::ExprName { id, .. }| exception_name.id == *id) + { + exception_raised = true; + } + } + } else { + exception_raised = true; + } + } + + let semantic = checker.semantic(); + let bindings: Vec<&Binding> = semantic + .current_scope() + .get_all(exception_name.id.as_str()) + .map(|binding_id| semantic.binding(binding_id)) + .collect(); + + let Some(binding) = bindings.first() else { + return; + }; + + for reference_id in binding.references() { + let reference = checker.semantic().reference(reference_id); + if let Some(node_id) = reference.expression_id() { + if let Some(expr) = semantic.parent_expression(node_id) { + if match_str_expr(expr) { + count_str_references += 1; + } + } + } + } + + let count_references = binding.references().count(); + let count_add_note = body + .iter() + .filter_map(match_add_note) + .filter(|add_note| add_note.receiver.id == exception_name.id) + .count(); + + if count_add_note > 0 && !exception_raised { + if count_references - count_str_references == count_add_note + || count_references == 0 + { + checker + .diagnostics + .push(Diagnostic::new(SuppressedException, exception_name.range())); + } + } else { + return; + } + } else { + return; + } + } +} diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B040_B040.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B040_B040.py.snap new file mode 100644 index 00000000000000..d3f94fabc02283 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B040_B040.py.snap @@ -0,0 +1,80 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +--- +B040.py:7:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +5 | try: +6 | ... +7 | except Exception as e: + | ^ B040 +8 | e.add_note("...") # error + | + +B040.py:27:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +25 | try: +26 | ... +27 | except Exception as e: + | ^ B040 +28 | f = ValueError() +29 | e.add_note("...") # error + | + +B040.py:94:26: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +92 | try: +93 | ... +94 | except ValueError as e: + | ^ B040 +95 | e.add_note("") # error +96 | except TypeError as e: + | + +B040.py:120:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +118 | try: +119 | ... +120 | except Exception as e: # error + | ^ B040 +121 | e.add_note(str(e)) +122 | e.add_note(str(e)) + | + +B040.py:128:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +126 | try: +127 | ... +128 | except Exception as e: # error + | ^ B040 +129 | e.add_note("") +130 | try: + | + +B040.py:139:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +137 | try: +138 | ... +139 | except Exception as e: + | ^ B040 +140 | e.add_note("") +141 | e = ValueError() + | + +B040.py:167:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +165 | try: +166 | ... +167 | except Exception as e: # should error + | ^ B040 +168 | e.add_note("") +169 | f = lambda e: e + | + +B040.py:174:25: B040 Caught exception with call to `add_note` not used. Did you forget to `raise` it? + | +172 | try: +173 | ... +174 | except Exception as e: # should error + | ^ B040 +175 | e.add_note("") + | diff --git a/ruff.schema.json b/ruff.schema.json index 58159219175465..6526e2384c4650 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2761,6 +2761,8 @@ "B034", "B035", "B039", + "B04", + "B040", "B9", "B90", "B901",