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

Add Expr::Name checks to rules which use is_logger_candidate #7521

Merged
merged 14 commits into from Sep 27, 2023
Merged
Expand Up @@ -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
Expand Up @@ -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)
Expand Up @@ -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!")
Expand Up @@ -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!")
Expand Up @@ -6,3 +6,7 @@

_LOGGER = logging.getLogger()
_LOGGER.info(f"{__name__}")

from logging import info
info(f"{name}")
info(f"{__name__}")
Expand Up @@ -8,3 +8,8 @@
logger.warn("Hello world!")

logging . warn("Hello World!")

from logging import warn, warning, exception
warn("foo")
warning("foo")
exception("foo")
Expand Up @@ -6,3 +6,12 @@
"name": "foobar",
},
)

from logging import info

info(
"Hello world!",
extra={
"name": "foobar",
},
)
Expand Up @@ -6,3 +6,12 @@
name="foobar",
),
)

from logging import info

info(
"Hello world!",
extra=dict(
name="foobar",
),
)
Expand Up @@ -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())

Expand Up @@ -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)
@@ -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;
Expand Down Expand Up @@ -43,35 +42,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;
};
match call.func.as_ref() {
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
if !matches!(
LoggingLevel::from_attribute(attr.as_str()),
Some(LoggingLevel::Exception)
) {
return;
}

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()));
}
}
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 call
.arguments
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()));
}
}
Expand Up @@ -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
|


Expand Up @@ -153,22 +153,36 @@ 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 Some(logging_call_type) = LoggingCallType::from_attribute(attr.as_str()) else {
return;
// 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;
};
if !logging::is_logger_candidate(
&call.func,
checker.semantic(),
&checker.settings.logger_objects,
) {
return;
}
(call_type, attr.range())
}
Expr::Name(_) => {
let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) else {
qdegraaf marked this conversation as resolved.
Show resolved Hide resolved
return;
};
let ["logging", attribute] = call_path.as_slice() else {
return;
};
let Some(call_type) = LoggingCallType::from_attribute(attribute) else {
return;
};
(call_type, call.func.range())
}
_ => return,
};

if !logging::is_logger_candidate(
&call.func,
checker.semantic(),
&checker.settings.logger_objects,
) {
return;
}

// G001 - G004
let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall));
if let Some(format_arg) = call.arguments.find_argument("msg", msg_pos) {
Expand All @@ -181,11 +195,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);
Expand Down Expand Up @@ -213,7 +227,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 => {
Expand Down
Expand Up @@ -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
|


Expand Up @@ -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
|