Skip to content

Commit

Permalink
Add Expr::Name checks to rules which use is_logger_candidate (#7521)
Browse files Browse the repository at this point in the history
## Summary

Expands several rules to also check for `Expr::Name` values. As they
would previously not consider:
```python
from logging import error

error("foo")
```
as potential violations
```python
import logging

logging.error("foo")
```
as potential violations leading to inconsistent behaviour. 

The rules impacted are:

- `BLE001`
- `TRY400`
- `TRY401`
- `PLE1205`
- `PLE1206`
- `LOG007`
- `G001`-`G004`
- `G101`
- `G201`
- `G202`

## Test Plan

Fixtures for all impacted rules expanded. 

## Issue Link

Refers: #7502
  • Loading branch information
qdegraaf authored Sep 27, 2023
1 parent 528f386 commit 2aef46c
Show file tree
Hide file tree
Showing 37 changed files with 823 additions and 107 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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)
Original file line number Diff line number Diff line change
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!")
Original file line number Diff line number Diff line change
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!")
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@

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

from logging import info
info(f"{name}")
info(f"{__name__}")
Original file line number Diff line number Diff line change
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")
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,12 @@
"name": "foobar",
},
)

from logging import info

info(
"Hello world!",
extra={
"name": "foobar",
},
)
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,12 @@
name="foobar",
),
)

from logging import info

info(
"Hello world!",
extra=dict(
name="foobar",
),
)
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,24 @@
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())

Original file line number Diff line number Diff line change
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)
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Original file line number Diff line number Diff line change
Expand Up @@ -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")
34 changes: 34 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/tryceratops/TRY400.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())
64 changes: 64 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/tryceratops/TRY401.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
if attr == "error" {
if let Some(keyword) = arguments.find_keyword("exc_info") {
if is_const_true(&keyword.value) {
return true;
match func.as_ref() {
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
if logging::is_logger_candidate(
func,
checker.semantic(),
&checker.settings.logger_objects,
) {
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 { .. }) => {
if checker
.semantic()
.resolve_call_path(func.as_ref())
.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;
}
}
_ => {
return false;
}
}
}
}
Expand Down
Loading

0 comments on commit 2aef46c

Please sign in to comment.