From ca329f00dd9c9d1e7296f2c8677ffe2653cd6889 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Tue, 19 Sep 2023 17:20:06 +0200 Subject: [PATCH 01/12] Expand `is_logger_candidate` to check `Expr::Name` --- .../src/analyze/logging.rs | 72 +++++++++++-------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/crates/ruff_python_semantic/src/analyze/logging.rs b/crates/ruff_python_semantic/src/analyze/logging.rs index 205e3bb8e4c3b..290bd7b92df35 100644 --- a/crates/ruff_python_semantic/src/analyze/logging.rs +++ b/crates/ruff_python_semantic/src/analyze/logging.rs @@ -21,41 +21,55 @@ pub fn is_logger_candidate( semantic: &SemanticModel, logger_objects: &[String], ) -> bool { - let Expr::Attribute(ast::ExprAttribute { value, .. }) = func else { - return false; - }; + // logging.exception(), logger.error(), ... + if let Expr::Attribute(ast::ExprAttribute { value, .. }) = func { + // If the symbol was imported from another module, ensure that it's either a user-specified + // logger object, the `logging` module itself, or `flask.current_app.logger`. + if let Some(call_path) = semantic.resolve_call_path(value) { + if matches!( + call_path.as_slice(), + ["logging"] | ["flask", "current_app", "logger"] + ) { + return true; + } - // If the symbol was imported from another module, ensure that it's either a user-specified - // logger object, the `logging` module itself, or `flask.current_app.logger`. - if let Some(call_path) = semantic.resolve_call_path(value) { - if matches!( - call_path.as_slice(), - ["logging"] | ["flask", "current_app", "logger"] - ) { - return true; - } + if logger_objects + .iter() + .any(|logger| from_qualified_name(logger) == call_path) + { + return true; + } - if logger_objects - .iter() - .any(|logger| from_qualified_name(logger) == call_path) - { - return true; + return false; } - return false; + // Otherwise, if the symbol was defined in the current module, match against some common + // logger names. + if let Some(call_path) = collect_call_path(value) { + if let Some(tail) = call_path.last() { + if tail.starts_with("log") + || tail.ends_with("logger") + || tail.ends_with("logging") + || tail.starts_with("LOG") + || tail.ends_with("LOGGER") + || tail.ends_with("LOGGING") + { + return true; + } + } + } } - // Otherwise, if the symbol was defined in the current module, match against some common - // logger names. - if let Some(call_path) = collect_call_path(value) { - if let Some(tail) = call_path.last() { - if tail.starts_with("log") - || tail.ends_with("logger") - || tail.ends_with("logging") - || tail.starts_with("LOG") - || tail.ends_with("LOGGER") - || tail.ends_with("LOGGING") - { + // exception(), error(), ... + if let Expr::Name(_) = func { + if let Some(call_path) = semantic.resolve_call_path(func) { + if matches!( + call_path.as_slice(), + [ + "logging", + "critical" | "debug" | "error" | "exception" | "info" | "log" | "warning" + ] + ) { return true; } } From 340033534ba3aae307c4c265879d0321b8c2fd53 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Wed, 20 Sep 2023 15:28:53 +0200 Subject: [PATCH 02/12] Move Expr::Name check to LOG007 --- .../rules/exception_without_exc_info.rs | 58 ++++++++++----- ...ake8_logging__tests__LOG007_LOG007.py.snap | 9 +++ .../src/analyze/logging.rs | 73 ++++++++----------- 3 files changed, 76 insertions(+), 64 deletions(-) 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 49eb539b31595..318e10ef4b5ad 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 @@ -43,35 +43,53 @@ impl Violation for ExceptionWithoutExcInfo { /// LOG007 pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) { - let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = call.func.as_ref() else { - return; + if let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = call.func.as_ref() { + if !matches!( + LoggingLevel::from_attribute(attr.as_str()), + Some(LoggingLevel::Exception) + ) { + return; + } + + if !logging::is_logger_candidate( + &call.func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + return; + } + + if exc_info_arg_is_falsey(call, checker) { + checker + .diagnostics + .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); + } }; + if let Expr::Name(ast::ExprName { id, ..}) = call.func.as_ref() { + if id != "exception" { + return; + } - if !matches!( - LoggingLevel::from_attribute(attr.as_str()), - Some(LoggingLevel::Exception) - ) { - return; - } + if let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) { + if !matches!(call_path.as_slice(),["logging", "exception"]) { + return; + } + } - if !logging::is_logger_candidate( - &call.func, - checker.semantic(), - &checker.settings.logger_objects, - ) { - return; + if exc_info_arg_is_falsey(call, checker) { + checker + .diagnostics + .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); + } } +} - if call +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| { 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_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG007_LOG007.py.snap b/crates/ruff_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG007_LOG007.py.snap index df6cb6a004be0..a8df5679127e4 100644 --- a/crates/ruff_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG007_LOG007.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging/snapshots/ruff_linter__rules__flake8_logging__tests__LOG007_LOG007.py.snap @@ -40,4 +40,13 @@ LOG007.py:10:1: LOG007 Use of `logging.exception` with falsy `exc_info` 12 | logger.info("foo", exc_info=False) # OK | +LOG007.py:17:1: LOG007 Use of `logging.exception` with falsy `exc_info` + | +15 | from logging import exception +16 | +17 | exception("foo", exc_info=False) # LOG007 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LOG007 +18 | exception("foo", exc_info=True) # OK + | + diff --git a/crates/ruff_python_semantic/src/analyze/logging.rs b/crates/ruff_python_semantic/src/analyze/logging.rs index 290bd7b92df35..82b9bf2a1522b 100644 --- a/crates/ruff_python_semantic/src/analyze/logging.rs +++ b/crates/ruff_python_semantic/src/analyze/logging.rs @@ -21,55 +21,40 @@ pub fn is_logger_candidate( semantic: &SemanticModel, logger_objects: &[String], ) -> bool { - // logging.exception(), logger.error(), ... - if let Expr::Attribute(ast::ExprAttribute { value, .. }) = func { - // If the symbol was imported from another module, ensure that it's either a user-specified - // logger object, the `logging` module itself, or `flask.current_app.logger`. - if let Some(call_path) = semantic.resolve_call_path(value) { - if matches!( - call_path.as_slice(), - ["logging"] | ["flask", "current_app", "logger"] - ) { - return true; - } - - if logger_objects - .iter() - .any(|logger| from_qualified_name(logger) == call_path) - { - return true; - } - - return false; + let Expr::Attribute(ast::ExprAttribute { value, .. }) = func else { + return false; + }; + // If the symbol was imported from another module, ensure that it's either a user-specified + // logger object, the `logging` module itself, or `flask.current_app.logger`. + if let Some(call_path) = semantic.resolve_call_path(value) { + if matches!( + call_path.as_slice(), + ["logging"] | ["flask", "current_app", "logger"] + ) { + return true; } - // Otherwise, if the symbol was defined in the current module, match against some common - // logger names. - if let Some(call_path) = collect_call_path(value) { - if let Some(tail) = call_path.last() { - if tail.starts_with("log") - || tail.ends_with("logger") - || tail.ends_with("logging") - || tail.starts_with("LOG") - || tail.ends_with("LOGGER") - || tail.ends_with("LOGGING") - { - return true; - } - } + if logger_objects + .iter() + .any(|logger| from_qualified_name(logger) == call_path) + { + return true; } + + return false; } - // exception(), error(), ... - if let Expr::Name(_) = func { - if let Some(call_path) = semantic.resolve_call_path(func) { - if matches!( - call_path.as_slice(), - [ - "logging", - "critical" | "debug" | "error" | "exception" | "info" | "log" | "warning" - ] - ) { + // Otherwise, if the symbol was defined in the current module, match against some common + // logger names. + if let Some(call_path) = collect_call_path(value) { + if let Some(tail) = call_path.last() { + if tail.starts_with("log") + || tail.ends_with("logger") + || tail.ends_with("logging") + || tail.starts_with("LOG") + || tail.ends_with("LOGGER") + || tail.ends_with("LOGGING") + { return true; } } From 861131a0497d10f220fbd074c65bc65e9570b070 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Wed, 20 Sep 2023 16:16:11 +0200 Subject: [PATCH 03/12] Also check for Expr::Name in `logging_call.rs` expand tests for GXXX --- .../fixtures/flake8_logging_format/G001.py | 9 +++ .../fixtures/flake8_logging_format/G002.py | 5 ++ .../fixtures/flake8_logging_format/G003.py | 5 ++ .../fixtures/flake8_logging_format/G004.py | 4 ++ .../fixtures/flake8_logging_format/G010.py | 5 ++ .../fixtures/flake8_logging_format/G101_1.py | 9 +++ .../fixtures/flake8_logging_format/G101_2.py | 9 +++ .../fixtures/flake8_logging_format/G201.py | 22 +++++++ .../fixtures/flake8_logging_format/G202.py | 20 +++++++ .../rules/logging_call.rs | 46 +++++++++------ ...flake8_logging_format__tests__G001.py.snap | 58 +++++++++++++++++++ ...flake8_logging_format__tests__G002.py.snap | 18 ++++++ ...flake8_logging_format__tests__G003.py.snap | 18 ++++++ ...flake8_logging_format__tests__G004.py.snap | 28 +++++++-- ...flake8_logging_format__tests__G010.py.snap | 25 ++++++++ ...ake8_logging_format__tests__G101_1.py.snap | 10 ++++ ...ake8_logging_format__tests__G101_2.py.snap | 10 ++++ ...flake8_logging_format__tests__G201.py.snap | 20 +++++++ ...flake8_logging_format__tests__G202.py.snap | 20 +++++++ 19 files changed, 319 insertions(+), 22 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G001.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G001.py index 7a3a49a6a7d10..f100eae9a4d6c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G001.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G001.py @@ -16,3 +16,12 @@ flask.current_app.logger.info("Hello {}".format("World!")) current_app.logger.info("Hello {}".format("World!")) app.logger.log(logging.INFO, "Hello {}".format("World!")) + +from logging import info, log + +info("Hello {}".format("World!")) +log(logging.INFO, "Hello {}".format("World!")) +info("Hello {}".format("World!")) +log(logging.INFO, msg="Hello {}".format("World!")) +log(level=logging.INFO, msg="Hello {}".format("World!")) +log(msg="Hello {}".format("World!"), level=logging.INFO) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G002.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G002.py index 11b61a1cb8e7a..6c01eb8819e19 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G002.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G002.py @@ -2,3 +2,8 @@ logging.info("Hello %s" % "World!") logging.log(logging.INFO, "Hello %s" % "World!") + +from logging import info, log + +info("Hello %s" % "World!") +log(logging.INFO, "Hello %s" % "World!") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G003.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G003.py index 4e20dc6d9b285..1f52dbe3df615 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G003.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G003.py @@ -2,3 +2,8 @@ logging.info("Hello" + " " + "World!") logging.log(logging.INFO, "Hello" + " " + "World!") + +from logging import info, log + +info("Hello" + " " + "World!") +log(logging.INFO, "Hello" + " " + "World!") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py index 56e005293a62a..ffc9ed7dd2c03 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py @@ -6,3 +6,7 @@ _LOGGER = logging.getLogger() _LOGGER.info(f"{__name__}") + +from logging import info +info(f"{name}") +info(f"{__name__}") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G010.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G010.py index fc0029c40987a..3c1294069b06e 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G010.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G010.py @@ -8,3 +8,8 @@ logger.warn("Hello world!") logging . warn("Hello World!") + +from logging import warn, warning, exception +warn("foo") +warning("foo") +exception("foo") diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_1.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_1.py index 3386f5e480c88..08131551a2f7c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_1.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_1.py @@ -6,3 +6,12 @@ "name": "foobar", }, ) + +from logging import info + +info( + "Hello world!", + extra={ + "name": "foobar", + }, +) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_2.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_2.py index c73c54e4c67e0..c21e3c37cffc0 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_2.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G101_2.py @@ -6,3 +6,12 @@ name="foobar", ), ) + +from logging import info + +info( + "Hello world!", + extra=dict( + name="foobar", + ), +) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py index e85490752028e..4b4ec795f886c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py @@ -19,3 +19,25 @@ logging.error("Hello World", exc_info=False) logging.error("Hello World", exc_info=sys.exc_info()) + + +# G201 +from logging import error +try: + pass +except: + error("Hello World", exc_info=True) + +try: + pass +except: + error("Hello World", exc_info=sys.exc_info()) + +# OK +try: + pass +except: + error("Hello World", exc_info=False) + +error("Hello World", exc_info=sys.exc_info()) + diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G202.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G202.py index 6af630930ca1b..aca5f3a45baa1 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G202.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G202.py @@ -19,3 +19,23 @@ logging.exception("Hello World", exc_info=False) logging.exception("Hello World", exc_info=True) + +# G202 +from logging import exception +try: + pass +except: + exception("Hello World", exc_info=True) + +try: + pass +except: + exception("Hello World", exc_info=sys.exc_info()) + +# OK +try: + pass +except: + exception("Hello World", exc_info=False) + +exception("Hello World", exc_info=True) diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index 29b2b9711b602..687786dfa2b66 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Keyword, Operator}; use ruff_python_semantic::analyze::logging; use ruff_python_stdlib::logging::LoggingLevel; -use ruff_text_size::Ranged; +use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::registry::{AsRule, Rule}; @@ -153,20 +153,32 @@ impl LoggingCallType { /// Check logging calls for violations. pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { - let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = call.func.as_ref() else { - return; - }; + let range: TextRange; + let logging_call_type; - let Some(logging_call_type) = LoggingCallType::from_attribute(attr.as_str()) else { - return; - }; - - if !logging::is_logger_candidate( - &call.func, - checker.semantic(), - &checker.settings.logger_objects, - ) { - return; + match call.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) => { + let Some(call_type) = LoggingCallType::from_attribute(attr.as_str()) else { + return; + }; + if !logging::is_logger_candidate( + &call.func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + return; + } + logging_call_type = call_type; + range = attr.range(); + } + Expr::Name(ast::ExprName{ id , range: id_range, .. }) => { + let Some(call_type) = LoggingCallType::from_attribute(id.as_str()) else { + return; + }; + logging_call_type = call_type; + range = *id_range; + } + _ => {return} } // G001 - G004 @@ -181,11 +193,11 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { logging_call_type, LoggingCallType::LevelCall(LoggingLevel::Warn) ) { - let mut diagnostic = Diagnostic::new(LoggingWarn, attr.range()); + let mut diagnostic = Diagnostic::new(LoggingWarn, range); if checker.patch(diagnostic.kind.rule()) { diagnostic.set_fix(Fix::automatic(Edit::range_replacement( "warning".to_string(), - attr.range(), + range, ))); } checker.diagnostics.push(diagnostic); @@ -213,7 +225,7 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { if checker.enabled(Rule::LoggingExcInfo) { checker .diagnostics - .push(Diagnostic::new(LoggingExcInfo, attr.range())); + .push(Diagnostic::new(LoggingExcInfo, range)); } } LoggingLevel::Exception => { diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G001.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G001.py.snap index 7e2391e7b35b5..f6e052cf559f7 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G001.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G001.py.snap @@ -83,6 +83,64 @@ G001.py:18:30: G001 Logging statement uses `str.format` 17 | current_app.logger.info("Hello {}".format("World!")) 18 | app.logger.log(logging.INFO, "Hello {}".format("World!")) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 +19 | +20 | from logging import info, log + | + +G001.py:22:6: G001 Logging statement uses `str.format` + | +20 | from logging import info, log +21 | +22 | info("Hello {}".format("World!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 +23 | log(logging.INFO, "Hello {}".format("World!")) +24 | info("Hello {}".format("World!")) + | + +G001.py:23:19: G001 Logging statement uses `str.format` + | +22 | info("Hello {}".format("World!")) +23 | log(logging.INFO, "Hello {}".format("World!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 +24 | info("Hello {}".format("World!")) +25 | log(logging.INFO, msg="Hello {}".format("World!")) + | + +G001.py:24:6: G001 Logging statement uses `str.format` + | +22 | info("Hello {}".format("World!")) +23 | log(logging.INFO, "Hello {}".format("World!")) +24 | info("Hello {}".format("World!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 +25 | log(logging.INFO, msg="Hello {}".format("World!")) +26 | log(level=logging.INFO, msg="Hello {}".format("World!")) + | + +G001.py:25:23: G001 Logging statement uses `str.format` + | +23 | log(logging.INFO, "Hello {}".format("World!")) +24 | info("Hello {}".format("World!")) +25 | log(logging.INFO, msg="Hello {}".format("World!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 +26 | log(level=logging.INFO, msg="Hello {}".format("World!")) +27 | log(msg="Hello {}".format("World!"), level=logging.INFO) + | + +G001.py:26:29: G001 Logging statement uses `str.format` + | +24 | info("Hello {}".format("World!")) +25 | log(logging.INFO, msg="Hello {}".format("World!")) +26 | log(level=logging.INFO, msg="Hello {}".format("World!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 +27 | log(msg="Hello {}".format("World!"), level=logging.INFO) + | + +G001.py:27:9: G001 Logging statement uses `str.format` + | +25 | log(logging.INFO, msg="Hello {}".format("World!")) +26 | log(level=logging.INFO, msg="Hello {}".format("World!")) +27 | log(msg="Hello {}".format("World!"), level=logging.INFO) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ G001 | diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G002.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G002.py.snap index ebe2ad34825a0..6ba827bafa5e2 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G002.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G002.py.snap @@ -15,6 +15,24 @@ G002.py:4:27: G002 Logging statement uses `%` 3 | logging.info("Hello %s" % "World!") 4 | logging.log(logging.INFO, "Hello %s" % "World!") | ^^^^^^^^^^^^^^^^^^^^^ G002 +5 | +6 | from logging import info, log + | + +G002.py:8:6: G002 Logging statement uses `%` + | +6 | from logging import info, log +7 | +8 | info("Hello %s" % "World!") + | ^^^^^^^^^^^^^^^^^^^^^ G002 +9 | log(logging.INFO, "Hello %s" % "World!") + | + +G002.py:9:19: G002 Logging statement uses `%` + | +8 | info("Hello %s" % "World!") +9 | log(logging.INFO, "Hello %s" % "World!") + | ^^^^^^^^^^^^^^^^^^^^^ G002 | diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G003.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G003.py.snap index 873b9e74f91da..494761d268587 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G003.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G003.py.snap @@ -15,6 +15,24 @@ G003.py:4:27: G003 Logging statement uses `+` 3 | logging.info("Hello" + " " + "World!") 4 | logging.log(logging.INFO, "Hello" + " " + "World!") | ^^^^^^^^^^^^^^^^^^^^^^^^ G003 +5 | +6 | from logging import info, log + | + +G003.py:8:6: G003 Logging statement uses `+` + | +6 | from logging import info, log +7 | +8 | info("Hello" + " " + "World!") + | ^^^^^^^^^^^^^^^^^^^^^^^^ G003 +9 | log(logging.INFO, "Hello" + " " + "World!") + | + +G003.py:9:19: G003 Logging statement uses `+` + | +8 | info("Hello" + " " + "World!") +9 | log(logging.INFO, "Hello" + " " + "World!") + | ^^^^^^^^^^^^^^^^^^^^^^^^ G003 | diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap index a2d3ef00f8417..5594145d4fc34 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap @@ -20,10 +20,28 @@ G004.py:5:27: G004 Logging statement uses f-string | G004.py:8:14: G004 Logging statement uses f-string - | -7 | _LOGGER = logging.getLogger() -8 | _LOGGER.info(f"{__name__}") - | ^^^^^^^^^^^^^ G004 - | + | + 7 | _LOGGER = logging.getLogger() + 8 | _LOGGER.info(f"{__name__}") + | ^^^^^^^^^^^^^ G004 + 9 | +10 | from logging import info + | + +G004.py:11:6: G004 Logging statement uses f-string + | +10 | from logging import info +11 | info(f"{name}") + | ^^^^^^^^^ G004 +12 | info(f"{__name__}") + | + +G004.py:12:6: G004 Logging statement uses f-string + | +10 | from logging import info +11 | info(f"{name}") +12 | info(f"{__name__}") + | ^^^^^^^^^^^^^ G004 + | diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G010.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G010.py.snap index f56206e88c036..df5ab9dfa124f 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G010.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G010.py.snap @@ -41,6 +41,7 @@ G010.py:8:8: G010 [*] Logging statement uses `warn` instead of `warning` 8 |+logger.warning("Hello world!") 9 9 | 10 10 | logging . warn("Hello World!") +11 11 | G010.py:10:11: G010 [*] Logging statement uses `warn` instead of `warning` | @@ -48,6 +49,8 @@ G010.py:10:11: G010 [*] Logging statement uses `warn` instead of `warning` 9 | 10 | logging . warn("Hello World!") | ^^^^ G010 +11 | +12 | from logging import warn, warning, exception | = help: Convert to `warn` @@ -57,5 +60,27 @@ G010.py:10:11: G010 [*] Logging statement uses `warn` instead of `warning` 9 9 | 10 |-logging . warn("Hello World!") 10 |+logging . warning("Hello World!") +11 11 | +12 12 | from logging import warn, warning, exception +13 13 | warn("foo") + +G010.py:13:1: G010 [*] Logging statement uses `warn` instead of `warning` + | +12 | from logging import warn, warning, exception +13 | warn("foo") + | ^^^^ G010 +14 | warning("foo") +15 | exception("foo") + | + = help: Convert to `warn` + +ℹ Fix +10 10 | logging . warn("Hello World!") +11 11 | +12 12 | from logging import warn, warning, exception +13 |-warn("foo") +14 13 | warning("foo") + 14 |+warning("foo") +15 15 | exception("foo") diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_1.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_1.py.snap index d35d9f2bd46ba..e0fff24c03a91 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_1.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_1.py.snap @@ -11,4 +11,14 @@ G101_1.py:6:9: G101 Logging statement uses an `extra` field that clashes with a 8 | ) | +G101_1.py:15:9: G101 Logging statement uses an `extra` field that clashes with a `LogRecord` field: `name` + | +13 | "Hello world!", +14 | extra={ +15 | "name": "foobar", + | ^^^^^^ G101 +16 | }, +17 | ) + | + diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_2.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_2.py.snap index 2ea3d56ded6ad..7e5e636f87afb 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_2.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G101_2.py.snap @@ -11,4 +11,14 @@ G101_2.py:6:9: G101 Logging statement uses an `extra` field that clashes with a 8 | ) | +G101_2.py:15:9: G101 Logging statement uses an `extra` field that clashes with a `LogRecord` field: `name` + | +13 | "Hello world!", +14 | extra=dict( +15 | name="foobar", + | ^^^^^^^^^^^^^ G101 +16 | ), +17 | ) + | + diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap index a4cbafa5d2356..b0b3eb6d2b83c 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap @@ -21,4 +21,24 @@ G201.py:13:13: G201 Logging `.exception(...)` should be used instead of `.error( 15 | # OK | +G201.py:29:5: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)` + | +27 | pass +28 | except: +29 | error("Hello World", exc_info=True) + | ^^^^^ G201 +30 | +31 | try: + | + +G201.py:34:5: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)` + | +32 | pass +33 | except: +34 | error("Hello World", exc_info=sys.exc_info()) + | ^^^^^ G201 +35 | +36 | # OK + | + diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G202.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G202.py.snap index 9fc9a87608bec..17daeba81a893 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G202.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G202.py.snap @@ -21,4 +21,24 @@ G202.py:13:38: G202 Logging statement has redundant `exc_info` 15 | # OK | +G202.py:28:30: G202 Logging statement has redundant `exc_info` + | +26 | pass +27 | except: +28 | exception("Hello World", exc_info=True) + | ^^^^^^^^^^^^^ G202 +29 | +30 | try: + | + +G202.py:33:30: G202 Logging statement has redundant `exc_info` + | +31 | pass +32 | except: +33 | exception("Hello World", exc_info=sys.exc_info()) + | ^^^^^^^^^^^^^^^^^^^^^^^ G202 +34 | +35 | # OK + | + From ccce6708e86eb66b10a5ce8910195d583336aa0f Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Wed, 20 Sep 2023 16:17:37 +0200 Subject: [PATCH 04/12] cargo fmt --- .../flake8_logging/rules/exception_without_exc_info.rs | 7 +++---- .../src/rules/flake8_logging_format/rules/logging_call.rs | 8 ++++++-- 2 files changed, 9 insertions(+), 6 deletions(-) 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 318e10ef4b5ad..52ebe39e019a2 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 @@ -65,13 +65,13 @@ pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); } }; - if let Expr::Name(ast::ExprName { id, ..}) = call.func.as_ref() { + if let Expr::Name(ast::ExprName { id, .. }) = call.func.as_ref() { if id != "exception" { return; } if let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) { - if !matches!(call_path.as_slice(),["logging", "exception"]) { + if !matches!(call_path.as_slice(), ["logging", "exception"]) { return; } } @@ -85,8 +85,7 @@ pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) } fn exc_info_arg_is_falsey(call: &ExprCall, checker: &mut Checker) -> bool { - call - .arguments + call.arguments .find_keyword("exc_info") .map(|keyword| &keyword.value) .is_some_and(|value| { diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index 687786dfa2b66..98c0082bd2623 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -171,14 +171,18 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { logging_call_type = call_type; range = attr.range(); } - Expr::Name(ast::ExprName{ id , range: id_range, .. }) => { + Expr::Name(ast::ExprName { + id, + range: id_range, + .. + }) => { let Some(call_type) = LoggingCallType::from_attribute(id.as_str()) else { return; }; logging_call_type = call_type; range = *id_range; } - _ => {return} + _ => return, } // G001 - G004 From ed735440cf25992b08c8aa80aa435ea3962b99a6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 20 Sep 2023 16:29:38 -0400 Subject: [PATCH 05/12] Use match; match all imports --- .../test/fixtures/flake8_logging/LOG007.py | 6 ++ .../rules/exception_without_exc_info.rs | 66 +++++++++---------- .../rules/logging_call.rs | 30 ++++----- 3 files changed, 53 insertions(+), 49 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG007.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG007.py index 00fe85bf9159a..24a4cff872bf7 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG007.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging/LOG007.py @@ -16,3 +16,9 @@ exception("foo", exc_info=False) # LOG007 exception("foo", exc_info=True) # OK + + +exception = lambda *args, **kwargs: None + +exception("foo", exc_info=False) # OK +exception("foo", exc_info=True) # OK 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 52ebe39e019a2..d2eb8948b9a95 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,8 +1,7 @@ -use ruff_python_ast::{self as ast, Expr, ExprCall}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; 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; @@ -43,44 +42,45 @@ impl Violation for ExceptionWithoutExcInfo { /// LOG007 pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) { - if let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = call.func.as_ref() { - if !matches!( - LoggingLevel::from_attribute(attr.as_str()), - Some(LoggingLevel::Exception) - ) { - return; - } + match call.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if !matches!( + LoggingLevel::from_attribute(attr.as_str()), + Some(LoggingLevel::Exception) + ) { + return; + } - if !logging::is_logger_candidate( - &call.func, - checker.semantic(), - &checker.settings.logger_objects, - ) { - return; - } + if !logging::is_logger_candidate( + &call.func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + return; + } - if exc_info_arg_is_falsey(call, checker) { - checker - .diagnostics - .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); - } - }; - if let Expr::Name(ast::ExprName { id, .. }) = call.func.as_ref() { - if id != "exception" { - return; + if exc_info_arg_is_falsey(call, checker) { + checker + .diagnostics + .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); + } } - - if let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) { - if !matches!(call_path.as_slice(), ["logging", "exception"]) { + Expr::Name(_) => { + if !checker + .semantic() + .resolve_call_path(call.func.as_ref()) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "exception"])) + { return; } - } - if exc_info_arg_is_falsey(call, checker) { - checker - .diagnostics - .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); + if exc_info_arg_is_falsey(call, checker) { + checker + .diagnostics + .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); + } } + _ => {} } } diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index 98c0082bd2623..a526981a46487 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Keyword, Operator}; use ruff_python_semantic::analyze::logging; use ruff_python_stdlib::logging::LoggingLevel; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::registry::{AsRule, Rule}; @@ -153,10 +153,8 @@ impl LoggingCallType { /// Check logging calls for violations. pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { - let range: TextRange; - let logging_call_type; - - match call.func.as_ref() { + // Determine the call type (e.g., `info` vs. `exception`) and the range of the attribute. + let (logging_call_type, range) = match call.func.as_ref() { Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) => { let Some(call_type) = LoggingCallType::from_attribute(attr.as_str()) else { return; @@ -168,22 +166,22 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { ) { return; } - logging_call_type = call_type; - range = attr.range(); + (call_type, attr.range()) } - Expr::Name(ast::ExprName { - id, - range: id_range, - .. - }) => { - let Some(call_type) = LoggingCallType::from_attribute(id.as_str()) else { + Expr::Name(_) => { + let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) else { + return; + }; + let ["logging", attribute] = call_path.as_slice() else { + return; + }; + let Some(call_type) = LoggingCallType::from_attribute(attribute) else { return; }; - logging_call_type = call_type; - range = *id_range; + (call_type, call.func.range()) } _ => return, - } + }; // G001 - G004 let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall)); From ad98ca4ccc5915d59cf441d04c3498b61d96272b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 20 Sep 2023 16:31:02 -0400 Subject: [PATCH 06/12] Reformat test --- .../fixtures/flake8_logging_format/G201.py | 1 - ...flake8_logging_format__tests__G201.py.snap | 24 +++++++++---------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py index 4b4ec795f886c..d8610e0e26789 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G201.py @@ -20,7 +20,6 @@ logging.error("Hello World", exc_info=sys.exc_info()) - # G201 from logging import error try: diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap index b0b3eb6d2b83c..34c469ae68bbd 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G201.py.snap @@ -21,24 +21,24 @@ G201.py:13:13: G201 Logging `.exception(...)` should be used instead of `.error( 15 | # OK | -G201.py:29:5: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)` +G201.py:28:5: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)` | -27 | pass -28 | except: -29 | error("Hello World", exc_info=True) +26 | pass +27 | except: +28 | error("Hello World", exc_info=True) | ^^^^^ G201 -30 | -31 | try: +29 | +30 | try: | -G201.py:34:5: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)` +G201.py:33:5: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)` | -32 | pass -33 | except: -34 | error("Hello World", exc_info=sys.exc_info()) +31 | pass +32 | except: +33 | error("Hello World", exc_info=sys.exc_info()) | ^^^^^ G201 -35 | -36 | # OK +34 | +35 | # OK | From a702aa3478b0ae540bbc99dbebb7729eae9e292b Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 21 Sep 2023 11:59:00 +0200 Subject: [PATCH 07/12] Checker for Expr::Name in `blind_except.rs` --- .../test/fixtures/flake8_blind_except/BLE.py | 32 ++++++++++++ .../flake8_blind_except/rules/blind_except.rs | 51 ++++++++++++++----- ...e8_blind_except__tests__BLE001_BLE.py.snap | 27 ++++++++++ 3 files changed, 97 insertions(+), 13 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py b/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py index 6ce4ab3dcefbc..b2272b8ab591e 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py @@ -92,3 +92,35 @@ pass except Exception: logging.error("...", exc_info=True) + + +from logging import error, exception + +try: + pass +except Exception: + error("...") + + +try: + pass +except Exception: + error("...", exc_info=False) + + +try: + pass +except Exception: + error("...", exc_info=None) + + +try: + pass +except Exception: + exception("...") + + +try: + pass +except Exception: + error("...", exc_info=True) diff --git a/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs b/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs index 0dece92c29397..201485bee52a7 100644 --- a/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs +++ b/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs @@ -98,24 +98,49 @@ pub(crate) fn blind_except( func, arguments, .. }) = value.as_ref() { - if logging::is_logger_candidate( - func, - checker.semantic(), - &checker.settings.logger_objects, - ) { - if let Some(attribute) = func.as_attribute_expr() { - let attr = attribute.attr.as_str(); - if attr == "exception" { - return true; + match func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if logging::is_logger_candidate( + func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + let attr = attr.as_str(); + if attr == "exception" { + return true; + } + if attr == "error" { + if let Some(keyword) = arguments.find_keyword("exc_info") { + if is_const_true(&keyword.value) { + return true; + } + } + } } - if attr == "error" { - if let Some(keyword) = arguments.find_keyword("exc_info") { - if is_const_true(&keyword.value) { - return true; + } + Expr::Name(ast::ExprName { id, .. }) => { + if checker + .semantic() + .resolve_call_path(func.as_ref()) + .is_some_and(|call_path| { + matches!(call_path.as_slice(), ["logging", "error" | "exception"]) + }) + { + if id == "exception" { + return true; + } + if id == "error" { + if let Some(keyword) = arguments.find_keyword("exc_info") { + if is_const_true(&keyword.value) { + return true; + } } } } } + _ => { + return false; + } } } } diff --git a/crates/ruff_linter/src/rules/flake8_blind_except/snapshots/ruff_linter__rules__flake8_blind_except__tests__BLE001_BLE.py.snap b/crates/ruff_linter/src/rules/flake8_blind_except/snapshots/ruff_linter__rules__flake8_blind_except__tests__BLE001_BLE.py.snap index 0555d9a748f6b..16313b5f91537 100644 --- a/crates/ruff_linter/src/rules/flake8_blind_except/snapshots/ruff_linter__rules__flake8_blind_except__tests__BLE001_BLE.py.snap +++ b/crates/ruff_linter/src/rules/flake8_blind_except/snapshots/ruff_linter__rules__flake8_blind_except__tests__BLE001_BLE.py.snap @@ -94,4 +94,31 @@ BLE.py:81:8: BLE001 Do not catch blind exception: `Exception` 82 | logging.error("...", exc_info=None) | +BLE.py:101:8: BLE001 Do not catch blind exception: `Exception` + | + 99 | try: +100 | pass +101 | except Exception: + | ^^^^^^^^^ BLE001 +102 | error("...") + | + +BLE.py:107:8: BLE001 Do not catch blind exception: `Exception` + | +105 | try: +106 | pass +107 | except Exception: + | ^^^^^^^^^ BLE001 +108 | error("...", exc_info=False) + | + +BLE.py:113:8: BLE001 Do not catch blind exception: `Exception` + | +111 | try: +112 | pass +113 | except Exception: + | ^^^^^^^^^ BLE001 +114 | error("...", exc_info=None) + | + From b531f91839831004379157445866b8073319c10b Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 21 Sep 2023 12:37:32 +0200 Subject: [PATCH 08/12] Add Expr::Name check to PLE1205 and PLE1206 --- .../fixtures/pylint/logging_too_few_args.py | 26 +++++++++++++ .../fixtures/pylint/logging_too_many_args.py | 22 +++++++++++ .../src/rules/pylint/rules/logging.rs | 39 ++++++++++++------- ...sts__PLE1205_logging_too_many_args.py.snap | 20 ++++++++++ ...ests__PLE1206_logging_too_few_args.py.snap | 10 +++++ 5 files changed, 103 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_few_args.py b/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_few_args.py index 65f021ea428e1..60a7704e5c936 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_few_args.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_few_args.py @@ -26,3 +26,29 @@ import warning warning.warning("Hello %s %s", "World!") + + +from logging import error, info, warning + +warning("Hello %s %s", "World!") # [logging-too-few-args] + +# do not handle calls with kwargs (like pylint) +warning("Hello %s", "World!", "again", something="else") + +warning("Hello %s", "World!") + +# do not handle calls without any args +info("100% dynamic") + +# do not handle calls with *args +error("Example log %s, %s", "foo", "bar", "baz", *args) + +# do not handle calls with **kwargs +error("Example log %s, %s", "foo", "bar", "baz", **kwargs) + +# do not handle keyword arguments +error("%(objects)d modifications: %(modifications)d errors: %(errors)d") + +info(msg="Hello %s") + +info(msg="Hello %s %s") diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_many_args.py b/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_many_args.py index c64641eaed224..467336f8f2c4e 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_many_args.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/logging_too_many_args.py @@ -22,3 +22,25 @@ import warning warning.warning("Hello %s", "World!", "again") + + +from logging import info, error, warning + +warning("Hello %s", "World!", "again") # [logging-too-many-args] + +warning("Hello %s", "World!", "again", something="else") + +warning("Hello %s", "World!") + +# do not handle calls with *args +error("Example log %s, %s", "foo", "bar", "baz", *args) + +# do not handle calls with **kwargs +error("Example log %s, %s", "foo", "bar", "baz", **kwargs) + +# do not handle keyword arguments +error("%(objects)d modifications: %(modifications)d errors: %(errors)d", {"objects": 1, "modifications": 1, "errors": 1}) + +info(msg="Hello") + +info(msg="Hello", something="else") diff --git a/crates/ruff_linter/src/rules/pylint/rules/logging.rs b/crates/ruff_linter/src/rules/pylint/rules/logging.rs index ac1f47e68a19d..e101a5e235fc6 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/logging.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/logging.rs @@ -101,14 +101,33 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { return; } - let Expr::Attribute(ast::ExprAttribute { attr, .. }) = call.func.as_ref() else { - return; + match call.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if LoggingLevel::from_attribute(attr).is_none() { + return; + }; + if !logging::is_logger_candidate( + &call.func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + return; + } + } + Expr::Name(_) => { + let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) else { + return; + }; + let ["logging", attribute] = call_path.as_slice() else { + return; + }; + if LoggingLevel::from_attribute(attribute).is_none() { + return; + }; + } + _ => return, }; - if LoggingLevel::from_attribute(attr.as_str()).is_none() { - return; - } - let Some(Expr::Constant(ast::ExprConstant { value: Constant::Str(ast::StringConstant { value, .. }), .. @@ -117,14 +136,6 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { return; }; - if !logging::is_logger_candidate( - &call.func, - checker.semantic(), - &checker.settings.logger_objects, - ) { - return; - } - let Ok(summary) = CFormatSummary::try_from(value.as_str()) else { return; }; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap index cdbccdbd758fa..f202189a23a82 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap @@ -21,4 +21,24 @@ logging_too_many_args.py:5:1: PLE1205 Too many arguments for `logging` format st 7 | logging.warning("Hello %s", "World!") | +logging_too_many_args.py:29:1: PLE1205 Too many arguments for `logging` format string + | +27 | from logging import info, error, warning +28 | +29 | warning("Hello %s", "World!", "again") # [logging-too-many-args] + | ^^^^^^^ PLE1205 +30 | +31 | warning("Hello %s", "World!", "again", something="else") + | + +logging_too_many_args.py:31:1: PLE1205 Too many arguments for `logging` format string + | +29 | warning("Hello %s", "World!", "again") # [logging-too-many-args] +30 | +31 | warning("Hello %s", "World!", "again", something="else") + | ^^^^^^^ PLE1205 +32 | +33 | warning("Hello %s", "World!") + | + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap index b46b420741c94..abaec32b88bd9 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap @@ -11,4 +11,14 @@ logging_too_few_args.py:3:1: PLE1206 Not enough arguments for `logging` format s 5 | # do not handle calls with kwargs (like pylint) | +logging_too_few_args.py:33:1: PLE1206 Not enough arguments for `logging` format string + | +31 | from logging import error, info, warning +32 | +33 | warning("Hello %s %s", "World!") # [logging-too-few-args] + | ^^^^^^^ PLE1206 +34 | +35 | # do not handle calls with kwargs (like pylint) + | + From ec1373c69737d683c88149a9fb4723f7930eea47 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 21 Sep 2023 13:22:45 +0200 Subject: [PATCH 09/12] Add Expr::Name check to tryceratops helpers and verbose_log_message --- .../test/fixtures/tryceratops/TRY400.py | 34 +++++++ .../test/fixtures/tryceratops/TRY401.py | 64 ++++++++++++++ .../src/rules/tryceratops/helpers.rs | 25 +++++- .../tryceratops/rules/verbose_log_message.rs | 54 ++++++++---- ..._tests__verbose-log-message_TRY401.py.snap | 88 +++++++++++++++++++ 5 files changed, 246 insertions(+), 19 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY400.py b/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY400.py index 1c31197c26754..987c321907880 100644 --- a/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY400.py +++ b/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY400.py @@ -75,3 +75,37 @@ def fine(): a = 1 except Exception: logger.error("Context message here", exc_info=sys.exc_info()) + + +from logging import error, exception + + +def bad(): + try: + a = 1 + except Exception: + error("Context message here") + + if True: + error("Context message here") + + +def good(): + try: + a = 1 + except Exception: + exception("Context message here") + + +def fine(): + try: + a = 1 + except Exception: + error("Context message here", exc_info=True) + + +def fine(): + try: + a = 1 + except Exception: + error("Context message here", exc_info=sys.exc_info()) diff --git a/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY401.py b/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY401.py index b96661a68ba5c..0aa58677837fd 100644 --- a/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY401.py +++ b/crates/ruff_linter/resources/test/fixtures/tryceratops/TRY401.py @@ -62,3 +62,67 @@ def main_function(): except Exception as ex: logger.exception(f"Found an error: {er}") logger.exception(f"Found an error: {ex.field}") + + + +from logging import error, exception + + +def main_function(): + try: + process() + handle() + finish() + except Exception as ex: + exception(f"Found an error: {ex}") # TRY401 + + +def main_function(): + try: + process() + handle() + finish() + except ValueError as bad: + if True is False: + for i in range(10): + exception(f"Found an error: {bad} {good}") # TRY401 + except IndexError as bad: + exception(f"Found an error: {bad} {bad}") # TRY401 + except Exception as bad: + exception(f"Found an error: {bad}") # TRY401 + exception(f"Found an error: {bad}") # TRY401 + + if True: + exception(f"Found an error: {bad}") # TRY401 + + +def func_fstr(): + try: + ... + except Exception as ex: + exception(f"Logging an error: {ex}") # TRY401 + + +def func_concat(): + try: + ... + except Exception as ex: + exception("Logging an error: " + str(ex)) # TRY401 + + +def func_comma(): + try: + ... + except Exception as ex: + exception("Logging an error:", ex) # TRY401 + + +# OK +def main_function(): + try: + process() + handle() + finish() + except Exception as ex: + exception(f"Found an error: {er}") + exception(f"Found an error: {ex.field}") diff --git a/crates/ruff_linter/src/rules/tryceratops/helpers.rs b/crates/ruff_linter/src/rules/tryceratops/helpers.rs index 6a765ec0e3263..089e08ef3d404 100644 --- a/crates/ruff_linter/src/rules/tryceratops/helpers.rs +++ b/crates/ruff_linter/src/rules/tryceratops/helpers.rs @@ -4,6 +4,7 @@ use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; use ruff_python_semantic::analyze::logging; use ruff_python_semantic::SemanticModel; +use ruff_python_stdlib::logging::LoggingLevel; /// Collect `logging`-like calls from an AST. pub(super) struct LoggerCandidateVisitor<'a, 'b> { @@ -25,8 +26,28 @@ impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> { impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> { fn visit_expr(&mut self, expr: &'b Expr) { if let Expr::Call(call) = expr { - if logging::is_logger_candidate(&call.func, self.semantic, self.logger_objects) { - self.calls.push(call); + match call.func.as_ref() { + Expr::Attribute(_) => { + if logging::is_logger_candidate(&call.func, self.semantic, self.logger_objects) + { + self.calls.push(call); + } + } + Expr::Name(_) => { + let Some(call_path) = self.semantic.resolve_call_path(call.func.as_ref()) else { + return; + }; + let ["logging", attribute] = call_path.as_slice() else { + return; + }; + let Some(_) = LoggingLevel::from_attribute(attribute) else { + return; + }; + { + self.calls.push(call); + } + } + _ => {} } } visitor::walk_expr(self, expr); diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs index 4ddf0009b5892..fb259912c42e6 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::{self as ast, ExceptHandler, Expr}; +use ruff_python_ast::{self as ast, ExceptHandler, Expr, ExprCall}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -75,27 +75,47 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl }; for expr in calls { - if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() { - if attr == "exception" { - // Collect all referenced names in the `logging.exception` call. - let names: Vec<&ast::ExprName> = { - let mut names = Vec::new(); - for arg in &expr.arguments.args { - let mut visitor = NameVisitor::default(); - visitor.visit_expr(arg); - names.extend(visitor.names); + match expr.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if attr == "exception" { + let names = collect_referenced_names(expr); + for expr in names { + if expr.id == target.as_str() { + checker + .diagnostics + .push(Diagnostic::new(VerboseLogMessage, expr.range())); + } } - names - }; - for expr in names { - if expr.id == target.as_str() { - checker - .diagnostics - .push(Diagnostic::new(VerboseLogMessage, expr.range())); + } + } + Expr::Name(ast::ExprName { id, .. }) => { + if id.as_str() == "exception" { + let names = collect_referenced_names(expr); + for expr in names { + if expr.id == target.as_str() { + checker + .diagnostics + .push(Diagnostic::new(VerboseLogMessage, expr.range())); + } } } } + _ => {} } } } } + +fn collect_referenced_names(expr: &ExprCall) -> Vec<&ast::ExprName> { + // Collect all referenced names in the `logging.exception` call. + let names: Vec<&ast::ExprName> = { + let mut names = Vec::new(); + for arg in &expr.arguments.args { + let mut visitor = NameVisitor::default(); + visitor.visit_expr(arg); + names.extend(visitor.names); + } + names + }; + names +} diff --git a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap index 7143686b8708c..3681b880d1f06 100644 --- a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap +++ b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap @@ -89,4 +89,92 @@ TRY401.py:53:47: TRY401 Redundant exception object included in `logging.exceptio | ^^ TRY401 | +TRY401.py:77:38: TRY401 Redundant exception object included in `logging.exception` call + | +75 | finish() +76 | except Exception as ex: +77 | exception(f"Found an error: {ex}") # TRY401 + | ^^ TRY401 + | + +TRY401.py:88:46: TRY401 Redundant exception object included in `logging.exception` call + | +86 | if True is False: +87 | for i in range(10): +88 | exception(f"Found an error: {bad} {good}") # TRY401 + | ^^^ TRY401 +89 | except IndexError as bad: +90 | exception(f"Found an error: {bad} {bad}") # TRY401 + | + +TRY401.py:90:38: TRY401 Redundant exception object included in `logging.exception` call + | +88 | exception(f"Found an error: {bad} {good}") # TRY401 +89 | except IndexError as bad: +90 | exception(f"Found an error: {bad} {bad}") # TRY401 + | ^^^ TRY401 +91 | except Exception as bad: +92 | exception(f"Found an error: {bad}") # TRY401 + | + +TRY401.py:90:44: TRY401 Redundant exception object included in `logging.exception` call + | +88 | exception(f"Found an error: {bad} {good}") # TRY401 +89 | except IndexError as bad: +90 | exception(f"Found an error: {bad} {bad}") # TRY401 + | ^^^ TRY401 +91 | except Exception as bad: +92 | exception(f"Found an error: {bad}") # TRY401 + | + +TRY401.py:92:38: TRY401 Redundant exception object included in `logging.exception` call + | +90 | exception(f"Found an error: {bad} {bad}") # TRY401 +91 | except Exception as bad: +92 | exception(f"Found an error: {bad}") # TRY401 + | ^^^ TRY401 +93 | exception(f"Found an error: {bad}") # TRY401 + | + +TRY401.py:93:38: TRY401 Redundant exception object included in `logging.exception` call + | +91 | except Exception as bad: +92 | exception(f"Found an error: {bad}") # TRY401 +93 | exception(f"Found an error: {bad}") # TRY401 + | ^^^ TRY401 +94 | +95 | if True: + | + +TRY401.py:96:42: TRY401 Redundant exception object included in `logging.exception` call + | +95 | if True: +96 | exception(f"Found an error: {bad}") # TRY401 + | ^^^ TRY401 + | + +TRY401.py:103:40: TRY401 Redundant exception object included in `logging.exception` call + | +101 | ... +102 | except Exception as ex: +103 | exception(f"Logging an error: {ex}") # TRY401 + | ^^ TRY401 + | + +TRY401.py:110:46: TRY401 Redundant exception object included in `logging.exception` call + | +108 | ... +109 | except Exception as ex: +110 | exception("Logging an error: " + str(ex)) # TRY401 + | ^^ TRY401 + | + +TRY401.py:117:40: TRY401 Redundant exception object included in `logging.exception` call + | +115 | ... +116 | except Exception as ex: +117 | exception("Logging an error:", ex) # TRY401 + | ^^ TRY401 + | + From 28c36a980f3f8adb165f692e21093fbf525ae740 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 21 Sep 2023 13:30:48 +0200 Subject: [PATCH 10/12] Add Expr::Name check to `error_instead_of_exception` --- .../src/rules/tryceratops/helpers.rs | 3 ++- .../rules/error_instead_of_exception.rs | 18 +++++++++++------- ...__error-instead-of-exception_TRY400.py.snap | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/crates/ruff_linter/src/rules/tryceratops/helpers.rs b/crates/ruff_linter/src/rules/tryceratops/helpers.rs index 089e08ef3d404..4287fc7bd7c1b 100644 --- a/crates/ruff_linter/src/rules/tryceratops/helpers.rs +++ b/crates/ruff_linter/src/rules/tryceratops/helpers.rs @@ -34,7 +34,8 @@ impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> { } } Expr::Name(_) => { - let Some(call_path) = self.semantic.resolve_call_path(call.func.as_ref()) else { + let Some(call_path) = self.semantic.resolve_call_path(call.func.as_ref()) + else { return; }; let ["logging", attribute] = call_path.as_slice() else { diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs index 2d0404a50406a..9a8f8109ae1a7 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs @@ -1,4 +1,5 @@ use ruff_python_ast::{self as ast, ExceptHandler, Expr}; +use similar::DiffableStr; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -65,13 +66,16 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce visitor.calls }; for expr in calls { - if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() { - if attr == "error" { - if exc_info(&expr.arguments, checker.semantic()).is_none() { - checker - .diagnostics - .push(Diagnostic::new(ErrorInsteadOfException, expr.range())); - } + let identifier: &str = match expr.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => attr.as_str(), + Expr::Name(ast::ExprName { id, .. }) => id, + _ => return, + }; + if identifier == "error" { + if exc_info(&expr.arguments, checker.semantic()).is_none() { + checker + .diagnostics + .push(Diagnostic::new(ErrorInsteadOfException, expr.range())); } } } diff --git a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap index 4eb4aad7a1767..35b36493ec3a9 100644 --- a/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap +++ b/crates/ruff_linter/src/rules/tryceratops/snapshots/ruff_linter__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap @@ -69,4 +69,21 @@ TRY400.py:49:13: TRY400 Use `logging.exception` instead of `logging.error` | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 | +TRY400.py:87:9: TRY400 Use `logging.exception` instead of `logging.error` + | +85 | a = 1 +86 | except Exception: +87 | error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 +88 | +89 | if True: + | + +TRY400.py:90:13: TRY400 Use `logging.exception` instead of `logging.error` + | +89 | if True: +90 | error("Context message here") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400 + | + From e89ba8f0a37dc1c352b67f32b80116bcb0c37aa2 Mon Sep 17 00:00:00 2001 From: qdegraaf Date: Thu, 21 Sep 2023 13:34:54 +0200 Subject: [PATCH 11/12] refactor --- .../rules/error_instead_of_exception.rs | 1 - .../tryceratops/rules/verbose_log_message.rs | 59 +++++++------------ 2 files changed, 21 insertions(+), 39 deletions(-) diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs index 9a8f8109ae1a7..f8394b503731c 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs @@ -1,5 +1,4 @@ use ruff_python_ast::{self as ast, ExceptHandler, Expr}; -use similar::DiffableStr; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs index fb259912c42e6..d0a71d931bd45 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::{self as ast, ExceptHandler, Expr, ExprCall}; +use ruff_python_ast::{self as ast, ExceptHandler, Expr}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -75,47 +75,30 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl }; for expr in calls { - match expr.func.as_ref() { - Expr::Attribute(ast::ExprAttribute { attr, .. }) => { - if attr == "exception" { - let names = collect_referenced_names(expr); - for expr in names { - if expr.id == target.as_str() { - checker - .diagnostics - .push(Diagnostic::new(VerboseLogMessage, expr.range())); - } - } + let identifier = match expr.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => attr.as_str(), + Expr::Name(ast::ExprName { id, .. }) => id, + _ => return, + }; + if identifier == "exception" { + // Collect all referenced names in the `logging.exception` call. + let names: Vec<&ast::ExprName> = { + let mut names = Vec::new(); + for arg in &expr.arguments.args { + let mut visitor = NameVisitor::default(); + visitor.visit_expr(arg); + names.extend(visitor.names); } - } - Expr::Name(ast::ExprName { id, .. }) => { - if id.as_str() == "exception" { - let names = collect_referenced_names(expr); - for expr in names { - if expr.id == target.as_str() { - checker - .diagnostics - .push(Diagnostic::new(VerboseLogMessage, expr.range())); - } - } + names + }; + for expr in names { + if expr.id == target.as_str() { + checker + .diagnostics + .push(Diagnostic::new(VerboseLogMessage, expr.range())); } } - _ => {} } } } } - -fn collect_referenced_names(expr: &ExprCall) -> Vec<&ast::ExprName> { - // Collect all referenced names in the `logging.exception` call. - let names: Vec<&ast::ExprName> = { - let mut names = Vec::new(); - for arg in &expr.arguments.args { - let mut visitor = NameVisitor::default(); - visitor.visit_expr(arg); - names.extend(visitor.names); - } - names - }; - names -} From 6ef3b5af9186fdd04dd5cf9891c487c7c9652944 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 26 Sep 2023 20:01:15 -0400 Subject: [PATCH 12/12] Use LoggingLevel --- .../flake8_blind_except/rules/blind_except.rs | 42 +++++++++---------- .../rules/exception_without_exc_info.rs | 20 ++++----- .../src/rules/tryceratops/helpers.rs | 31 +++++++------- .../rules/error_instead_of_exception.rs | 13 ++---- .../tryceratops/rules/verbose_log_message.rs | 10 ++--- .../src/analyze/logging.rs | 1 + 6 files changed, 50 insertions(+), 67 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs b/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs index 201485bee52a7..102f40be9913e 100644 --- a/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs +++ b/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs @@ -105,37 +105,37 @@ pub(crate) fn blind_except( checker.semantic(), &checker.settings.logger_objects, ) { - let attr = attr.as_str(); - if attr == "exception" { - return true; - } - if attr == "error" { - if let Some(keyword) = arguments.find_keyword("exc_info") { - if is_const_true(&keyword.value) { - return true; + match attr.as_str() { + "exception" => return true, + "error" => { + if let Some(keyword) = arguments.find_keyword("exc_info") { + if is_const_true(&keyword.value) { + return true; + } } } + _ => {} } } } - Expr::Name(ast::ExprName { id, .. }) => { + Expr::Name(ast::ExprName { .. }) => { if checker .semantic() .resolve_call_path(func.as_ref()) - .is_some_and(|call_path| { - matches!(call_path.as_slice(), ["logging", "error" | "exception"]) - }) - { - if id == "exception" { - return true; - } - if id == "error" { - if let Some(keyword) = arguments.find_keyword("exc_info") { - if is_const_true(&keyword.value) { - return true; + .is_some_and(|call_path| match call_path.as_slice() { + ["logging", "exception"] => true, + ["logging", "error"] => { + if let Some(keyword) = arguments.find_keyword("exc_info") { + if is_const_true(&keyword.value) { + return true; + } } + false } - } + _ => false, + }) + { + return 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 d2eb8948b9a95..d3eac436387f5 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 @@ -58,12 +58,6 @@ pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) ) { return; } - - if exc_info_arg_is_falsey(call, checker) { - checker - .diagnostics - .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); - } } Expr::Name(_) => { if !checker @@ -73,14 +67,14 @@ pub(crate) fn exception_without_exc_info(checker: &mut Checker, call: &ExprCall) { return; } - - if exc_info_arg_is_falsey(call, checker) { - checker - .diagnostics - .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); - } } - _ => {} + _ => return, + } + + if exc_info_arg_is_falsey(call, checker) { + checker + .diagnostics + .push(Diagnostic::new(ExceptionWithoutExcInfo, call.range())); } } diff --git a/crates/ruff_linter/src/rules/tryceratops/helpers.rs b/crates/ruff_linter/src/rules/tryceratops/helpers.rs index 4287fc7bd7c1b..0e707c1e3924e 100644 --- a/crates/ruff_linter/src/rules/tryceratops/helpers.rs +++ b/crates/ruff_linter/src/rules/tryceratops/helpers.rs @@ -1,7 +1,6 @@ -use ruff_python_ast::{self as ast, Expr}; - use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{self as ast, Expr}; use ruff_python_semantic::analyze::logging; use ruff_python_semantic::SemanticModel; use ruff_python_stdlib::logging::LoggingLevel; @@ -10,7 +9,7 @@ use ruff_python_stdlib::logging::LoggingLevel; pub(super) struct LoggerCandidateVisitor<'a, 'b> { semantic: &'a SemanticModel<'b>, logger_objects: &'a [String], - pub(super) calls: Vec<&'b ast::ExprCall>, + pub(super) calls: Vec<(&'b ast::ExprCall, LoggingLevel)>, } impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> { @@ -27,25 +26,23 @@ impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> { fn visit_expr(&mut self, expr: &'b Expr) { if let Expr::Call(call) = expr { match call.func.as_ref() { - Expr::Attribute(_) => { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { if logging::is_logger_candidate(&call.func, self.semantic, self.logger_objects) { - self.calls.push(call); + if let Some(logging_level) = LoggingLevel::from_attribute(attr) { + self.calls.push((call, logging_level)); + }; } } Expr::Name(_) => { - let Some(call_path) = self.semantic.resolve_call_path(call.func.as_ref()) - else { - return; - }; - let ["logging", attribute] = call_path.as_slice() else { - return; - }; - let Some(_) = LoggingLevel::from_attribute(attribute) else { - return; - }; - { - self.calls.push(call); + if let Some(call_path) = self.semantic.resolve_call_path(call.func.as_ref()) { + if let ["logging", attribute] = call_path.as_slice() { + if let Some(logging_level) = LoggingLevel::from_attribute(attribute) { + { + self.calls.push((call, logging_level)); + } + } + } } } _ => {} diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs index f8394b503731c..fda19f79e8b58 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/error_instead_of_exception.rs @@ -1,9 +1,9 @@ -use ruff_python_ast::{self as ast, ExceptHandler, Expr}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{self as ast, ExceptHandler}; use ruff_python_semantic::analyze::logging::exc_info; +use ruff_python_stdlib::logging::LoggingLevel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -64,13 +64,8 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce visitor.visit_body(body); visitor.calls }; - for expr in calls { - let identifier: &str = match expr.func.as_ref() { - Expr::Attribute(ast::ExprAttribute { attr, .. }) => attr.as_str(), - Expr::Name(ast::ExprName { id, .. }) => id, - _ => return, - }; - if identifier == "error" { + for (expr, logging_level) in calls { + if matches!(logging_level, LoggingLevel::Error) { if exc_info(&expr.arguments, checker.semantic()).is_none() { checker .diagnostics diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs index d0a71d931bd45..8b3fad0f082d9 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_log_message.rs @@ -4,6 +4,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; +use ruff_python_stdlib::logging::LoggingLevel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -74,13 +75,8 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl visitor.calls }; - for expr in calls { - let identifier = match expr.func.as_ref() { - Expr::Attribute(ast::ExprAttribute { attr, .. }) => attr.as_str(), - Expr::Name(ast::ExprName { id, .. }) => id, - _ => return, - }; - if identifier == "exception" { + for (expr, logging_level) in calls { + if matches!(logging_level, LoggingLevel::Exception) { // Collect all referenced names in the `logging.exception` call. let names: Vec<&ast::ExprName> = { let mut names = Vec::new(); diff --git a/crates/ruff_python_semantic/src/analyze/logging.rs b/crates/ruff_python_semantic/src/analyze/logging.rs index 82b9bf2a1522b..205e3bb8e4c3b 100644 --- a/crates/ruff_python_semantic/src/analyze/logging.rs +++ b/crates/ruff_python_semantic/src/analyze/logging.rs @@ -24,6 +24,7 @@ pub fn is_logger_candidate( let Expr::Attribute(ast::ExprAttribute { value, .. }) = func else { return false; }; + // If the symbol was imported from another module, ensure that it's either a user-specified // logger object, the `logging` module itself, or `flask.current_app.logger`. if let Some(call_path) = semantic.resolve_call_path(value) {