Skip to content

Commit

Permalink
[tryceratops] Add fix for error-instead-of-exception (TRY400) (#…
Browse files Browse the repository at this point in the history 
…9520)

## Summary

add autofix for `error-instead-of-exception` (`TRY400`)

## Test Plan

`cargo test`
  • Loading branch information
@diceroll123
diceroll123 committed Jan 16, 2024
1 parent 2bddde2 commit 7ef7e0d
Show file tree
Hide file tree
Showing 5 changed files with 316 additions and 6 deletions.
20 changes: 20 additions & 0 deletions crates/ruff_linter/src/rules/tryceratops/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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(())
}
}
74 changes: 69 additions & 5 deletions crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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<String> {
Some(format!("Replace with `exception`"))
}
}

/// TRY400
Expand All @@ -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);
}
}
}
Expand Down
11 changes: 11 additions & 0 deletions ...apshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ 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`
|
18 | if True:
19 | logging.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
= help: Replace with `exception`

TRY400.py:26:9: TRY400 Use `logging.exception` instead of `logging.error`
|
Expand All @@ -27,13 +29,15 @@ 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`
|
28 | if True:
29 | logger.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
= help: Replace with `exception`

TRY400.py:36:9: TRY400 Use `logging.exception` instead of `logging.error`
|
Expand All @@ -44,13 +48,15 @@ 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`
|
38 | if True:
39 | log.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
= help: Replace with `exception`

TRY400.py:46:9: TRY400 Use `logging.exception` instead of `logging.error`
|
Expand All @@ -61,13 +67,15 @@ 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`
|
48 | if True:
49 | self.logger.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
= help: Replace with `exception`

TRY400.py:87:9: TRY400 Use `logging.exception` instead of `logging.error`
|
Expand All @@ -78,13 +86,15 @@ 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`
|
89 | if True:
90 | error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
= help: Replace with `exception`

TRY400.py:121:13: TRY400 Use `logging.exception` instead of `logging.error`
|
Expand All @@ -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`


0 comments on commit 7ef7e0d

Please sign in to comment.