From 7ef7e0ddb67cced658e571b3bb2518b927dbece3 Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 15 Jan 2024 22:00:04 -0500 Subject: [PATCH] [`tryceratops`] Add fix for `error-instead-of-exception` (`TRY400`) (#9520) ## Summary add autofix for `error-instead-of-exception` (`TRY400`) ## Test Plan `cargo test` --- .../ruff_linter/src/rules/tryceratops/mod.rs | 20 ++ .../rules/error_instead_of_exception.rs | 74 +++++- ..._error-instead-of-exception_TRY400.py.snap | 11 + ...ops__tests__preview__TRY400_TRY400.py.snap | 215 ++++++++++++++++++ .../src/analyze/logging.rs | 2 +- 5 files changed, 316 insertions(+), 6 deletions(-) create mode 100644 crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__preview__TRY400_TRY400.py.snap diff --git a/crates/ruff_linter/src/rules/tryceratops/mod.rs b/crates/ruff_linter/src/rules/tryceratops/mod.rs index c2418287caaf1..7be5a106b495c 100644 --- a/crates/ruff_linter/src/rules/tryceratops/mod.rs +++ b/crates/ruff_linter/src/rules/tryceratops/mod.rs @@ -11,6 +11,8 @@ mod tests { use test_case::test_case; use crate::registry::Rule; + use crate::settings::types::PreviewMode; + use crate::settings::LinterSettings; use crate::test::test_path; use crate::{assert_messages, settings}; @@ -33,4 +35,22 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + + #[test_case(Rule::ErrorInsteadOfException, Path::new("TRY400.py"))] + fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("tryceratops").join(path).as_path(), + &LinterSettings { + preview: PreviewMode::Enabled, + ..LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } } 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 index d390dafda4492..71889aa915b19 100644 --- 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,12 +1,13 @@ -use ruff_diagnostics::{Diagnostic, 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}; +use ruff_python_ast::{self as ast, ExceptHandler, Expr}; use ruff_python_semantic::analyze::logging::exc_info; use ruff_python_stdlib::logging::LoggingLevel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; use crate::rules::tryceratops::helpers::LoggerCandidateVisitor; /// ## What it does @@ -42,16 +43,28 @@ 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] pub struct ErrorInsteadOfException; impl Violation for ErrorInsteadOfException { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!("Use `logging.exception` instead of `logging.error`") } + + fn fix_title(&self) -> Option { + Some(format!("Replace with `exception`")) + } } /// TRY400 @@ -67,9 +80,60 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce for (expr, logging_level) in calls { if matches!(logging_level, LoggingLevel::Error) { if exc_info(&expr.arguments, checker.semantic()).is_none() { - checker - .diagnostics - .push(Diagnostic::new(ErrorInsteadOfException, expr.range())); + let mut diagnostic = Diagnostic::new(ErrorInsteadOfException, expr.range()); + if checker.settings.preview.is_enabled() { + match expr.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + 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(|| { + let (import_edit, binding) = + checker.importer().get_or_import_symbol( + &ImportRequest::import("logging", "exception"), + expr.start(), + checker.semantic(), + )?; + let name_edit = + Edit::range_replacement(binding, expr.func.range()); + 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 + }, + )) + }); + } + _ => {} + } + } + checker.diagnostics.push(diagnostic); } } } diff --git a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap index 918fae360ea3a..d19d1d713403d 100644 --- a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap +++ b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap @@ -10,6 +10,7 @@ TRY400.py:16:9: TRY400 Use `logging.exception` instead of `logging.error` 17 | 18 | if True: | + = help: Replace with `exception` TRY400.py:19:13: TRY400 Use `logging.exception` instead of `logging.error` | @@ -17,6 +18,7 @@ TRY400.py:19:13: TRY400 Use `logging.exception` instead of `logging.error` 19 | logging.error("Context message here") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | + = help: Replace with `exception` TRY400.py:26:9: TRY400 Use `logging.exception` instead of `logging.error` | @@ -27,6 +29,7 @@ TRY400.py:26:9: TRY400 Use `logging.exception` instead of `logging.error` 27 | 28 | if True: | + = help: Replace with `exception` TRY400.py:29:13: TRY400 Use `logging.exception` instead of `logging.error` | @@ -34,6 +37,7 @@ TRY400.py:29:13: TRY400 Use `logging.exception` instead of `logging.error` 29 | logger.error("Context message here") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | + = help: Replace with `exception` TRY400.py:36:9: TRY400 Use `logging.exception` instead of `logging.error` | @@ -44,6 +48,7 @@ TRY400.py:36:9: TRY400 Use `logging.exception` instead of `logging.error` 37 | 38 | if True: | + = help: Replace with `exception` TRY400.py:39:13: TRY400 Use `logging.exception` instead of `logging.error` | @@ -51,6 +56,7 @@ TRY400.py:39:13: TRY400 Use `logging.exception` instead of `logging.error` 39 | log.error("Context message here") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | + = help: Replace with `exception` TRY400.py:46:9: TRY400 Use `logging.exception` instead of `logging.error` | @@ -61,6 +67,7 @@ TRY400.py:46:9: TRY400 Use `logging.exception` instead of `logging.error` 47 | 48 | if True: | + = help: Replace with `exception` TRY400.py:49:13: TRY400 Use `logging.exception` instead of `logging.error` | @@ -68,6 +75,7 @@ TRY400.py:49:13: TRY400 Use `logging.exception` instead of `logging.error` 49 | self.logger.error("Context message here") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | + = help: Replace with `exception` TRY400.py:87:9: TRY400 Use `logging.exception` instead of `logging.error` | @@ -78,6 +86,7 @@ TRY400.py:87:9: TRY400 Use `logging.exception` instead of `logging.error` 88 | 89 | if True: | + = help: Replace with `exception` TRY400.py:90:13: TRY400 Use `logging.exception` instead of `logging.error` | @@ -85,6 +94,7 @@ TRY400.py:90:13: TRY400 Use `logging.exception` instead of `logging.error` 90 | error("Context message here") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | + = help: Replace with `exception` TRY400.py:121:13: TRY400 Use `logging.exception` instead of `logging.error` | @@ -93,5 +103,6 @@ TRY400.py:121:13: TRY400 Use `logging.exception` instead of `logging.error` 121 | error("Context message here") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | + = help: Replace with `exception` diff --git a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__preview__TRY400_TRY400.py.snap b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__preview__TRY400_TRY400.py.snap new file mode 100644 index 0000000000000..9171d46a3beb1 --- /dev/null +++ b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__preview__TRY400_TRY400.py.snap @@ -0,0 +1,215 @@ +--- +source: crates/ruff_linter/src/rules/tryceratops/mod.rs +--- +TRY400.py:16:9: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +14 | a = 1 +15 | except Exception: +16 | logging.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 +17 | +18 | if True: + | + = help: Replace with `exception` + +ℹ Safe fix +13 13 | try: +14 14 | a = 1 +15 15 | except Exception: +16 |- logging.error("Context message here") + 16 |+ logging.exception("Context message here") +17 17 | +18 18 | if True: +19 19 | logging.error("Context message here") + +TRY400.py:19:13: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +18 | if True: +19 | logging.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + = help: Replace with `exception` + +ℹ Safe fix +16 16 | logging.error("Context message here") +17 17 | +18 18 | if True: +19 |- logging.error("Context message here") + 19 |+ logging.exception("Context message here") +20 20 | +21 21 | +22 22 | def bad(): + +TRY400.py:26:9: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +24 | a = 1 +25 | except Exception: +26 | logger.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 +27 | +28 | if True: + | + = help: Replace with `exception` + +ℹ Unsafe fix +23 23 | try: +24 24 | a = 1 +25 25 | except Exception: +26 |- logger.error("Context message here") + 26 |+ logger.exception("Context message here") +27 27 | +28 28 | if True: +29 29 | logger.error("Context message here") + +TRY400.py:29:13: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +28 | if True: +29 | logger.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + = help: Replace with `exception` + +ℹ Unsafe fix +26 26 | logger.error("Context message here") +27 27 | +28 28 | if True: +29 |- logger.error("Context message here") + 29 |+ logger.exception("Context message here") +30 30 | +31 31 | +32 32 | def bad(): + +TRY400.py:36:9: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +34 | a = 1 +35 | except Exception: +36 | log.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 +37 | +38 | if True: + | + = help: Replace with `exception` + +ℹ Unsafe fix +33 33 | try: +34 34 | a = 1 +35 35 | except Exception: +36 |- log.error("Context message here") + 36 |+ log.exception("Context message here") +37 37 | +38 38 | if True: +39 39 | log.error("Context message here") + +TRY400.py:39:13: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +38 | if True: +39 | log.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + = help: Replace with `exception` + +ℹ Unsafe fix +36 36 | log.error("Context message here") +37 37 | +38 38 | if True: +39 |- log.error("Context message here") + 39 |+ log.exception("Context message here") +40 40 | +41 41 | +42 42 | def bad(): + +TRY400.py:46:9: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +44 | a = 1 +45 | except Exception: +46 | self.logger.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 +47 | +48 | if True: + | + = help: Replace with `exception` + +ℹ Unsafe fix +43 43 | try: +44 44 | a = 1 +45 45 | except Exception: +46 |- self.logger.error("Context message here") + 46 |+ self.logger.exception("Context message here") +47 47 | +48 48 | if True: +49 49 | self.logger.error("Context message here") + +TRY400.py:49:13: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +48 | if True: +49 | self.logger.error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + = help: Replace with `exception` + +ℹ Unsafe fix +46 46 | self.logger.error("Context message here") +47 47 | +48 48 | if True: +49 |- self.logger.error("Context message here") + 49 |+ self.logger.exception("Context message here") +50 50 | +51 51 | +52 52 | def good(): + +TRY400.py:87:9: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +85 | a = 1 +86 | except Exception: +87 | error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 +88 | +89 | if True: + | + = help: Replace with `exception` + +ℹ Safe fix +84 84 | try: +85 85 | a = 1 +86 86 | except Exception: +87 |- error("Context message here") + 87 |+ exception("Context message here") +88 88 | +89 89 | if True: +90 90 | error("Context message here") + +TRY400.py:90:13: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +89 | if True: +90 | error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + = help: Replace with `exception` + +ℹ Safe fix +87 87 | error("Context message here") +88 88 | +89 89 | if True: +90 |- error("Context message here") + 90 |+ exception("Context message here") +91 91 | +92 92 | +93 93 | def good(): + +TRY400.py:121:13: TRY400 [*] Use `logging.exception` instead of `logging.error` + | +119 | b = 2 +120 | except Exception: +121 | error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + = help: Replace with `exception` + +ℹ Safe fix +118 118 | try: +119 119 | b = 2 +120 120 | except Exception: +121 |- error("Context message here") + 121 |+ exception("Context message here") + + diff --git a/crates/ruff_python_semantic/src/analyze/logging.rs b/crates/ruff_python_semantic/src/analyze/logging.rs index 205e3bb8e4c3b..f4ab056f05f95 100644 --- a/crates/ruff_python_semantic/src/analyze/logging.rs +++ b/crates/ruff_python_semantic/src/analyze/logging.rs @@ -64,7 +64,7 @@ pub fn is_logger_candidate( false } -/// If the keywords to a logging call contain `exc_info=True` or `exc_info=sys.exc_info()`, +/// If the keywords to a logging call contain `exc_info=True` or `exc_info=sys.exc_info()`, /// return the `Keyword` for `exc_info`. pub fn exc_info<'a>(arguments: &'a Arguments, semantic: &SemanticModel) -> Option<&'a Keyword> { let exc_info = arguments.find_keyword("exc_info")?;