Skip to content

Commit

Permalink
Ignore error calls with exc_info in TRY400 (#4797)
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh @konstin
charliermarsh authored and konstin committed Jun 13, 2023
1 parent be48306 commit 7b4db51
Show file tree
Hide file tree
Showing 7 changed files with 128 additions and 112 deletions.
15 changes: 15 additions & 0 deletions crates/ruff/resources/test/fixtures/tryceratops/TRY400.py
View file
Expand Up @@ -4,6 +4,7 @@
"""

import logging
import sys

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -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())
61 changes: 20 additions & 41 deletions crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
));
}
_ => {}
}
_ => {}
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions crates/ruff/src/rules/tryceratops/helpers.rs
View file
Expand Up @@ -7,24 +7,24 @@ 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(),
}
}
}

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);
Expand Down
15 changes: 9 additions & 6 deletions crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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()));
}
}
}
}
Expand Down
32 changes: 11 additions & 21 deletions 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};
Expand Down Expand Up @@ -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),
}
Expand All @@ -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()));
Expand Down
72 changes: 36 additions & 36 deletions ...tops/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
|

Expand Down
31 changes: 30 additions & 1 deletion 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;

Expand Down Expand Up @@ -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
}

0 comments on commit 7b4db51

Please sign in to comment.