Skip to content

Commit

Permalink
Avoid repeated triggers in nested tryceratops diagnostics (#8772)
Browse files Browse the repository at this point in the history
Closes #8770.
  • Loading branch information
charliermarsh committed Nov 19, 2023
1 parent 95e2f63 commit 71573fd
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 11 deletions.
10 changes: 10 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/tryceratops/TRY400.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,13 @@ def fine():
a = 1
except Exception:
error("Context message here", exc_info=sys.exc_info())


def nested():
try:
a = 1
except Exception:
try:
b = 2
except Exception:
error("Context message here")
22 changes: 22 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/tryceratops/TRY401.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,25 @@ def main_function():
except Exception as ex:
exception(f"Found an error: {er}")
exception(f"Found an error: {ex.field}")


def nested():
try:
process()
handle()
except Exception as ex:
try:
finish()
except Exception as ex:
logger.exception(f"Found an error: {ex}") # TRY401


def nested():
try:
process()
handle()
except Exception as ex:
try:
finish()
except Exception:
logger.exception(f"Found an error: {ex}") # TRY401
7 changes: 6 additions & 1 deletion crates/ruff_linter/src/rules/tryceratops/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_python_ast::visitor;
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, Expr};
use ruff_python_ast::{self as ast, ExceptHandler, Expr};
use ruff_python_semantic::analyze::logging;
use ruff_python_semantic::SemanticModel;
use ruff_python_stdlib::logging::LoggingLevel;
Expand Down Expand Up @@ -50,4 +50,9 @@ impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> {
}
visitor::walk_expr(self, expr);
}

fn visit_except_handler(&mut self, _except_handler: &'b ExceptHandler) {
// Don't recurse into exception handlers, since we'll re-run the visitor on any such
// handlers.
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
/// import logging
///
///
/// def foo():
/// def func():
/// try:
/// raise NotImplementedError
/// except NotImplementedError:
Expand All @@ -35,10 +35,10 @@ use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
/// import logging
///
///
/// def foo():
/// def func():
/// try:
/// raise NotImplementedError
/// except NotImplementedError as exc:
/// except NotImplementedError:
/// logging.exception("Exception occurred")
/// ```
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
/// ```python
/// try:
/// ...
/// except ValueError as e:
/// except ValueError:
/// logger.exception("Found an error")
/// ```
#[violation]
Expand Down Expand Up @@ -61,11 +61,7 @@ impl<'a> Visitor<'a> for NameVisitor<'a> {
/// TRY401
pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandler]) {
for handler in handlers {
let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { name, body, .. }) =
handler;
let Some(target) = name else {
continue;
};
let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler;

// Find all calls to `logging.exception`.
let calls = {
Expand All @@ -87,8 +83,14 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl
}
names
};

// Find any bound exceptions in the call.
for expr in names {
if expr.id == target.as_str() {
let Some(id) = checker.semantic().resolve_name(expr) else {
continue;
};
let binding = checker.semantic().binding(id);
if binding.kind.is_bound_exception() {
checker
.diagnostics
.push(Diagnostic::new(VerboseLogMessage, expr.range()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,12 @@ TRY400.py:90:13: TRY400 Use `logging.exception` instead of `logging.error`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|

TRY400.py:121:13: TRY400 Use `logging.exception` instead of `logging.error`
|
119 | b = 2
120 | except Exception:
121 | error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|


Original file line number Diff line number Diff line change
Expand Up @@ -177,4 +177,20 @@ TRY401.py:117:40: TRY401 Redundant exception object included in `logging.excepti
| ^^ TRY401
|

TRY401.py:139:49: TRY401 Redundant exception object included in `logging.exception` call
|
137 | finish()
138 | except Exception as ex:
139 | logger.exception(f"Found an error: {ex}") # TRY401
| ^^ TRY401
|

TRY401.py:150:49: TRY401 Redundant exception object included in `logging.exception` call
|
148 | finish()
149 | except Exception:
150 | logger.exception(f"Found an error: {ex}") # TRY401
| ^^ TRY401
|


0 comments on commit 71573fd

Please sign in to comment.