Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-logging] Implement LOG007: ExceptionWithoutExcInfo #7410

Merged
merged 9 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add ExcInfoFalseInException Violation
  • Loading branch information
qdegraaf committed Sep 18, 2023
commit dc1bacb7ec50ddb8e80bd8d3088e43bcad340787
17 changes: 17 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_logging/LOG007.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import logging
qdegraaf marked this conversation as resolved.
Show resolved Hide resolved
from logging import exception


logging.exception("foo") # OK


logging.exception("foo", exc_info=False) # LOG007


logging.exception("foo", exc_info=[]) # LOG007


exception("foo", exc_info=False) # LOG007


logging.exception("foo", exc_info=True) # OK
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::ExcInfoFalseInException) {
flake8_logging::rules::exc_info_false_in_exception(checker, call);
}
}
Expr::Dict(ast::ExprDict {
keys,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Logging, "009") => (RuleGroup::Preview, rules::flake8_logging::rules::UndocumentedWarn),

// flake8-logging
(Flake8Logging, "007") => (RuleGroup::Nursery, rules::flake8_logging::rules::ExcInfoFalseInException),
(Flake8Logging, "009") => (RuleGroup::Nursery, rules::flake8_logging::rules::UndocumentedWarn),

_ => return None,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/flake8_logging/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::ExcInfoFalseInException, 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());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
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_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 `exception()` method captures the exception automatically. Disabling this by setting
/// `exc_info=False` is the same as using `error()`, which is clearer and doesn’t need the
/// `exc_info`argument. This rule detects `exception()` calls with an exc_info argument that is
qdegraaf marked this conversation as resolved.
Show resolved Hide resolved
/// falsy.
///
/// ## Example
/// ```python
/// logging.exception("foo", exc_info=False)
/// ```
///
/// Use instead:
/// ```python
/// logging.error("foo")
/// ```
#[violation]
pub struct ExcInfoFalseInException;

impl Violation for ExcInfoFalseInException {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use of `logging.exception` with falsy `exc_info`")
}
}

/// LOG007
pub(crate) fn exc_info_false_in_exception(checker: &mut Checker, call: &ExprCall) {
if checker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to catch all log messages.

More common than

logging.exception("...")

is doing

logger = logging.getLogger(__name__)

logger.exception("...")

ruff has utils to detect logging calls, see is_logger_candidate and e.g. the G001 implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote to utilise is_logger_candidate. It flags logger.exception and logging.exception now but does not flag from logging import exception; exception() Is that a limitation of is_logger_candidate? And if so, do we want to create another branch of logic to catch those cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to #7502

.semantic()
.resolve_call_path(call.func.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "exception"]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth it to check the name of the call first before calling resolve_call_path()for performance reasons? Or would that just add unnecessary clutter?

{
if let Some(_) = call.arguments.find_keyword("exc_info").filter(|keyword| {
Truthiness::from_expr(&keyword.value, |id| checker.semantic().is_builtin(id))
.is_falsey()
}) {
checker
.diagnostics
.push(Diagnostic::new(ExcInfoFalseInException, call.range()));
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_logging/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
pub(crate) use direct_logger_instantiation::*;
pub(crate) use exc_info_false_in_exception::*;
pub(crate) use invalid_get_logger_argument::*;
pub(crate) use undocumented_warn::*;

mod direct_logger_instantiation;
mod exc_info_false_in_exception;
mod invalid_get_logger_argument;
mod undocumented_warn;
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/flake8_logging/mod.rs
---
LOG007.py:8:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
8 | logging.exception("foo", exc_info=False) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
|

LOG007.py:11:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
11 | logging.exception("foo", exc_info=[]) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
|

LOG007.py:14:1: LOG007 Use of `logging.exception` with falsy `exc_info`
|
14 | exception("foo", exc_info=False) # LOG007
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.