From bccba5d73ffd72f5f4cc09e25453ba343414c49c Mon Sep 17 00:00:00 2001 From: qdegraaf <34540841+qdegraaf@users.noreply.github.com> Date: Mon, 18 Sep 2023 19:46:12 +0200 Subject: [PATCH] [`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` --- .../test/fixtures/flake8_logging/LOG007.py | 16 +++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_logging/mod.rs | 1 + .../rules/exception_without_exc_info.rs | 65 +++++++++++++++++++ .../src/rules/flake8_logging/rules/mod.rs | 2 + ...ake8_logging__tests__LOG007_LOG007.py.snap | 40 ++++++++++++ ruff.schema.json | 1 + 8 files changed, 129 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_logging/LOG007.py create mode 100644 crates/ruff/src/rules/flake8_logging/rules/exception_without_exc_info.rs create mode 100644 crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG007_LOG007.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_logging/LOG007.py b/crates/ruff/resources/test/fixtures/flake8_logging/LOG007.py new file mode 100644 index 0000000000000..25dab6e099648 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_logging/LOG007.py @@ -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 diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index f0b2a05fce4b8..b28170a35039c 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -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, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index df4c8a5f0b8f2..d9d8b7b1b5db5 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -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, diff --git a/crates/ruff/src/rules/flake8_logging/mod.rs b/crates/ruff/src/rules/flake8_logging/mod.rs index 88ff0afccd51d..7a759b206fd9f 100644 --- a/crates/ruff/src/rules/flake8_logging/mod.rs +++ b/crates/ruff/src/rules/flake8_logging/mod.rs @@ -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()); diff --git a/crates/ruff/src/rules/flake8_logging/rules/exception_without_exc_info.rs b/crates/ruff/src/rules/flake8_logging/rules/exception_without_exc_info.rs new file mode 100644 index 0000000000000..ac3c32709e4f4 --- /dev/null +++ b/crates/ruff/src/rules/flake8_logging/rules/exception_without_exc_info.rs @@ -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())); + } +} diff --git a/crates/ruff/src/rules/flake8_logging/rules/mod.rs b/crates/ruff/src/rules/flake8_logging/rules/mod.rs index 85272a5c72d37..6a1756de94ce1 100644 --- a/crates/ruff/src/rules/flake8_logging/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_logging/rules/mod.rs @@ -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; diff --git a/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG007_LOG007.py.snap b/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG007_LOG007.py.snap new file mode 100644 index 0000000000000..598d9fd8b1e57 --- /dev/null +++ b/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG007_LOG007.py.snap @@ -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 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index c09942966a871..74f7245fcbbc7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2140,6 +2140,7 @@ "LOG00", "LOG001", "LOG002", + "LOG007", "LOG009", "N", "N8",