[flake8-logging] Implement LOG007: ExceptionWithoutExcInfo (#7410)

## Summary

This PR implements a new rule for `flake8-logging` plugin that checks
for uses of `logging.exception()` with `exc_info` set to `False` or a
falsy value. It suggests using `logging.error` in these cases instead.

I am unsure about the name. Open to suggestions there, went with the
most explicit name I could think of in the meantime.

Refer https://github.com/astral-sh/ruff/issues/7248

## Test Plan

Added a new fixture cases and ran `cargo test`
This commit is contained in:
qdegraaf 2023-09-18 19:46:12 +02:00 committed by GitHub
parent 0bfdb15ecf
commit bccba5d73f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 129 additions and 0 deletions

View file

@ -0,0 +1,16 @@
import logging
logger = logging.getLogger(__name__)
logging.exception("foo") # OK
logging.exception("foo", exc_info=False) # LOG007
logging.exception("foo", exc_info=[]) # LOG007
logger.exception("foo") # OK
logger.exception("foo", exc_info=False) # LOG007
logger.exception("foo", exc_info=[]) # LOG007
from logging import exception
exception("foo", exc_info=False) # LOG007
exception("foo", exc_info=True) # OK

View file

@ -898,6 +898,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::InvalidGetLoggerArgument) {
flake8_logging::rules::invalid_get_logger_argument(checker, call);
}
if checker.enabled(Rule::ExceptionWithoutExcInfo) {
flake8_logging::rules::exception_without_exc_info(checker, call);
}
}
Expr::Dict(ast::ExprDict {
keys,

View file

@ -923,6 +923,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
// flake8-logging
(Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation),
(Flake8Logging, "002") => (RuleGroup::Preview, rules::flake8_logging::rules::InvalidGetLoggerArgument),
(Flake8Logging, "007") => (RuleGroup::Preview, rules::flake8_logging::rules::ExceptionWithoutExcInfo),
(Flake8Logging, "009") => (RuleGroup::Preview, rules::flake8_logging::rules::UndocumentedWarn),
_ => return None,

View file

@ -15,6 +15,7 @@ mod tests {
#[test_case(Rule::DirectLoggerInstantiation, Path::new("LOG001.py"))]
#[test_case(Rule::InvalidGetLoggerArgument, Path::new("LOG002.py"))]
#[test_case(Rule::ExceptionWithoutExcInfo, Path::new("LOG007.py"))]
#[test_case(Rule::UndocumentedWarn, Path::new("LOG009.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());

View file

@ -0,0 +1,65 @@
use ruff_python_ast::ExprCall;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::Truthiness;
use ruff_python_semantic::analyze::logging::is_logger_candidate;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for uses of `logging.exception()` with `exc_info` set to `False`.
///
/// ## Why is this bad?
/// The `logging.exception()` method captures the exception automatically, but
/// accepts an optional `exc_info` argument to override this behavior. Setting
/// `exc_info` to `False` disables the automatic capture of the exception and
/// stack trace.
///
/// Instead of setting `exc_info` to `False`, prefer `logging.error()`, which
/// has equivalent behavior to `logging.exception()` with `exc_info` set to
/// `False`, but is clearer in intent.
///
/// ## Example
/// ```python
/// logging.exception("...", exc_info=False)
/// ```
///
/// Use instead:
/// ```python
/// logging.error("...")
/// ```
#[violation]
pub struct ExceptionWithoutExcInfo;
impl Violation for ExceptionWithoutExcInfo {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use of `logging.exception` with falsy `exc_info`")
}
}
/// LOG007
pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) {
if !is_logger_candidate(
call.func.as_ref(),
checker.semantic(),
&["exception".to_string()],
) {
return;
}
if call
.arguments
.find_keyword("exc_info")
.map(|keyword| &keyword.value)
.is_some_and(|value| {
Truthiness::from_expr(value, |id| checker.semantic().is_builtin(id)).is_falsey()
})
{
checker
.diagnostics
.push(Diagnostic::new(ExceptionWithoutExcInfo, call.range()));
}
}

View file

@ -1,7 +1,9 @@
pub(crate) use direct_logger_instantiation::*;
pub(crate) use exception_without_exc_info::*;
pub(crate) use invalid_get_logger_argument::*;
pub(crate) use undocumented_warn::*;
mod direct_logger_instantiation;
mod exception_without_exc_info;
mod invalid_get_logger_argument;
mod undocumented_warn;

View file

@ -0,0 +1,40 @@
---
source: crates/ruff/src/rules/flake8_logging/mod.rs
---
LOG007.py:6:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
5 | logging.exception("foo") # OK
6 | logging.exception("foo", exc_info=False) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
7 | logging.exception("foo", exc_info=[]) # LOG007
8 | logger.exception("foo") # OK
|
LOG007.py:7:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
5 | logging.exception("foo") # OK
6 | logging.exception("foo", exc_info=False) # LOG007
7 | logging.exception("foo", exc_info=[]) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
8 | logger.exception("foo") # OK
9 | logger.exception("foo", exc_info=False) # LOG007
|
LOG007.py:9:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
7 | logging.exception("foo", exc_info=[]) # LOG007
8 | logger.exception("foo") # OK
9 | logger.exception("foo", exc_info=False) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
10 | logger.exception("foo", exc_info=[]) # LOG007
|
LOG007.py:10:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
8 | logger.exception("foo") # OK
9 | logger.exception("foo", exc_info=False) # LOG007
10 | logger.exception("foo", exc_info=[]) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
|

1
ruff.schema.json generated
View file

@ -2140,6 +2140,7 @@
"LOG00",
"LOG001",
"LOG002",
"LOG007",
"LOG009",
"N",
"N8",