[flake8-logging] .exception() and exc_info= outside exception handlers (LOG004, LOG014) (#15799)

This commit is contained in:
InSync 2025-02-04 15:52:12 +07:00 committed by GitHub
parent 11cfe2ea8a
commit ba02294af3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
16 changed files with 720 additions and 15 deletions

View file

@ -0,0 +1,49 @@
from logging import exception as exc
import logging
logger = logging.getLogger(__name__)
# Tests adapted from:
# https://github.com/adamchainz/flake8-logging/blob/dbe88e7/tests/test_flake8_logging.py
### Errors
logging.exception("")
logger.exception("")
exc("")
def _():
logging.exception("")
logger.exception("")
exc("")
try:
...
except ...:
def _():
logging.exception("")
logger.exception("")
exc("")
### No errors
try:
...
except ...:
logging.exception("")
logger.exception("")
exc("")
def _():
try:
...
except ...:
logging.exception("")
logger.exception("")
exc("")

View file

@ -0,0 +1,4 @@
_ = (logger := __import__("somewhere").logger)
logger.exception("")

View file

@ -0,0 +1,57 @@
import logging
logger = logging.getLogger(__name__)
# Tests adapted from:
# https://github.com/adamchainz/flake8-logging/blob/dbe88e7/tests/test_flake8_logging.py
### Errors
logging.info("", exc_info=True)
logger.info("", exc_info=True)
logging.info("", exc_info=1)
logger.info("", exc_info=1)
def _():
logging.info("", exc_info=True)
logger.info("", exc_info=True)
try:
...
except ...:
def _():
logging.info("", exc_info=True)
logger.info("", exc_info=True)
### No errors
logging.info("", exc_info=a)
logger.info("", exc_info=a)
logging.info("", exc_info=False)
logger.info("", exc_info=False)
try:
...
except ...:
logging.info("", exc_info=True)
logger.info("", exc_info=True)
def _():
try:
...
except ...:
logging.info("", exc_info=True)
logger.info("", exc_info=True)
some_variable_that_ends_with_logger.not_a_recognized_method("", exc_info=True)

View file

@ -0,0 +1,4 @@
_ = (logger := __import__("somewhere").logger)
logger.info("", exc_info=True)

View file

@ -1176,6 +1176,12 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::StarmapZip) { if checker.enabled(Rule::StarmapZip) {
ruff::rules::starmap_zip(checker, call); ruff::rules::starmap_zip(checker, call);
} }
if checker.enabled(Rule::LogExceptionOutsideExceptHandler) {
flake8_logging::rules::log_exception_outside_except_handler(checker, call);
}
if checker.enabled(Rule::ExcInfoOutsideExceptHandler) {
flake8_logging::rules::exc_info_outside_except_handler(checker, call);
}
} }
Expr::Dict(dict) => { Expr::Dict(dict) => {
if checker.any_enabled(&[ if checker.any_enabled(&[

View file

@ -1125,8 +1125,10 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
// flake8-logging // flake8-logging
(Flake8Logging, "001") => (RuleGroup::Stable, rules::flake8_logging::rules::DirectLoggerInstantiation), (Flake8Logging, "001") => (RuleGroup::Stable, rules::flake8_logging::rules::DirectLoggerInstantiation),
(Flake8Logging, "002") => (RuleGroup::Stable, rules::flake8_logging::rules::InvalidGetLoggerArgument), (Flake8Logging, "002") => (RuleGroup::Stable, rules::flake8_logging::rules::InvalidGetLoggerArgument),
(Flake8Logging, "004") => (RuleGroup::Preview, rules::flake8_logging::rules::LogExceptionOutsideExceptHandler),
(Flake8Logging, "007") => (RuleGroup::Stable, rules::flake8_logging::rules::ExceptionWithoutExcInfo), (Flake8Logging, "007") => (RuleGroup::Stable, rules::flake8_logging::rules::ExceptionWithoutExcInfo),
(Flake8Logging, "009") => (RuleGroup::Stable, rules::flake8_logging::rules::UndocumentedWarn), (Flake8Logging, "009") => (RuleGroup::Stable, rules::flake8_logging::rules::UndocumentedWarn),
(Flake8Logging, "014") => (RuleGroup::Preview, rules::flake8_logging::rules::ExcInfoOutsideExceptHandler),
(Flake8Logging, "015") => (RuleGroup::Preview, rules::flake8_logging::rules::RootLoggerCall), (Flake8Logging, "015") => (RuleGroup::Preview, rules::flake8_logging::rules::RootLoggerCall),
_ => return None, _ => return None,

View file

@ -10,7 +10,6 @@ mod tests {
use crate::assert_messages; use crate::assert_messages;
use crate::registry::Rule; use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::settings::LinterSettings; use crate::settings::LinterSettings;
use crate::test::test_path; use crate::test::test_path;
@ -18,6 +17,11 @@ mod tests {
#[test_case(Rule::InvalidGetLoggerArgument, Path::new("LOG002.py"))] #[test_case(Rule::InvalidGetLoggerArgument, Path::new("LOG002.py"))]
#[test_case(Rule::ExceptionWithoutExcInfo, Path::new("LOG007.py"))] #[test_case(Rule::ExceptionWithoutExcInfo, Path::new("LOG007.py"))]
#[test_case(Rule::UndocumentedWarn, Path::new("LOG009.py"))] #[test_case(Rule::UndocumentedWarn, Path::new("LOG009.py"))]
#[test_case(Rule::LogExceptionOutsideExceptHandler, Path::new("LOG004_0.py"))]
#[test_case(Rule::LogExceptionOutsideExceptHandler, Path::new("LOG004_1.py"))]
#[test_case(Rule::ExcInfoOutsideExceptHandler, Path::new("LOG014_0.py"))]
#[test_case(Rule::ExcInfoOutsideExceptHandler, Path::new("LOG014_1.py"))]
#[test_case(Rule::RootLoggerCall, Path::new("LOG015.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> { fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path( let diagnostics = test_path(
@ -27,18 +31,4 @@ mod tests {
assert_messages!(snapshot, diagnostics); assert_messages!(snapshot, diagnostics);
Ok(()) Ok(())
} }
#[test_case(Rule::RootLoggerCall, Path::new("LOG015.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("flake8_logging").join(path).as_path(),
&LinterSettings {
preview: PreviewMode::Enabled,
..LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
} }

View file

@ -0,0 +1,118 @@
use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::helpers::Truthiness;
use ruff_python_ast::{Expr, ExprAttribute, ExprCall};
use ruff_python_semantic::analyze::logging::is_logger_candidate;
use ruff_python_stdlib::logging::LoggingLevel;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::fix::edits::{remove_argument, Parentheses};
use crate::rules::flake8_logging::rules::helpers::outside_handlers;
/// ## What it does
/// Checks for logging calls with `exc_info=` outside exception handlers.
///
/// ## Why is this bad?
/// Using `exc_info=True` outside of an exception handler
/// attaches `None` as the exception information, leading to confusing messages:
///
/// ```pycon
/// >>> logging.warning("Uh oh", exc_info=True)
/// WARNING:root:Uh oh
/// NoneType: None
/// ```
///
/// ## Example
///
/// ```python
/// import logging
///
///
/// logging.warning("Foobar", exc_info=True)
/// ```
///
/// Use instead:
///
/// ```python
/// import logging
///
///
/// logging.warning("Foobar")
/// ```
///
/// ## Fix safety
/// The fix is always marked as unsafe, as it changes runtime behavior.
#[derive(ViolationMetadata)]
pub(crate) struct ExcInfoOutsideExceptHandler;
impl Violation for ExcInfoOutsideExceptHandler {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
"`exc_info=` outside exception handlers".to_string()
}
fn fix_title(&self) -> Option<String> {
Some("Remove `exc_info=`".to_string())
}
}
pub(crate) fn exc_info_outside_except_handler(checker: &mut Checker, call: &ExprCall) {
let semantic = checker.semantic();
if !outside_handlers(call.start(), semantic) {
return;
}
match &*call.func {
func @ Expr::Attribute(ExprAttribute { attr, .. }) => {
if !is_logger_candidate(func, semantic, &checker.settings.logger_objects) {
return;
}
if LoggingLevel::from_attribute(attr).is_none() {
return;
}
}
func @ Expr::Name(_) => {
let Some(qualified_name) = semantic.resolve_qualified_name(func) else {
return;
};
let ["logging", attr] = qualified_name.segments() else {
return;
};
if *attr != "log" && LoggingLevel::from_attribute(attr).is_none() {
return;
}
}
_ => return,
}
let Some(exc_info) = call.arguments.find_keyword("exc_info") else {
return;
};
let truthiness = Truthiness::from_expr(&exc_info.value, |id| semantic.has_builtin_binding(id));
if truthiness.into_bool() != Some(true) {
return;
}
let arguments = &call.arguments;
let source = checker.source();
let mut diagnostic = Diagnostic::new(ExcInfoOutsideExceptHandler, exc_info.range);
diagnostic.try_set_fix(|| {
let edit = remove_argument(exc_info, arguments, Parentheses::Preserve, source)?;
Ok(Fix::unsafe_edit(edit))
});
checker.diagnostics.push(diagnostic);
}

View file

@ -0,0 +1,24 @@
use ruff_python_ast::{Stmt, StmtTry};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::{Ranged, TextSize};
pub(super) fn outside_handlers(offset: TextSize, semantic: &SemanticModel) -> bool {
for stmt in semantic.current_statements() {
if matches!(stmt, Stmt::FunctionDef(_)) {
break;
}
let Stmt::Try(StmtTry { handlers, .. }) = stmt else {
continue;
};
if handlers
.iter()
.any(|handler| handler.range().contains(offset))
{
return false;
}
}
true
}

View file

@ -0,0 +1,109 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{Expr, ExprAttribute, ExprCall};
use ruff_python_semantic::analyze::logging;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::rules::flake8_logging::rules::helpers::outside_handlers;
/// ## What it does
/// Checks for `.exception()` logging calls outside of exception handlers.
///
/// ## Why is this bad?
/// [The documentation] states:
/// > This function should only be called from an exception handler.
///
/// Calling `.exception()` outside of an exception handler
/// attaches `None` as exception information, leading to confusing messages:
///
/// ```pycon
/// >>> logging.exception("example")
/// ERROR:root:example
/// NoneType: None
/// ```
///
/// ## Example
///
/// ```python
/// import logging
///
/// logging.exception("Foobar")
/// ```
///
/// Use instead:
///
/// ```python
/// import logging
///
/// logging.error("Foobar")
/// ```
///
/// ## Fix safety
/// The fix, if available, will always be marked as unsafe, as it changes runtime behavior.
///
/// [The documentation]: https://docs.python.org/3/library/logging.html#logging.exception
#[derive(ViolationMetadata)]
pub(crate) struct LogExceptionOutsideExceptHandler;
impl Violation for LogExceptionOutsideExceptHandler {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
"`.exception()` call outside exception handlers".to_string()
}
fn fix_title(&self) -> Option<String> {
Some("Replace with `.error()`".to_string())
}
}
/// LOG004
pub(crate) fn log_exception_outside_except_handler(checker: &mut Checker, call: &ExprCall) {
let semantic = checker.semantic();
if !outside_handlers(call.start(), semantic) {
return;
}
let fix = match &*call.func {
func @ Expr::Attribute(ExprAttribute { attr, .. }) => {
let logger_objects = &checker.settings.logger_objects;
if !logging::is_logger_candidate(func, semantic, logger_objects) {
return;
}
if attr != "exception" {
return;
}
let edit = Edit::range_replacement("error".to_string(), attr.range);
Some(Fix::unsafe_edit(edit))
}
func @ Expr::Name(_) => {
let Some(qualified_name) = semantic.resolve_qualified_name(func) else {
return;
};
if !matches!(qualified_name.segments(), ["logging", "exception"]) {
return;
}
None
}
_ => return,
};
let mut diagnostic = Diagnostic::new(LogExceptionOutsideExceptHandler, call.range);
if let Some(fix) = fix {
diagnostic.set_fix(fix);
}
checker.diagnostics.push(diagnostic);
}

View file

@ -1,11 +1,16 @@
pub(crate) use direct_logger_instantiation::*; pub(crate) use direct_logger_instantiation::*;
pub(crate) use exc_info_outside_except_handler::*;
pub(crate) use exception_without_exc_info::*; pub(crate) use exception_without_exc_info::*;
pub(crate) use invalid_get_logger_argument::*; pub(crate) use invalid_get_logger_argument::*;
pub(crate) use log_exception_outside_except_handler::*;
pub(crate) use root_logger_call::*; pub(crate) use root_logger_call::*;
pub(crate) use undocumented_warn::*; pub(crate) use undocumented_warn::*;
mod direct_logger_instantiation; mod direct_logger_instantiation;
mod exc_info_outside_except_handler;
mod exception_without_exc_info; mod exception_without_exc_info;
mod helpers;
mod invalid_get_logger_argument; mod invalid_get_logger_argument;
mod log_exception_outside_except_handler;
mod root_logger_call; mod root_logger_call;
mod undocumented_warn; mod undocumented_warn;

View file

@ -0,0 +1,150 @@
---
source: crates/ruff_linter/src/rules/flake8_logging/mod.rs
---
LOG004_0.py:13:1: LOG004 [*] `.exception()` call outside exception handlers
|
11 | ### Errors
12 |
13 | logging.exception("")
| ^^^^^^^^^^^^^^^^^^^^^ LOG004
14 | logger.exception("")
15 | exc("")
|
= help: Replace with `.error()`
Unsafe fix
10 10 |
11 11 | ### Errors
12 12 |
13 |-logging.exception("")
13 |+logging.error("")
14 14 | logger.exception("")
15 15 | exc("")
16 16 |
LOG004_0.py:14:1: LOG004 [*] `.exception()` call outside exception handlers
|
13 | logging.exception("")
14 | logger.exception("")
| ^^^^^^^^^^^^^^^^^^^^ LOG004
15 | exc("")
|
= help: Replace with `.error()`
Unsafe fix
11 11 | ### Errors
12 12 |
13 13 | logging.exception("")
14 |-logger.exception("")
14 |+logger.error("")
15 15 | exc("")
16 16 |
17 17 |
LOG004_0.py:15:1: LOG004 `.exception()` call outside exception handlers
|
13 | logging.exception("")
14 | logger.exception("")
15 | exc("")
| ^^^^^^^ LOG004
|
= help: Replace with `.error()`
LOG004_0.py:19:5: LOG004 [*] `.exception()` call outside exception handlers
|
18 | def _():
19 | logging.exception("")
| ^^^^^^^^^^^^^^^^^^^^^ LOG004
20 | logger.exception("")
21 | exc("")
|
= help: Replace with `.error()`
Unsafe fix
16 16 |
17 17 |
18 18 | def _():
19 |- logging.exception("")
19 |+ logging.error("")
20 20 | logger.exception("")
21 21 | exc("")
22 22 |
LOG004_0.py:20:5: LOG004 [*] `.exception()` call outside exception handlers
|
18 | def _():
19 | logging.exception("")
20 | logger.exception("")
| ^^^^^^^^^^^^^^^^^^^^ LOG004
21 | exc("")
|
= help: Replace with `.error()`
Unsafe fix
17 17 |
18 18 | def _():
19 19 | logging.exception("")
20 |- logger.exception("")
20 |+ logger.error("")
21 21 | exc("")
22 22 |
23 23 |
LOG004_0.py:21:5: LOG004 `.exception()` call outside exception handlers
|
19 | logging.exception("")
20 | logger.exception("")
21 | exc("")
| ^^^^^^^ LOG004
|
= help: Replace with `.error()`
LOG004_0.py:28:9: LOG004 [*] `.exception()` call outside exception handlers
|
26 | except ...:
27 | def _():
28 | logging.exception("")
| ^^^^^^^^^^^^^^^^^^^^^ LOG004
29 | logger.exception("")
30 | exc("")
|
= help: Replace with `.error()`
Unsafe fix
25 25 | ...
26 26 | except ...:
27 27 | def _():
28 |- logging.exception("")
28 |+ logging.error("")
29 29 | logger.exception("")
30 30 | exc("")
31 31 |
LOG004_0.py:29:9: LOG004 [*] `.exception()` call outside exception handlers
|
27 | def _():
28 | logging.exception("")
29 | logger.exception("")
| ^^^^^^^^^^^^^^^^^^^^ LOG004
30 | exc("")
|
= help: Replace with `.error()`
Unsafe fix
26 26 | except ...:
27 27 | def _():
28 28 | logging.exception("")
29 |- logger.exception("")
29 |+ logger.error("")
30 30 | exc("")
31 31 |
32 32 |
LOG004_0.py:30:9: LOG004 `.exception()` call outside exception handlers
|
28 | logging.exception("")
29 | logger.exception("")
30 | exc("")
| ^^^^^^^ LOG004
|
= help: Replace with `.error()`

View file

@ -0,0 +1,16 @@
---
source: crates/ruff_linter/src/rules/flake8_logging/mod.rs
---
LOG004_1.py:4:1: LOG004 [*] `.exception()` call outside exception handlers
|
4 | logger.exception("")
| ^^^^^^^^^^^^^^^^^^^^ LOG004
|
= help: Replace with `.error()`
Unsafe fix
1 1 | _ = (logger := __import__("somewhere").logger)
2 2 |
3 3 |
4 |-logger.exception("")
4 |+logger.error("")

View file

@ -0,0 +1,153 @@
---
source: crates/ruff_linter/src/rules/flake8_logging/mod.rs
---
LOG014_0.py:12:18: LOG014 [*] `exc_info=` outside exception handlers
|
10 | ### Errors
11 |
12 | logging.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
13 | logger.info("", exc_info=True)
|
= help: Remove `exc_info=`
Unsafe fix
9 9 |
10 10 | ### Errors
11 11 |
12 |-logging.info("", exc_info=True)
12 |+logging.info("")
13 13 | logger.info("", exc_info=True)
14 14 |
15 15 |
LOG014_0.py:13:17: LOG014 [*] `exc_info=` outside exception handlers
|
12 | logging.info("", exc_info=True)
13 | logger.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
|
= help: Remove `exc_info=`
Unsafe fix
10 10 | ### Errors
11 11 |
12 12 | logging.info("", exc_info=True)
13 |-logger.info("", exc_info=True)
13 |+logger.info("")
14 14 |
15 15 |
16 16 | logging.info("", exc_info=1)
LOG014_0.py:16:18: LOG014 [*] `exc_info=` outside exception handlers
|
16 | logging.info("", exc_info=1)
| ^^^^^^^^^^ LOG014
17 | logger.info("", exc_info=1)
|
= help: Remove `exc_info=`
Unsafe fix
13 13 | logger.info("", exc_info=True)
14 14 |
15 15 |
16 |-logging.info("", exc_info=1)
16 |+logging.info("")
17 17 | logger.info("", exc_info=1)
18 18 |
19 19 |
LOG014_0.py:17:17: LOG014 [*] `exc_info=` outside exception handlers
|
16 | logging.info("", exc_info=1)
17 | logger.info("", exc_info=1)
| ^^^^^^^^^^ LOG014
|
= help: Remove `exc_info=`
Unsafe fix
14 14 |
15 15 |
16 16 | logging.info("", exc_info=1)
17 |-logger.info("", exc_info=1)
17 |+logger.info("")
18 18 |
19 19 |
20 20 | def _():
LOG014_0.py:21:22: LOG014 [*] `exc_info=` outside exception handlers
|
20 | def _():
21 | logging.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
22 | logger.info("", exc_info=True)
|
= help: Remove `exc_info=`
Unsafe fix
18 18 |
19 19 |
20 20 | def _():
21 |- logging.info("", exc_info=True)
21 |+ logging.info("")
22 22 | logger.info("", exc_info=True)
23 23 |
24 24 |
LOG014_0.py:22:21: LOG014 [*] `exc_info=` outside exception handlers
|
20 | def _():
21 | logging.info("", exc_info=True)
22 | logger.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
|
= help: Remove `exc_info=`
Unsafe fix
19 19 |
20 20 | def _():
21 21 | logging.info("", exc_info=True)
22 |- logger.info("", exc_info=True)
22 |+ logger.info("")
23 23 |
24 24 |
25 25 | try:
LOG014_0.py:29:26: LOG014 [*] `exc_info=` outside exception handlers
|
27 | except ...:
28 | def _():
29 | logging.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
30 | logger.info("", exc_info=True)
|
= help: Remove `exc_info=`
Unsafe fix
26 26 | ...
27 27 | except ...:
28 28 | def _():
29 |- logging.info("", exc_info=True)
29 |+ logging.info("")
30 30 | logger.info("", exc_info=True)
31 31 |
32 32 |
LOG014_0.py:30:25: LOG014 [*] `exc_info=` outside exception handlers
|
28 | def _():
29 | logging.info("", exc_info=True)
30 | logger.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
|
= help: Remove `exc_info=`
Unsafe fix
27 27 | except ...:
28 28 | def _():
29 29 | logging.info("", exc_info=True)
30 |- logger.info("", exc_info=True)
30 |+ logger.info("")
31 31 |
32 32 |
33 33 | ### No errors

View file

@ -0,0 +1,16 @@
---
source: crates/ruff_linter/src/rules/flake8_logging/mod.rs
---
LOG014_1.py:4:17: LOG014 [*] `exc_info=` outside exception handlers
|
4 | logger.info("", exc_info=True)
| ^^^^^^^^^^^^^ LOG014
|
= help: Remove `exc_info=`
Unsafe fix
1 1 | _ = (logger := __import__("somewhere").logger)
2 2 |
3 3 |
4 |-logger.info("", exc_info=True)
4 |+logger.info("")

2
ruff.schema.json generated
View file

@ -3411,9 +3411,11 @@
"LOG00", "LOG00",
"LOG001", "LOG001",
"LOG002", "LOG002",
"LOG004",
"LOG007", "LOG007",
"LOG009", "LOG009",
"LOG01", "LOG01",
"LOG014",
"LOG015", "LOG015",
"N", "N",
"N8", "N8",