mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-04 18:58:04 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Ignore error calls with exc_info in TRY400 (#4797)

Browse source
This commit is contained in:
Charlie Marsh 2023-06-02 00:59:45 -04:00 • committed by GitHub
parent b92be59ffe
commit 211d8e170d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 129 additions and 113 deletions
Download patch file Download diff file Expand all files Collapse all files

15
crates/ruff/resources/test/fixtures/tryceratops/TRY400.py vendored
View file

@ -4,6 +4,7 @@ Use '.exception' over '.error' inside except blocks
"""
import logging
import sys
logger = logging.getLogger(__name__)
@ -60,3 +61,17 @@ def good():
a = 1
except Exception:
foo.exception("Context message here")
def fine():
try:
a = 1
except Exception:
logger.error("Context message here", exc_info=True)
def fine():
try:
a = 1
except Exception:
logger.error("Context message here", exc_info=sys.exc_info())

63
crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs
View file

@ -4,6 +4,7 @@ use rustpython_parser::ast::{self, Constant, Expr, Keyword, Operator, Ranged};
use ruff_diagnostics::{Diagnostic, Edit, Fix};
use ruff_python_ast::helpers::{find_keyword, SimpleCallArgs};
use ruff_python_semantic::analyze::logging;
use ruff_python_semantic::analyze::logging::exc_info;
use ruff_python_stdlib::logging::LoggingLevel;
use crate::checkers::ast::Checker;
@ -192,53 +193,31 @@ pub(crate) fn logging_call(
}
// G201, G202
if checker.enabled(Rule::LoggingExcInfo)
|| checker.enabled(Rule::LoggingRedundantExcInfo)
{
if checker.any_enabled(&[Rule::LoggingExcInfo, Rule::LoggingRedundantExcInfo]) {
if !checker.semantic_model().in_exception_handler() {
return;
}
if let Some(exc_info) = find_keyword(keywords, "exc_info") {
// If `exc_info` is `True` or `sys.exc_info()`, it's redundant; but otherwise,
// return.
if !(matches!(
exc_info.value,
Expr::Constant(ast::ExprConstant {
value: Constant::Bool(true),
..
})
) || if let Expr::Call(ast::ExprCall { func, .. }) = &exc_info.value {
checker
.semantic_model()
.resolve_call_path(func)
.map_or(false, |call_path| {
call_path.as_slice() == ["sys", "exc_info"]
})
} else {
false
}) {
return;
}
if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
match logging_level {
LoggingLevel::Error => {
if checker.enabled(Rule::LoggingExcInfo) {
checker
.diagnostics
.push(Diagnostic::new(LoggingExcInfo, level_call_range));
}
let Some(exc_info) = exc_info(keywords, checker.semantic_model()) else {
return;
};
if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
match logging_level {
LoggingLevel::Error => {
if checker.enabled(Rule::LoggingExcInfo) {
checker
.diagnostics
.push(Diagnostic::new(LoggingExcInfo, level_call_range));
}
LoggingLevel::Exception => {
if checker.enabled(Rule::LoggingRedundantExcInfo) {
checker.diagnostics.push(Diagnostic::new(
LoggingRedundantExcInfo,
exc_info.range(),
));
}
}
_ => {}
}
LoggingLevel::Exception => {
if checker.enabled(Rule::LoggingRedundantExcInfo) {
checker.diagnostics.push(Diagnostic::new(
LoggingRedundantExcInfo,
exc_info.range(),
));
}
}
_ => {}
}
}
}

14
crates/ruff/src/rules/tryceratops/helpers.rs
View file

@ -7,14 +7,14 @@ use ruff_python_semantic::model::SemanticModel;
/// Collect `logging`-like calls from an AST.
pub(crate) struct LoggerCandidateVisitor<'a, 'b> {
context: &'a SemanticModel<'b>,
pub(crate) calls: Vec<(&'b Expr, &'b Expr)>,
semantic_model: &'a SemanticModel<'b>,
pub(crate) calls: Vec<&'b ast::ExprCall>,
}
impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> {
pub(crate) fn new(context: &'a SemanticModel<'b>) -> Self {
pub(crate) fn new(semantic_model: &'a SemanticModel<'b>) -> Self {
LoggerCandidateVisitor {
context,
semantic_model,
calls: Vec::new(),
}
}
@ -22,9 +22,9 @@ impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> {
impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> {
fn visit_expr(&mut self, expr: &'b Expr) {
if let Expr::Call(ast::ExprCall { func, .. }) = expr {
if logging::is_logger_candidate(func, self.context) {
self.calls.push((expr, func));
if let Expr::Call(call) = expr {
if logging::is_logger_candidate(&call.func, self.semantic_model) {
self.calls.push(call);
}
}
visitor::walk_expr(self, expr);

15
crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs
View file

@ -3,6 +3,7 @@ use rustpython_parser::ast::{self, Excepthandler, Expr, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::visitor::Visitor;
use ruff_python_semantic::analyze::logging::exc_info;
use crate::checkers::ast::Checker;
use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
@ -41,7 +42,7 @@ use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
/// ```
///
/// ## References
/// - [Python documentation](https://docs.python.org/3/library/logging.html#logging.exception)
/// - [Python documentation: `logging.exception`](https://docs.python.org/3/library/logging.html#logging.exception)
#[violation]
pub struct ErrorInsteadOfException;
@ -61,12 +62,14 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
visitor.visit_body(body);
visitor.calls
};
for (expr, func) in calls {
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func {
for expr in calls {
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() {
if attr == "error" {
checker
.diagnostics
.push(Diagnostic::new(ErrorInsteadOfException, expr.range()));
if exc_info(&expr.keywords, checker.semantic_model()).is_none() {
checker
.diagnostics
.push(Diagnostic::new(ErrorInsteadOfException, expr.range()));
}
}
}
}

32
crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs
View file

@ -1,4 +1,4 @@
use rustpython_parser::ast::{self, Excepthandler, Expr, ExprContext, Ranged};
use rustpython_parser::ast::{self, Excepthandler, Expr, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
@ -43,20 +43,13 @@ impl Violation for VerboseLogMessage {
#[derive(Default)]
struct NameVisitor<'a> {
names: Vec<(&'a str, &'a Expr)>,
names: Vec<&'a ast::ExprName>,
}
impl<'a, 'b> Visitor<'b> for NameVisitor<'a>
where
'b: 'a,
{
fn visit_expr(&mut self, expr: &'b Expr) {
impl<'a> Visitor<'a> for NameVisitor<'a> {
fn visit_expr(&mut self, expr: &'a Expr) {
match expr {
Expr::Name(ast::ExprName {
id,
ctx: ExprContext::Load,
range: _,
}) => self.names.push((id, expr)),
Expr::Name(name) if name.ctx.is_load() => self.names.push(name),
Expr::Attribute(_) => {}
_ => visitor::walk_expr(self, expr),
}
@ -79,24 +72,21 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[Excepthandl
visitor.calls
};
for (expr, func) in calls {
let Expr::Call(ast::ExprCall { args, .. }) = expr else {
continue;
};
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func {
for expr in calls {
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() {
if attr == "exception" {
// Collect all referenced names in the `logging.exception` call.
let names: Vec<(&str, &Expr)> = {
let names: Vec<&ast::ExprName> = {
let mut names = Vec::new();
for arg in args {
for arg in &expr.args {
let mut visitor = NameVisitor::default();
visitor.visit_expr(arg);
names.extend(visitor.names);
}
names
};
for (id, expr) in names {
if target == id {
for expr in names {
if expr.id == *target {
checker
.diagnostics
.push(Diagnostic::new(VerboseLogMessage, expr.range()));

72
crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap
View file

@ -1,71 +1,71 @@
---
source: crates/ruff/src/rules/tryceratops/mod.rs
---
TRY400.py:15:9: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:16:9: TRY400 Use `logging.exception` instead of `logging.error`
|
15 | a = 1
16 | except Exception:
17 | logging.error("Context message here")
16 | a = 1
17 | except Exception:
18 | logging.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
18 |
19 | if True:
19 |
20 | if True:
|
TRY400.py:18:13: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:19:13: TRY400 Use `logging.exception` instead of `logging.error`
|
18 | if True:
19 | logging.error("Context message here")
19 | if True:
20 | logging.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
TRY400.py:25:9: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:26:9: TRY400 Use `logging.exception` instead of `logging.error`
|
25 | a = 1
26 | except Exception:
27 | logger.error("Context message here")
26 | a = 1
27 | except Exception:
28 | logger.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
28 |
29 | if True:
29 |
30 | if True:
|
TRY400.py:28:13: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:29:13: TRY400 Use `logging.exception` instead of `logging.error`
|
28 | if True:
29 | logger.error("Context message here")
29 | if True:
30 | logger.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
TRY400.py:35:9: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:36:9: TRY400 Use `logging.exception` instead of `logging.error`
|
35 | a = 1
36 | except Exception:
37 | log.error("Context message here")
36 | a = 1
37 | except Exception:
38 | log.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
38 |
39 | if True:
39 |
40 | if True:
|
TRY400.py:38:13: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:39:13: TRY400 Use `logging.exception` instead of `logging.error`
|
38 | if True:
39 | log.error("Context message here")
39 | if True:
40 | log.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|
TRY400.py:45:9: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:46:9: TRY400 Use `logging.exception` instead of `logging.error`
|
45 | a = 1
46 | except Exception:
47 | self.logger.error("Context message here")
46 | a = 1
47 | except Exception:
48 | self.logger.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
48 |
49 | if True:
49 |
50 | if True:
|
TRY400.py:48:13: TRY400 Use `logging.exception` instead of `logging.error`
TRY400.py:49:13: TRY400 Use `logging.exception` instead of `logging.error`
|
48 | if True:
49 | self.logger.error("Context message here")
49 | if True:
50 | self.logger.error("Context message here")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
|

31
crates/ruff_python_semantic/src/analyze/logging.rs
View file

@ -1,6 +1,7 @@
use rustpython_parser::ast::{self, Expr};
use rustpython_parser::ast::{self, Constant, Expr, Keyword};
use ruff_python_ast::call_path::collect_call_path;
use ruff_python_ast::helpers::find_keyword;
use crate::model::SemanticModel;
@ -37,3 +38,31 @@ pub fn is_logger_candidate(func: &Expr, model: &SemanticModel) -> bool {
}
false
}
/// 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>(keywords: &'a [Keyword], model: &SemanticModel) -> Option<&'a Keyword> {
let exc_info = find_keyword(keywords, "exc_info")?;
// Ex) `logging.error("...", exc_info=True)`
if matches!(
exc_info.value,
Expr::Constant(ast::ExprConstant {
value: Constant::Bool(true),
..
})
) {
return Some(exc_info);
}
// Ex) `logging.error("...", exc_info=sys.exc_info())`
if let Expr::Call(ast::ExprCall { func, .. }) = &exc_info.value {
if model.resolve_call_path(func).map_or(false, |call_path| {
call_path.as_slice() == ["sys", "exc_info"]
}) {
return Some(exc_info);
}
}
None
}