diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG014.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG014.py new file mode 100644 index 00000000000000..46c9b7b99c8c50 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG014.py @@ -0,0 +1,154 @@ +import logging + +# Logging ---------------------------------------------------------------------- + +logging.exception("foo") # OK +logging.exception("foo", exc_info=False) # OK +logging.exception("foo", exc_info=[]) # OK +logging.exception("foo", exc_info=True) # LOG014 +logging.exception("foo", exc_info=[1]) # LOG014 + +logging.error("foo", exc_info=False) # OK +logging.error("foo", exc_info=True) # LOG014 + +logging.info("foo", exc_info=False) # OK +logging.info("foo", exc_info=True) # LOG014 + +try: + a = 1 * 2 +except Exception: + logging.exception("foo") # OK + logging.exception("foo", exc_info=False) # OK + logging.exception("foo", exc_info=[]) # OK + logging.exception("foo", exc_info=True) # OK + logging.exception("foo", exc_info=[1]) # OK + + logging.error("foo", exc_info=False) # OK + logging.error("foo", exc_info=True) # OK + + logging.info("foo", exc_info=False) # OK + logging.info("foo", exc_info=True) # OK + +# Logger ----------------------------------------------------------------------- + +logger = logging.getLogger(__name__) + +logger.exception("foo") # OK +logger.exception("foo", exc_info=False) # OK +logger.exception("foo", exc_info=[]) # OK +logger.exception("foo", exc_info=True) # LOG014 +logger.exception("foo", exc_info=[1]) # LOG014 + +logger.error("foo", exc_info=False) # OK +logger.error("foo", exc_info=True) # LOG014 + +logger.info("foo", exc_info=False) # OK +logger.info("foo", exc_info=True) # LOG014 + +try: + a = 1 * 2 +except Exception: + logger.exception("foo") # OK + logger.exception("foo", exc_info=False) # OK + logger.exception("foo", exc_info=[]) # OK + logger.exception("foo", exc_info=True) # OK + logger.exception("foo", exc_info=[1]) # OK + + logger.error("foo", exc_info=False) # OK + logger.error("foo", exc_info=True) # OK + + logger.info("foo", exc_info=False) # OK + logger.info("foo", exc_info=True) # OK + +# Direct Call ------------------------------------------------------------------ + +from logging import exception, error, info + +exception("foo") # OK +exception("foo", exc_info=False) # OK +exception("foo", exc_info=[]) # OK +exception("foo", exc_info=True) # LOG014 +exception("foo", exc_info=[1]) # LOG014 + +error("foo", exc_info=False) # OK +error("foo", exc_info=True) # LOG014 + +info("foo", exc_info=False) # OK +info("foo", exc_info=True) # LOG014 + +try: + a = 1 * 2 +except Exception: + exception("foo") # OK + exception("foo", exc_info=False) # OK + exception("foo", exc_info=[]) # OK + exception("foo", exc_info=True) # OK + exception("foo", exc_info=[1]) # OK + + error("foo", exc_info=False) # OK + error("foo", exc_info=True) # OK + + info("foo", exc_info=False) # OK + info("foo", exc_info=True) # OK + +# Fake Call -------------------------------------------------------------------- + +def wrapper(): + exception = lambda *args, **kwargs: None + error = lambda *args, **kwargs: None + info = lambda *args, **kwargs: None + + exception("foo") # OK + exception("foo", exc_info=False) # OK + exception("foo", exc_info=[]) # OK + exception("foo", exc_info=True) # OK + exception("foo", exc_info=[1]) # OK + + error("foo", exc_info=False) # OK + error("foo", exc_info=True) # OK + + info("foo", exc_info=False) # OK + info("foo", exc_info=True) # OK + + try: + a = 1 * 2 + except Exception: + exception("foo") # OK + exception("foo", exc_info=False) # OK + exception("foo", exc_info=[]) # OK + exception("foo", exc_info=True) # OK + exception("foo", exc_info=[1]) # OK + + error("foo", exc_info=False) # OK + error("foo", exc_info=True) # OK + + info("foo", exc_info=False) # OK + info("foo", exc_info=True) # OK + +# Nested ----------------------------------------------------------------------- + +def apple_1(): + try: + logging.exception("foo", exc_info=True) # LOG014 + except Exception: + pass + + +def apple_2(): + def banana(): + logging.exception("foo", exc_info=True) # LOG014 + + try: + banana() + except Exception: + pass + + +def apple_3(): + def banana(): + logging.exception("foo", exc_info=True) # LOG014 + + try: + a = 1 * 2 + except Exception: + banana() diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 304e142ba9ff00..0fdb5a32c797aa 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1030,6 +1030,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::RootLoggerCall) { flake8_logging::rules::root_logger_call(checker, call); } + if checker.enabled(Rule::ExcInfoOutsideExceptionHandler) { + flake8_logging::rules::exc_info_outside_exception_handler(checker, call); + } if checker.enabled(Rule::IsinstanceTypeNone) { refurb::rules::isinstance_type_none(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index dc28b554894463..d8e2115e796b2b 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1101,6 +1101,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Logging, "002") => (RuleGroup::Stable, rules::flake8_logging::rules::InvalidGetLoggerArgument), (Flake8Logging, "007") => (RuleGroup::Stable, rules::flake8_logging::rules::ExceptionWithoutExcInfo), (Flake8Logging, "009") => (RuleGroup::Stable, rules::flake8_logging::rules::UndocumentedWarn), + (Flake8Logging, "014") => (RuleGroup::Stable, rules::flake8_logging::rules::ExcInfoOutsideExceptionHandler), (Flake8Logging, "015") => (RuleGroup::Preview, rules::flake8_logging::rules::RootLoggerCall), _ => return None, diff --git a/crates/ruff_linter/src/registry/rule_set.rs b/crates/ruff_linter/src/registry/rule_set.rs index 103906118af3a7..241cb4020d4b37 100644 --- a/crates/ruff_linter/src/registry/rule_set.rs +++ b/crates/ruff_linter/src/registry/rule_set.rs @@ -5,7 +5,7 @@ use ruff_macros::CacheKey; use crate::registry::Rule; -const RULESET_SIZE: usize = 14; +const RULESET_SIZE: usize = 15; /// A set of [`Rule`]s. /// diff --git a/crates/ruff_linter/src/rules/flake8_logging/helpers.rs b/crates/ruff_linter/src/rules/flake8_logging/helpers.rs new file mode 100644 index 00000000000000..ea6d91987abac1 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_logging/helpers.rs @@ -0,0 +1,30 @@ +use crate::checkers::ast::Checker; +use ruff_python_ast::helpers::Truthiness; +use ruff_python_ast::ExprCall; + +fn _exc_info_arg_is_falsey_is(checker: &mut Checker, call: &ExprCall, boolean: bool) -> bool { + call.arguments + .find_keyword("exc_info") + .map(|keyword| &keyword.value) + .is_some_and(|value| { + let truthiness = + Truthiness::from_expr(value, |id| checker.semantic().has_builtin_binding(id)); + truthiness.into_bool() == Some(boolean) + }) +} + +pub(super) fn exc_info_arg_is_falsey(checker: &mut Checker, call: &ExprCall) -> bool { + _exc_info_arg_is_falsey_is(checker, call, false) +} + +pub(super) fn exc_info_arg_is_truey(checker: &mut Checker, call: &ExprCall) -> bool { + _exc_info_arg_is_falsey_is(checker, call, true) +} + +#[inline] +pub(super) fn is_logger_method_name(attr: &str) -> bool { + matches!( + attr, + "debug" | "info" | "warn" | "warning" | "error" | "critical" | "log" | "exception" + ) +} diff --git a/crates/ruff_linter/src/rules/flake8_logging/mod.rs b/crates/ruff_linter/src/rules/flake8_logging/mod.rs index 28991de4ebd93e..a113eeb27cd5f5 100644 --- a/crates/ruff_linter/src/rules/flake8_logging/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_logging/mod.rs @@ -1,4 +1,5 @@ //! Rules from [flake8-logging](https://pypi.org/project/flake8-logging/). +mod helpers; pub(crate) mod rules; #[cfg(test)] @@ -18,6 +19,7 @@ mod tests { #[test_case(Rule::InvalidGetLoggerArgument, Path::new("LOG002.py"))] #[test_case(Rule::ExceptionWithoutExcInfo, Path::new("LOG007.py"))] #[test_case(Rule::UndocumentedWarn, Path::new("LOG009.py"))] + #[test_case(Rule::ExcInfoOutsideExceptionHandler, Path::new("LOG014.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_logging/rules/exc_info_outside_exception_handler.rs b/crates/ruff_linter/src/rules/flake8_logging/rules/exc_info_outside_exception_handler.rs new file mode 100644 index 00000000000000..d76491d4e34d24 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_logging/rules/exc_info_outside_exception_handler.rs @@ -0,0 +1,102 @@ +use crate::checkers::ast::Checker; +use crate::rules::flake8_logging::helpers; +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::{self as ast, Expr, ExprCall, Stmt}; +use ruff_python_semantic::analyze::logging; +use ruff_python_stdlib::logging::LoggingLevel; +use ruff_text_size::Ranged; + +/// ## What it does +/// Checks for cases where a logging call is made with `exc_info` set to `True`, +/// outside of an exception handling block. +/// +/// ## Why is this bad? +/// When outside of an exception handling block, the variable holding the +/// exception information will be assigned to `None` as there is no active +/// exception, which then causes the final line of the logging call to contain +/// `NoneType: None`, which is meaningless +/// +/// ## Example +/// ```python +/// logging.error("...", exc_info=True) +/// ``` +/// +/// Either add an exception handling block: +/// ```python +/// try: +/// logging.error("...", exc_info=True) +/// except ...: +/// ... +/// ``` +/// +/// Or don't set `exc_info` to `True`: +/// logging.error("...") +#[derive(ViolationMetadata)] +pub(crate) struct ExcInfoOutsideExceptionHandler; + +impl Violation for ExcInfoOutsideExceptionHandler { + #[derive_message_formats] + fn message(&self) -> String { + "Use of `exc_info` outside an exception handler".to_string() + } +} + +/// LOG014 +pub(crate) fn exc_info_outside_exception_handler(checker: &mut Checker, call: &ExprCall) { + if is_logging_call(checker, call) + && helpers::exc_info_arg_is_truey(checker, call) + && currently_outside_exception_handler(checker) + { + checker.diagnostics.push(Diagnostic::new( + ExcInfoOutsideExceptionHandler, + call.range(), + )); + } +} + +fn currently_outside_exception_handler(checker: &mut Checker) -> bool { + for parent_stmt in checker.semantic().current_statements() { + if let Stmt::Try(_) = parent_stmt { + return false; + } + } + + true +} + +fn is_logging_call(checker: &mut Checker, call: &ExprCall) -> bool { + match call.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + // Match any logging level + if LoggingLevel::from_attribute(attr.as_str()).is_none() { + return false; + } + + if !logging::is_logger_candidate( + &call.func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + return false; + } + } + Expr::Name(_) => { + if !checker + .semantic() + .resolve_qualified_name(call.func.as_ref()) + .is_some_and(|qualified_name| { + matches!( + qualified_name.segments(), + ["logging", attr] if helpers::is_logger_method_name(attr) + ) + }) + { + return false; + } + } + _ => return false, + } + + true +} diff --git a/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs b/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs index eb7d92d1237e74..2129920b2f8339 100644 --- a/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs +++ b/crates/ruff_linter/src/rules/flake8_logging/rules/exception_without_exc_info.rs @@ -1,13 +1,12 @@ +use crate::checkers::ast::Checker; +use crate::rules::flake8_logging::helpers; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::helpers::Truthiness; use ruff_python_ast::{self as ast, Expr, ExprCall}; use ruff_python_semantic::analyze::logging; use ruff_python_stdlib::logging::LoggingLevel; 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`. /// @@ -73,20 +72,9 @@ pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) _ => return, } - if exc_info_arg_is_falsey(call, checker) { + if helpers::exc_info_arg_is_falsey(checker, call) { checker .diagnostics .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); } } - -fn exc_info_arg_is_falsey(call: &ExprCall, checker: &mut Checker) -> bool { - call.arguments - .find_keyword("exc_info") - .map(|keyword| &keyword.value) - .is_some_and(|value| { - let truthiness = - Truthiness::from_expr(value, |id| checker.semantic().has_builtin_binding(id)); - truthiness.into_bool() == Some(false) - }) -} diff --git a/crates/ruff_linter/src/rules/flake8_logging/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_logging/rules/mod.rs index 2e1e6fee4e3c1b..9bb8b99c3c37fd 100644 --- a/crates/ruff_linter/src/rules/flake8_logging/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_logging/rules/mod.rs @@ -1,10 +1,12 @@ pub(crate) use direct_logger_instantiation::*; +pub(crate) use exc_info_outside_exception_handler::*; pub(crate) use exception_without_exc_info::*; pub(crate) use invalid_get_logger_argument::*; pub(crate) use root_logger_call::*; pub(crate) use undocumented_warn::*; mod direct_logger_instantiation; +mod exc_info_outside_exception_handler; mod exception_without_exc_info; mod invalid_get_logger_argument; mod root_logger_call; diff --git a/crates/ruff_linter/src/rules/flake8_logging/rules/root_logger_call.rs b/crates/ruff_linter/src/rules/flake8_logging/rules/root_logger_call.rs index dab4d2a47e2ce6..8d7232f4344e60 100644 --- a/crates/ruff_linter/src/rules/flake8_logging/rules/root_logger_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging/rules/root_logger_call.rs @@ -4,6 +4,7 @@ use ruff_python_ast::ExprCall; use ruff_python_semantic::Modules; use crate::checkers::ast::Checker; +use crate::rules::flake8_logging::helpers; /// ## What it does /// Checks for usages of the following `logging` top-level functions: @@ -56,7 +57,7 @@ pub(crate) fn root_logger_call(checker: &mut Checker, call: &ExprCall) { }; let attr = match qualified_name.segments() { - ["logging", attr] if is_logger_method_name(attr) => attr, + ["logging", attr] if helpers::is_logger_method_name(attr) => attr, _ => return, }; @@ -67,11 +68,3 @@ pub(crate) fn root_logger_call(checker: &mut Checker, call: &ExprCall) { checker.diagnostics.push(diagnostic); } - -#[inline] -fn is_logger_method_name(attr: &str) -> bool { - matches!( - attr, - "debug" | "info" | "warn" | "warning" | "error" | "critical" | "log" | "exception" - ) -} diff --git a/crates/ruff_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG014_LOG014.py.snap b/crates/ruff_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG014_LOG014.py.snap new file mode 100644 index 00000000000000..9262ffa73c7ebc --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG014_LOG014.py.snap @@ -0,0 +1,134 @@ +--- +source: crates/ruff_linter/src/rules/flake8_logging/mod.rs +snapshot_kind: text +--- +LOG014.py:8:1: LOG014 Use of `exc_info` outside an exception handler + | +6 | logging.exception("foo", exc_info=False) # OK +7 | logging.exception("foo", exc_info=[]) # OK +8 | logging.exception("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +9 | logging.exception("foo", exc_info=[1]) # LOG014 + | + +LOG014.py:9:1: LOG014 Use of `exc_info` outside an exception handler + | + 7 | logging.exception("foo", exc_info=[]) # OK + 8 | logging.exception("foo", exc_info=True) # LOG014 + 9 | logging.exception("foo", exc_info=[1]) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +10 | +11 | logging.error("foo", exc_info=False) # OK + | + +LOG014.py:12:1: LOG014 Use of `exc_info` outside an exception handler + | +11 | logging.error("foo", exc_info=False) # OK +12 | logging.error("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +13 | +14 | logging.info("foo", exc_info=False) # OK + | + +LOG014.py:15:1: LOG014 Use of `exc_info` outside an exception handler + | +14 | logging.info("foo", exc_info=False) # OK +15 | logging.info("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +16 | +17 | try: + | + +LOG014.py:39:1: LOG014 Use of `exc_info` outside an exception handler + | +37 | logger.exception("foo", exc_info=False) # OK +38 | logger.exception("foo", exc_info=[]) # OK +39 | logger.exception("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +40 | logger.exception("foo", exc_info=[1]) # LOG014 + | + +LOG014.py:40:1: LOG014 Use of `exc_info` outside an exception handler + | +38 | logger.exception("foo", exc_info=[]) # OK +39 | logger.exception("foo", exc_info=True) # LOG014 +40 | logger.exception("foo", exc_info=[1]) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +41 | +42 | logger.error("foo", exc_info=False) # OK + | + +LOG014.py:43:1: LOG014 Use of `exc_info` outside an exception handler + | +42 | logger.error("foo", exc_info=False) # OK +43 | logger.error("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +44 | +45 | logger.info("foo", exc_info=False) # OK + | + +LOG014.py:46:1: LOG014 Use of `exc_info` outside an exception handler + | +45 | logger.info("foo", exc_info=False) # OK +46 | logger.info("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +47 | +48 | try: + | + +LOG014.py:70:1: LOG014 Use of `exc_info` outside an exception handler + | +68 | exception("foo", exc_info=False) # OK +69 | exception("foo", exc_info=[]) # OK +70 | exception("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +71 | exception("foo", exc_info=[1]) # LOG014 + | + +LOG014.py:71:1: LOG014 Use of `exc_info` outside an exception handler + | +69 | exception("foo", exc_info=[]) # OK +70 | exception("foo", exc_info=True) # LOG014 +71 | exception("foo", exc_info=[1]) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +72 | +73 | error("foo", exc_info=False) # OK + | + +LOG014.py:74:1: LOG014 Use of `exc_info` outside an exception handler + | +73 | error("foo", exc_info=False) # OK +74 | error("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +75 | +76 | info("foo", exc_info=False) # OK + | + +LOG014.py:77:1: LOG014 Use of `exc_info` outside an exception handler + | +76 | info("foo", exc_info=False) # OK +77 | info("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +78 | +79 | try: + | + +LOG014.py:139:9: LOG014 Use of `exc_info` outside an exception handler + | +137 | def apple_2(): +138 | def banana(): +139 | logging.exception("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +140 | +141 | try: + | + +LOG014.py:149:9: LOG014 Use of `exc_info` outside an exception handler + | +147 | def apple_3(): +148 | def banana(): +149 | logging.exception("foo", exc_info=True) # LOG014 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG014 +150 | +151 | try: + | diff --git a/ruff.schema.json b/ruff.schema.json index 40cdf7e96d4a9c..67327f626c0a26 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3320,6 +3320,7 @@ "LOG007", "LOG009", "LOG01", + "LOG014", "LOG015", "N", "N8",