Make unsafe sometimes

astral-sh · Jan 16, 2024 · 47b4422 · 47b4422
1 parent 6e63f4d
commit 47b4422
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 12 deletions.
diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs
@@ -1,4 +1,4 @@
-use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::visitor::Visitor;
 use ruff_python_ast::{self as ast, ExceptHandler, Expr};
@@ -43,6 +43,12 @@ use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
 ///         logging.exception("Exception occurred")
 /// ```
 ///
+/// ## Fix safety
+/// This rule's fix is marked as safe when run against `logging.error` calls,
+/// but unsafe when marked against other logger-like calls (e.g.,
+/// `logger.error`), since the rule is prone to false positives when detecting
+/// logger-like calls outside of the `logging` module.
+///
 /// ## References
 /// - [Python documentation: `logging.exception`](https://docs.python.org/3/library/logging.html#logging.exception)
 #[violation]
@@ -78,10 +84,22 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
                     if checker.settings.preview.is_enabled() {
                         match expr.func.as_ref() {
                             Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
-                                diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
-                                    "exception".to_string(),
-                                    attr.range(),
-                                )));
+                                diagnostic.set_fix(Fix::applicable_edit(
+                                    Edit::range_replacement("exception".to_string(), attr.range()),
+                                    // When run against `logging.error`, the fix is safe; otherwise,
+                                    // the object _may_ not be a logger.
+                                    if checker
+                                        .semantic()
+                                        .resolve_call_path(expr.func.as_ref())
+                                        .is_some_and(|call_path| {
+                                            matches!(call_path.as_slice(), ["logging", "error"])
+                                        })
+                                    {
+                                        Applicability::Safe
+                                    } else {
+                                        Applicability::Unsafe
+                                    },
+                                ));
                             }
                             Expr::Name(_) => {
                                 diagnostic.try_set_fix(|| {
@@ -93,7 +111,23 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
                                         )?;
                                     let name_edit =
                                         Edit::range_replacement(binding, expr.func.range());
-                                    Ok(Fix::safe_edits(import_edit, [name_edit]))
+                                    Ok(Fix::applicable_edits(
+                                        import_edit,
+                                        [name_edit],
+                                        // When run against `logging.error`, the fix is safe; otherwise,
+                                        // the object _may_ not be a logger.
+                                        if checker
+                                            .semantic()
+                                            .resolve_call_path(expr.func.as_ref())
+                                            .is_some_and(|call_path| {
+                                                matches!(call_path.as_slice(), ["logging", "error"])
+                                            })
+                                        {
+                                            Applicability::Safe
+                                        } else {
+                                            Applicability::Unsafe
+                                        },
+                                    ))
                                 });
                             }
                             _ => {}

diff --git a/...ceratops/snapshots/ruff_linter__rules__tryceratops__tests__preview__TRY400_TRY400.py.snap b/...ceratops/snapshots/ruff_linter__rules__tryceratops__tests__preview__TRY400_TRY400.py.snap
@@ -51,7 +51,7 @@ TRY400.py:26:9: TRY400 [*] Use `logging.exception` instead of `logging.error`
    |
    = help: Replace with `exception`
 
-ℹ Safe fix
+ℹ Unsafe fix
 23 23 |     try:
 24 24 |         a = 1
 25 25 |     except Exception:
@@ -69,7 +69,7 @@ TRY400.py:29:13: TRY400 [*] Use `logging.exception` instead of `logging.error`
    |
    = help: Replace with `exception`
 
-ℹ Safe fix
+ℹ Unsafe fix
 26 26 |         logger.error("Context message here")
 27 27 | 
 28 28 |         if True:
@@ -90,7 +90,7 @@ TRY400.py:36:9: TRY400 [*] Use `logging.exception` instead of `logging.error`
    |
    = help: Replace with `exception`
 
-ℹ Safe fix
+ℹ Unsafe fix
 33 33 |     try:
 34 34 |         a = 1
 35 35 |     except Exception:
@@ -108,7 +108,7 @@ TRY400.py:39:13: TRY400 [*] Use `logging.exception` instead of `logging.error`
    |
    = help: Replace with `exception`
 
-ℹ Safe fix
+ℹ Unsafe fix
 36 36 |         log.error("Context message here")
 37 37 | 
 38 38 |         if True:
@@ -129,7 +129,7 @@ TRY400.py:46:9: TRY400 [*] Use `logging.exception` instead of `logging.error`
    |
    = help: Replace with `exception`
 
-ℹ Safe fix
+ℹ Unsafe fix
 43 43 |     try:
 44 44 |         a = 1
 45 45 |     except Exception:
@@ -147,7 +147,7 @@ TRY400.py:49:13: TRY400 [*] Use `logging.exception` instead of `logging.error`
    |
    = help: Replace with `exception`
 
-ℹ Safe fix
+ℹ Unsafe fix
 46 46 |         self.logger.error("Context message here")
 47 47 | 
 48 48 |         if True: